xen-ioctls: add SAFETY docs to unsafe {} blocks

Signed-off-by: Manos Pitsidianakis <[email protected]>

Author: epilys

xen-ioctls/src/domctl/domctl.rs

@@ -10,36 +10,35 @@
 
 use std::convert::TryFrom;
 
-use libc::c_void;
-
 use crate::{
     domctl::{types::*, xc_types::XcDominfo},
     private::*,
 };
 
 fn do_domctl(xen_domctl: &mut XenDomctl) -> Result<(), std::io::Error> {
     let bouncebuffer = BounceBuffer::new(std::mem::size_of::<XenDomctl>())?;
-    let vaddr = bouncebuffer.to_vaddr() as *mut XenDomctl;
-    let mut privcmd_hypercall = PrivCmdHypercall {
+    let vaddr = bouncebuffer.vaddr() as *mut XenDomctl;
+    let mut privcmd = PrivCmdHypercall {
         op: __HYPERVISOR_DOMCTL,
         arg: [vaddr as u64, 0, 0, 0, 0],
     };
-    /*
-     * The expression "&mut privcmd_hypercall as *mut _" creates a reference
-     * to privcmd_hypercall before casting it to a *mut c_void
-     */
-    let privcmd_ptr: *mut c_void = &mut privcmd_hypercall as *mut _ as *mut c_void;
+    // Write content of XenDomctl to the bounce buffer so that Xen knows what
+    // we are asking for.
+    // SAFETY: vaddr is a valid bounce buffer of XenDomctl size
+    unsafe { vaddr.write(*xen_domctl) };
 
+    // SAFETY: we pass a PrivCmdHypercall sized value to an IOCTL_PRIVCMD_HYPERCALL
+    // ioctl
     unsafe {
-        // Write content of XenDomctl to the bounce buffer so that Xen knows what
-        // we are asking for.
-        vaddr.write(*xen_domctl);
-
-        do_ioctl(IOCTL_PRIVCMD_HYPERCALL(), privcmd_ptr).map(|_| {
-            // Read back content from bounce buffer if no errors.
-            *xen_domctl = vaddr.read();
-        })
-    }
+        do_ioctl(
+            IOCTL_PRIVCMD_HYPERCALL(),
+            std::ptr::addr_of_mut!(privcmd).cast(),
+        )?
+    };
+    // Read back content from bounce buffer if no errors.
+    // SAFETY: vaddr is a valid bounce buffer of XenDomctl size
+    *xen_domctl = unsafe { vaddr.read() };
+    Ok(())
 }
 
 pub fn xc_domain_info(first_domain: u16, max_domain: u32) -> Vec<XcDominfo> {
@@ -59,7 +58,10 @@ pub fn xc_domain_info(first_domain: u16, max_domain: u32) -> Vec<XcDominfo> {
 
         match do_domctl(&mut domctl) {
             Ok(()) => {
-                if let Ok(dominfo) = XcDominfo::try_from(unsafe { domctl.u.domaininfo }) {
+                if let Ok(dominfo) = XcDominfo::try_from(
+                    // SAFETY: domctl was successful and the union is a XenDomctlPayload variant
+                    unsafe { domctl.u.domaininfo },
+                ) {
                     vec.push(dominfo);
                 }
             }

xen-ioctls/src/private.rs

@@ -120,50 +120,56 @@ impl BounceBuffer {
             .write(true)
             .open(HYPERCALL_BUFFER_FILE)?;
 
-        unsafe {
-            // Setup a bounce buffer for Xen to use.
-            let vaddr = mmap(
+        // Setup a bounce buffer for Xen to use.
+        // SAFETY: `fd is a valid HYPERCALL_BUFFER_FILE descriptor
+        let vaddr = unsafe {
+            mmap(
                 core::ptr::null_mut::<c_void>(),
                 bounce_buffer_size,
                 PROT_READ | PROT_WRITE,
                 MAP_SHARED,
                 fd.as_raw_fd(),
                 0,
-            ) as *mut u8;
-
-            // Function mmap() returns -1 in case of error.  Casting to i16 or i64
-            // yield the same result.
-            if vaddr as i8 == -1 {
-                return Err(Error::last_os_error());
-            }
-
-            // Zero-out the memory we got from Xen.  This will give us a clean slate and add
-            // the pages in the EL1 and EL2 page tables.  Otherwise the MMU
-            // throws and exception and Xen will abort the transfer.
-            vaddr.write_bytes(0, bounce_buffer_size);
-
-            Ok(BounceBuffer {
-                vaddr: vaddr as *mut c_void,
-                size: bounce_buffer_size,
-            })
+            )
+        } as *mut u8;
+
+        // Function mmap() returns -1 in case of error.  Casting to i16 or i64
+        // yield the same result.
+        if vaddr as i8 == -1 {
+            return Err(Error::last_os_error());
         }
+
+        // Zero-out the memory we got from Xen.  This will give us a clean slate and add
+        // the pages in the EL1 and EL2 page tables.  Otherwise the MMU
+        // throws an exception and Xen will abort the transfer.
+        // SAFETY: vaddr was mmapped and we checked mmap return value for errors.
+        unsafe { vaddr.write_bytes(0, bounce_buffer_size) };
+
+        Ok(BounceBuffer {
+            vaddr: vaddr as *mut c_void,
+            size: bounce_buffer_size,
+        })
     }
 
-    pub(crate) fn to_vaddr(&self) -> *mut c_void {
+    pub(crate) fn vaddr(&self) -> *mut c_void {
         self.vaddr
     }
+
+    pub(crate) unsafe fn into_vec<T: Clone>(self, len: usize) -> Vec<T> {
+        assert!(len * std::mem::size_of::<T>() < self.size);
+        core::slice::from_raw_parts::<T>(self.vaddr.cast(), len).to_vec()
+    }
 }
 
 impl Drop for BounceBuffer {
     fn drop(&mut self) {
-        unsafe {
-            if munmap(self.vaddr, self.size) < 0 {
-                println!(
-                    "Error {} unmapping vaddr: {:?}",
-                    Error::last_os_error(),
-                    self.vaddr
-                );
-            }
+        // SAFETY: we allocated self.vaddr in Self::new
+        if unsafe { munmap(self.vaddr, self.size) } < 0 {
+            println!(
+                "Error {} unmapping vaddr: {:?}",
+                Error::last_os_error(),
+                self.vaddr
+            );
         }
     }
 }

xen-ioctls/src/sysctl/sysctl.rs

@@ -8,10 +8,6 @@
  * except according to those terms.
  */
 
-use std::slice;
-
-use libc::c_void;
-
 #[cfg(target_arch = "aarch64")]
 use crate::aarch64::types::*;
 #[cfg(target_arch = "x86_64")]
@@ -20,27 +16,30 @@ use crate::{domctl::types::*, private::*, sysctl::types::*};
 
 fn do_sysctl(xen_sysctl: &mut XenSysctl) -> Result<(), std::io::Error> {
     let bouncebuffer = BounceBuffer::new(std::mem::size_of::<XenSysctl>())?;
-    let vaddr = bouncebuffer.to_vaddr() as *mut XenSysctl;
-    let mut privcmd_hypercall = PrivCmdHypercall {
+    let vaddr = bouncebuffer.vaddr() as *mut XenSysctl;
+    let mut privcmd = PrivCmdHypercall {
         op: __HYPERVISOR_SYSCTL,
         arg: [vaddr as u64, 0, 0, 0, 0],
     };
-    /*
-     * The expression "&mut privcmd_hypercall as *mut _" creates a reference
-     * to privcmd_hypercall before casting it to a *mut c_void
-     */
-    let privcmd_ptr: *mut c_void = &mut privcmd_hypercall as *mut _ as *mut c_void;
 
-    unsafe {
-        // Write content of XenSysctl to the bounce buffer so that Xen knows what
-        // we are asking for.
-        vaddr.write(*xen_sysctl);
+    // Write content of XenSysctl to the bounce buffer so that Xen knows what
+    // we are asking for.
+    // SAFETY: vaddr points to valid memory allocated when we created the bounce
+    // buffer
+    unsafe { vaddr.write(*xen_sysctl) };
 
-        do_ioctl(IOCTL_PRIVCMD_HYPERCALL(), privcmd_ptr).map(|_| {
-            // Read back content from bounce buffer if no errors.
-            *xen_sysctl = vaddr.read();
-        })
-    }
+    // SAFETY: we pass a PrivCmdHypercall value to an IOCTL_PRIVCMD_HYPERCALL ioctl
+    unsafe {
+        do_ioctl(
+            IOCTL_PRIVCMD_HYPERCALL(),
+            std::ptr::addr_of_mut!(privcmd).cast(),
+        )?
+    };
+    // Read back content from bounce buffer if no errors.
+    // SAFETY: the ioctl succeeded and vaddr was previously successfully allocated
+    // by us
+    *xen_sysctl = unsafe { vaddr.read() };
+    Ok(())
 }
 
 pub fn xc_physinfo() -> Result<XenSysctlPhysinfo, std::io::Error> {
@@ -52,7 +51,11 @@ pub fn xc_physinfo() -> Result<XenSysctlPhysinfo, std::io::Error> {
         },
     };
 
-    do_sysctl(&mut sysctl).map(|_| unsafe { sysctl.u.physinfo })
+    do_sysctl(&mut sysctl)?;
+    Ok(
+        // SAFETY: sysctl was successful, and we initialized the union ourselves.
+        unsafe { sysctl.u.physinfo },
+    )
 }
 
 pub fn xc_domain_getinfolist(
@@ -61,7 +64,6 @@ pub fn xc_domain_getinfolist(
 ) -> Result<Vec<XenDomctlGetDomainInfo>, std::io::Error> {
     let bouncebuffer =
         BounceBuffer::new(std::mem::size_of::<XenDomctlGetDomainInfo>() * max_domain as usize)?;
-    let vaddr = bouncebuffer.to_vaddr() as *mut XenDomctlGetDomainInfo;
 
     let mut sysctl = XenSysctl {
         cmd: XEN_SYSCTL_getdomaininfolist,
@@ -70,13 +72,18 @@ pub fn xc_domain_getinfolist(
             domaininfolist: XenSysctlGetdomaininfolist {
                 first_domain,
                 max_domain,
-                buffer: U64Aligned { v: vaddr as u64 },
+                buffer: U64Aligned {
+                    v: bouncebuffer.vaddr() as u64,
+                },
                 num_domains: 0,
             },
         },
     };
 
-    do_sysctl(&mut sysctl).map(|_| unsafe {
-        slice::from_raw_parts(vaddr, sysctl.u.domaininfolist.num_domains as usize).to_vec()
-    })
+    do_sysctl(&mut sysctl)?;
+    Ok(
+        // SAFETY: sysctl was successful, so bounce buffer must be populated with num_domains
+        // elements
+        unsafe { bouncebuffer.into_vec(sysctl.u.domaininfolist.num_domains as usize) },
+    )
 }