From ce10855e915b5d28a4024ad761593c3bdf2ade3b Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Mon, 1 Mar 2021 17:52:47 +0000 Subject: [PATCH] enable_raw_capability and kvm_enable_cap are unsafe The args may be interpreted as pointers for some capabilities, so the caller must ensure that any such pointers are allocated appropriately. BUG=b:181564686 TEST=cargo test Change-Id: I244f4d9417e588a6be5681f4718bb9ad7b262c3e Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2726709 Auto-Submit: Andrew Walbran Commit-Queue: Dylan Reid Tested-by: kokoro Reviewed-by: Dylan Reid --- hypervisor/src/kvm/mod.rs | 17 +++++++++++------ hypervisor/src/kvm/x86_64.rs | 3 +-- hypervisor/src/lib.rs | 7 ++++++- kvm/src/lib.rs | 22 +++++++++++++--------- src/plugin/vcpu.rs | 3 ++- 5 files changed, 33 insertions(+), 19 deletions(-) diff --git a/hypervisor/src/kvm/mod.rs b/hypervisor/src/kvm/mod.rs index 49b6af6c01..24955c2872 100644 --- a/hypervisor/src/kvm/mod.rs +++ b/hypervisor/src/kvm/mod.rs @@ -386,6 +386,11 @@ impl KvmVm { // Currently only used on aarch64, but works on any architecture. #[allow(dead_code)] /// Enables a KVM-specific capability for this VM, with the given arguments. + /// + /// # Safety + /// This function is marked as unsafe because `args` may be interpreted as pointers for some + /// capabilities. The caller must ensure that any pointers passed in the `args` array are + /// allocated as the kernel expects, and that mutable pointers are owned. unsafe fn enable_raw_capability( &self, capability: KvmCap, @@ -398,8 +403,8 @@ impl KvmVm { flags, ..Default::default() }; - // Safe because we allocated the struct and we know the kernel will read - // exactly the size of the struct. + // Safe because we allocated the struct and we know the kernel will read exactly the size of + // the struct, and because we assume the caller has allocated the args appropriately. let ret = ioctl_with_ref(self, KVM_ENABLE_CAP(), &kvm_cap); if ret == 0 { Ok(()) @@ -842,15 +847,15 @@ impl Vcpu for KvmVcpu { } } - fn enable_raw_capability(&self, cap: u32, args: &[u64; 4]) -> Result<()> { + unsafe fn enable_raw_capability(&self, cap: u32, args: &[u64; 4]) -> Result<()> { let kvm_cap = kvm_enable_cap { cap, args: *args, ..Default::default() }; - // Safe because we allocated the struct and we know the kernel will read - // exactly the size of the struct. - let ret = unsafe { ioctl_with_ref(self, KVM_ENABLE_CAP(), &kvm_cap) }; + // Safe because we allocated the struct and we know the kernel will read exactly the size of + // the struct, and because we assume the caller has allocated the args appropriately. + let ret = ioctl_with_ref(self, KVM_ENABLE_CAP(), &kvm_cap); if ret == 0 { Ok(()) } else { diff --git a/hypervisor/src/kvm/x86_64.rs b/hypervisor/src/kvm/x86_64.rs index 96b99d3134..b43a02d789 100644 --- a/hypervisor/src/kvm/x86_64.rs +++ b/hypervisor/src/kvm/x86_64.rs @@ -1479,8 +1479,7 @@ mod tests { let vm = KvmVm::new(&kvm, gm).unwrap(); vm.create_irq_chip().unwrap(); let vcpu = vm.create_vcpu(0).unwrap(); - vcpu.enable_raw_capability(kvm_sys::KVM_CAP_HYPERV_SYNIC, &[0; 4]) - .unwrap(); + unsafe { vcpu.enable_raw_capability(kvm_sys::KVM_CAP_HYPERV_SYNIC, &[0; 4]) }.unwrap(); } #[test] diff --git a/hypervisor/src/lib.rs b/hypervisor/src/lib.rs index 1fced1c6ce..cb534114d2 100644 --- a/hypervisor/src/lib.rs +++ b/hypervisor/src/lib.rs @@ -270,7 +270,12 @@ pub trait Vcpu: downcast_rs::DowncastSync { /// Enables a hypervisor-specific extension on this Vcpu. `cap` is a constant defined by the /// hypervisor API (e.g., kvm.h). `args` are the arguments for enabling the feature, if any. - fn enable_raw_capability(&self, cap: u32, args: &[u64; 4]) -> Result<()>; + /// + /// # Safety + /// This function is marked as unsafe because `args` may be interpreted as pointers for some + /// capabilities. The caller must ensure that any pointers passed in the `args` array are + /// allocated as the kernel expects, and that mutable pointers are owned. + unsafe fn enable_raw_capability(&self, cap: u32, args: &[u64; 4]) -> Result<()>; } downcast_rs::impl_downcast!(sync Vcpu); diff --git a/kvm/src/lib.rs b/kvm/src/lib.rs index 7d8c74ad45..357020af2f 100644 --- a/kvm/src/lib.rs +++ b/kvm/src/lib.rs @@ -834,10 +834,12 @@ impl Vm { /// Enable the specified capability. /// See documentation for KVM_ENABLE_CAP. - pub fn kvm_enable_cap(&self, cap: &kvm_enable_cap) -> Result<()> { - // safe becuase we allocated the struct and we know the kernel will read - // exactly the size of the struct - let ret = unsafe { ioctl_with_ref(self, KVM_ENABLE_CAP(), cap) }; + /// This function is marked as unsafe because `cap` may contain values which are interpreted as + /// pointers by the kernel. + pub unsafe fn kvm_enable_cap(&self, cap: &kvm_enable_cap) -> Result<()> { + // Safe because we allocated the struct and we know the kernel will read exactly the size of + // the struct. + let ret = ioctl_with_ref(self, KVM_ENABLE_CAP(), cap); if ret < 0 { errno_result() } else { @@ -1403,10 +1405,12 @@ impl Vcpu { /// Enable the specified capability. /// See documentation for KVM_ENABLE_CAP. - pub fn kvm_enable_cap(&self, cap: &kvm_enable_cap) -> Result<()> { - // safe becuase we allocated the struct and we know the kernel will read - // exactly the size of the struct - let ret = unsafe { ioctl_with_ref(self, KVM_ENABLE_CAP(), cap) }; + /// This function is marked as unsafe because `cap` may contain values which are interpreted as + /// pointers by the kernel. + pub unsafe fn kvm_enable_cap(&self, cap: &kvm_enable_cap) -> Result<()> { + // Safe because we allocated the struct and we know the kernel will read exactly the size of + // the struct. + let ret = ioctl_with_ref(self, KVM_ENABLE_CAP(), cap); if ret < 0 { return errno_result(); } @@ -2081,7 +2085,7 @@ mod tests { let vcpu = Vcpu::new(0, &kvm, &vm).unwrap(); let mut cap: kvm_enable_cap = Default::default(); cap.cap = kvm_sys::KVM_CAP_HYPERV_SYNIC; - vcpu.kvm_enable_cap(&cap).unwrap(); + unsafe { vcpu.kvm_enable_cap(&cap) }.unwrap(); } #[test] diff --git a/src/plugin/vcpu.rs b/src/plugin/vcpu.rs index 9438bb21a5..535ee1c394 100644 --- a/src/plugin/vcpu.rs +++ b/src/plugin/vcpu.rs @@ -729,7 +729,8 @@ impl PluginVcpu { } else { let mut cap: kvm_enable_cap = Default::default(); cap.cap = capability; - vcpu.kvm_enable_cap(&cap) + // Safe because the allowed capabilities don't take pointer arguments. + unsafe { vcpu.kvm_enable_cap(&cap) } } } else if request.has_shutdown() { return Err(SysError::new(EPIPE));