Move permission checks to global VS
When a client sends a request to virtmgr, it checks the client's
corresponding permission. Handling the request may later require calling
into VirtualizationServiceInternal (global VS), at which point no more
checks are performed as the only domain allowed to call it is virtmgr.
We can make this a little neater by moving the permission check to
the global VS and let virtmgr propagate the error if it fails. This way
we do not need to assume virtmgr is the only domain that calls VS.
This works for:
* create VM => check MANAGE_VIRTUAL_MACHINE in VS when VM context is
allocated
* debug list VMs => check DEBUG_VIRTUAL_MACHINE in VS
On the other hand, it does not work for USE_CUSTOM_VIRTUAL_MACHINE,
which can only be checked in virtmgr.
We also keep checking MANAGE_VIRTUAL_MACHINE for virtmgr operations that
create idsig/instance.img/encrypted store. These do not perform any
privileged operations but we keep them for API consistency.
Bug: 245727626
Test: atest -p packages/modules/Virtualization:avf-presubmit
Change-Id: I5bc5e73ec72173d5422f27cec91b6227a11f744d
diff --git a/virtualizationservice/src/aidl.rs b/virtualizationservice/src/aidl.rs
index 374b90f..df8ad84 100644
--- a/virtualizationservice/src/aidl.rs
+++ b/virtualizationservice/src/aidl.rs
@@ -193,6 +193,8 @@
&self,
requester_debug_pid: i32,
) -> binder::Result<Strong<dyn IGlobalVmContext>> {
+ check_manage_access()?;
+
let requester_uid = get_calling_uid();
let requester_debug_pid = requester_debug_pid as pid_t;
let state = &mut *self.state.lock().unwrap();
@@ -217,6 +219,8 @@
}
fn debugListVms(&self) -> binder::Result<Vec<VirtualMachineDebugInfo>> {
+ check_debug_access()?;
+
let state = &mut *self.state.lock().unwrap();
let cids = state
.held_contexts
@@ -499,7 +503,7 @@
/// Get a list of all currently running VMs. This method is only intended for debug purposes,
/// and as such is only permitted from the shell user.
fn debugListVms(&self) -> binder::Result<Vec<VirtualMachineDebugInfo>> {
- check_debug_access()?;
+ // Delegate to the global service, including checking the debug permission.
GLOBAL_SERVICE.debugListVms()
}
}
@@ -562,7 +566,10 @@
VirtualizationService::default()
}
- fn create_vm_context(&self, requester_debug_pid: pid_t) -> Result<(VmContext, Cid, PathBuf)> {
+ fn create_vm_context(
+ &self,
+ requester_debug_pid: pid_t,
+ ) -> binder::Result<(VmContext, Cid, PathBuf)> {
const NUM_ATTEMPTS: usize = 5;
for _ in 0..NUM_ATTEMPTS {
@@ -583,7 +590,10 @@
}
}
}
- bail!("Too many attempts to create VM context failed.");
+ Err(Status::new_service_specific_error_str(
+ -1,
+ Some("Too many attempts to create VM context failed."),
+ ))
}
fn create_vm_internal(
@@ -593,7 +603,11 @@
log_fd: Option<&ParcelFileDescriptor>,
is_protected: &mut bool,
) -> binder::Result<Strong<dyn IVirtualMachine>> {
- check_manage_access()?;
+ let requester_uid = get_calling_uid();
+ let requester_debug_pid = get_calling_pid();
+
+ // Allocating VM context checks the MANAGE_VIRTUAL_MACHINE permission.
+ let (vm_context, cid, temporary_directory) = self.create_vm_context(requester_debug_pid)?;
let is_custom = match config {
VirtualMachineConfig::RawConfig(_) => true,
@@ -609,18 +623,6 @@
check_use_custom_virtual_machine()?;
}
- let requester_uid = get_calling_uid();
- let requester_debug_pid = get_calling_pid();
-
- let (vm_context, cid, temporary_directory) =
- self.create_vm_context(requester_debug_pid).map_err(|e| {
- error!("Failed to create VmContext: {:?}", e);
- Status::new_service_specific_error_str(
- -1,
- Some(format!("Failed to create VmContext: {:?}", e)),
- )
- })?;
-
let state = &mut *self.state.lock().unwrap();
let console_fd = console_fd.map(clone_file).transpose()?;
let log_fd = log_fd.map(clone_file).transpose()?;