Merge pull request #574 from stefano-garzarella/unsafe-comments-vtpm

vtpm: add safety comments and fix small issues

kernel/src/mm/alloc.rs

@@ -34,6 +34,8 @@ pub enum AllocError {
     InvalidFilePage(VirtAddr),
     /// The page frame number (PFN) is invalid.
     InvalidPfn(usize),
+    /// The specified size causes an error when creating the layout.
+    InvalidLayout,
 }
 
 impl From<AllocError> for SvsmError {
@@ -1720,15 +1722,15 @@ static TEST_ROOT_MEM_LOCK: SpinLock<()> = SpinLock::new(());
 
 pub const MIN_ALIGN: usize = 32;
 
-pub fn layout_from_size(size: usize) -> Layout {
+pub fn layout_from_size(size: usize) -> Result<Layout, AllocError> {
     let align: usize = {
         if (size % PAGE_SIZE) == 0 {
             PAGE_SIZE
         } else {
             MIN_ALIGN
         }
     };
-    Layout::from_size_align(size, align).unwrap()
+    Layout::from_size_align(size, align).map_err(|_| AllocError::InvalidLayout)
 }
 
 pub fn layout_from_ptr(ptr: *mut u8) -> Option<Layout> {

kernel/src/vtpm/tcgtpm/mod.rs

@@ -40,6 +40,7 @@ impl TcgTpm {
     }
 
     fn teardown(&self) -> Result<(), SvsmReqError> {
+        // SAFETY: FFI call. Return value is checked.
         let result = unsafe { TPM_TearDown() };
         match result {
             0 => Ok(()),
@@ -51,6 +52,7 @@ impl TcgTpm {
     }
 
     fn manufacture(&self, first_time: i32) -> Result<i32, SvsmReqError> {
+        // SAFETY: FFI call. Parameter and return values are checked.
         let result = unsafe { TPM_Manufacture(first_time) };
         match result {
             // TPM manufactured successfully
@@ -96,6 +98,9 @@ impl TcgTpmSimulatorInterface for TcgTpm {
         let mut response_ffi_p = response_ffi.as_mut_ptr();
         let mut response_ffi_size = TPM_BUFFER_MAX_SIZE as u32;
 
+        // SAFETY: FFI calls. Parameters are checked. Both calls are void,
+        // _plat__RunCommand() returns `response_ffi_size` value by reference
+        // and it is validated.
         unsafe {
             _plat__LocalitySet(locality);
             _plat__RunCommand(
@@ -128,10 +133,20 @@ impl TcgTpmSimulatorInterface for TcgTpm {
             return Err(SvsmReqError::invalid_request());
         }
         if !only_reset {
-            unsafe { _plat__Signal_PowerOn() };
+            // SAFETY: FFI call. No parameter, return value is checked.
+            let result = unsafe { _plat__Signal_PowerOn() };
+            if result != 0 {
+                log::error!("_plat__Signal_PowerOn failed rc={}", result);
+                return Err(SvsmReqError::incomplete());
+            }
         }
         // It calls TPM_init() within to indicate that a TPM2_Startup is required.
-        unsafe { _plat__Signal_Reset() };
+        // SAFETY: FFI call. No parameter, return value is checked.
+        let result = unsafe { _plat__Signal_Reset() };
+        if result != 0 {
+            log::error!("_plat__Signal_Reset failed rc={}", result);
+            return Err(SvsmReqError::incomplete());
+        }
         self.is_powered_on = true;
 
         Ok(())
@@ -141,6 +156,7 @@ impl TcgTpmSimulatorInterface for TcgTpm {
         if !self.is_powered_on {
             return Err(SvsmReqError::invalid_request());
         }
+        // SAFETY: FFI call. No Parameters or return values.
         unsafe { _plat__SetNvAvail() };
 
         Ok(())
@@ -162,10 +178,16 @@ impl VtpmInterface for TcgTpm {
         // 5. Power it on indicating it requires startup. By default, OVMF will start
         //    and selftest it.
 
-        unsafe { _plat__NVEnable(VirtAddr::null().as_mut_ptr::<c_void>(), 0) };
+        // SAFETY: FFI call. Parameters and return values are checked.
+        let mut rc = unsafe { _plat__NVEnable(VirtAddr::null().as_mut_ptr::<c_void>(), 0) };
+        if rc != 0 {
+            log::error!("_plat__NVEnable failed rc={}", rc);
+            return Err(SvsmReqError::incomplete());
+        }
 
-        let mut rc = self.manufacture(1)?;
+        rc = self.manufacture(1)?;
         if rc != 0 {
+            // SAFETY: FFI call. Parameter checked, no return value.
             unsafe { _plat__NVDisable(1 as *mut c_void, 0) };
             return Err(SvsmReqError::incomplete());
         }

kernel/src/vtpm/tcgtpm/wrapper.rs

@@ -28,27 +28,68 @@ use alloc::alloc::{alloc, alloc_zeroed, dealloc, realloc as _realloc};
 
 #[no_mangle]
 pub extern "C" fn malloc(size: c_ulong) -> *mut c_void {
-    let layout: Layout = layout_from_size(size as usize);
+    if size == 0 {
+        return ptr::null_mut();
+    }
+
+    let Ok(layout) = layout_from_size(size as usize) else {
+        return ptr::null_mut();
+    };
+
+    // SAFETY: layout is guaranteed to be non-zero size. Memory may not be
+    // initiatlized, but that's what the caller expects.
     unsafe { alloc(layout).cast() }
 }
 
 #[no_mangle]
 pub extern "C" fn calloc(items: c_ulong, size: c_ulong) -> *mut c_void {
-    if let Some(new_size) = items.checked_mul(size) {
-        let layout = layout_from_size(new_size as usize);
-        return unsafe { alloc_zeroed(layout).cast() };
+    let Some(new_size) = items.checked_mul(size) else {
+        return ptr::null_mut();
+    };
+
+    if new_size == 0 {
+        return ptr::null_mut();
     }
-    ptr::null_mut()
+
+    let Ok(layout) = layout_from_size(new_size as usize) else {
+        return ptr::null_mut();
+    };
+
+    // SAFETY: layout is guaranteed to be non-zero size.
+    unsafe { alloc_zeroed(layout).cast() }
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn realloc(p: *mut c_void, size: c_ulong) -> *mut c_void {
     let ptr = p as *mut u8;
     let new_size = size as usize;
-    if let Some(layout) = layout_from_ptr(ptr) {
-        return unsafe { _realloc(ptr, layout, new_size).cast() };
+
+    if p.is_null() {
+        return malloc(size);
     }
-    ptr::null_mut()
+
+    let Some(layout) = layout_from_ptr(ptr) else {
+        return ptr::null_mut();
+    };
+
+    if new_size == 0 {
+        // SAFETY: layout_from_ptr() call ensures that `ptr` was allocated
+        // with this allocator and we are using the same `layout` used to
+        // allocate `ptr`.
+        unsafe { dealloc(ptr, layout) };
+        return ptr::null_mut();
+    }
+
+    // This will fail if `new_size` rounded value exceeds `isize::MAX`
+    if Layout::from_size_align(new_size, layout.align()).is_err() {
+        return ptr::null_mut();
+    }
+
+    // SAFETY: layout_from_ptr() call ensures that `ptr` was allocated with
+    // this allocator and we are using the same `layout` used to allocate
+    // `ptr`. We also checked that `new_size` aligned does not overflow and
+    // it is not 0.
+    unsafe { _realloc(ptr, layout, new_size).cast() }
 }
 
 #[no_mangle]
@@ -57,13 +98,19 @@ pub unsafe extern "C" fn free(p: *mut c_void) {
         return;
     }
     let ptr = p as *mut u8;
-    if let Some(layout) = layout_from_ptr(ptr.cast()) {
-        unsafe { dealloc(ptr, layout) }
-    }
+    let Some(layout) = layout_from_ptr(ptr.cast()) else {
+        return;
+    };
+    // SAFETY: layout_from_ptr() call ensures that `ptr` was allocated
+    // with this allocator and we are using the same `layout` used to
+    // allocate `ptr`.
+    unsafe { dealloc(ptr, layout) }
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn serial_out(s: *const c_char, size: c_int) {
+    // SAFETY: caller must provide safety requirements for
+    // [`core::slice::from_raw_parts`]
     let str_slice: &[u8] = unsafe { from_raw_parts(s as *const u8, size as usize) };
     if let Ok(rust_str) = from_utf8(str_slice) {
         _print(format_args!("[SVSM] {}", rust_str));