From 489ca5ceb10e1a8a2ea9ea4c399d14005e2b8053 Mon Sep 17 00:00:00 2001 From: Stefano Garzarella Date: Mon, 16 Dec 2024 12:35:10 +0100 Subject: [PATCH 1/5] kernel/mm: return an error in layout_from_size() `Layout::from_size_align(size, align)` can return an error if following conditions are not met: - align must not be zero, - align must be a power of two, - size, when rounded up to the nearest multiple of align, must not overflow isize (i.e., the rounded value must be less than or equal to isize::MAX). Conditions on `align` are always met, but the size is specified by the caller, so it can fail. Let's propagate the error instead of panic. Signed-off-by: Stefano Garzarella --- kernel/src/mm/alloc.rs | 6 ++++-- kernel/src/vtpm/tcgtpm/wrapper.rs | 12 +++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/kernel/src/mm/alloc.rs b/kernel/src/mm/alloc.rs index b4a9fd43b..a7fad1914 100644 --- a/kernel/src/mm/alloc.rs +++ b/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 for SvsmError { @@ -1720,7 +1722,7 @@ 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 { let align: usize = { if (size % PAGE_SIZE) == 0 { PAGE_SIZE @@ -1728,7 +1730,7 @@ pub fn layout_from_size(size: usize) -> Layout { 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 { diff --git a/kernel/src/vtpm/tcgtpm/wrapper.rs b/kernel/src/vtpm/tcgtpm/wrapper.rs index 25cc22e18..2e31d6564 100644 --- a/kernel/src/vtpm/tcgtpm/wrapper.rs +++ b/kernel/src/vtpm/tcgtpm/wrapper.rs @@ -16,7 +16,6 @@ use crate::{ }; use core::{ - alloc::Layout, ffi::{c_char, c_int, c_ulong, c_void}, ptr, slice::from_raw_parts, @@ -28,15 +27,18 @@ 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); - unsafe { alloc(layout).cast() } + if let Ok(layout) = layout_from_size(size as usize) { + return unsafe { alloc(layout).cast() }; + } + ptr::null_mut() } #[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() }; + if let Ok(layout) = layout_from_size(new_size as usize) { + return unsafe { alloc_zeroed(layout).cast() }; + } } ptr::null_mut() } From 37d618ad493b6523bafada14d4478fcb06355371 Mon Sep 17 00:00:00 2001 From: Stefano Garzarella Date: Mon, 16 Dec 2024 16:20:45 +0100 Subject: [PATCH 2/5] kernel/vtpm: fix realloc() wrapper The current implementation lacks some requirements defined by POSIX documentation of `void *realloc(void *ptr, size_t size);`: - If ptr is a null pointer, realloc() shall be equivalent to malloc() for the specified size. - If the size of the space requested is zero, the behavior shall be implementation-defined. In this case we are following Linux implementation, from malloc(3) man page: - If size is equal to zero, and ptr is not NULL, then the call is equivalent to free(ptr). [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/realloc.html Signed-off-by: Stefano Garzarella --- kernel/src/vtpm/tcgtpm/wrapper.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/kernel/src/vtpm/tcgtpm/wrapper.rs b/kernel/src/vtpm/tcgtpm/wrapper.rs index 2e31d6564..f368d0fb3 100644 --- a/kernel/src/vtpm/tcgtpm/wrapper.rs +++ b/kernel/src/vtpm/tcgtpm/wrapper.rs @@ -47,7 +47,20 @@ pub extern "C" fn calloc(items: c_ulong, size: c_ulong) -> *mut c_void { 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 p.is_null() { + return malloc(size); + } + if let Some(layout) = layout_from_ptr(ptr) { + 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(); + } + return unsafe { _realloc(ptr, layout, new_size).cast() }; } ptr::null_mut() From 5d78d6b43a5d0af296940b774b326583753dbbf1 Mon Sep 17 00:00:00 2001 From: Stefano Garzarella Date: Mon, 16 Dec 2024 16:30:27 +0100 Subject: [PATCH 3/5] kernel/vtpm/wrapper: add more safety comments and checks Add SAFETY comments around calls to GlobalAlloc and checks as required by the documentation[1], especially on non-zero size and pointer validity. [1] https://doc.rust-lang.org/alloc/alloc/trait.GlobalAlloc.html Signed-off-by: Stefano Garzarella --- kernel/src/vtpm/tcgtpm/wrapper.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/kernel/src/vtpm/tcgtpm/wrapper.rs b/kernel/src/vtpm/tcgtpm/wrapper.rs index f368d0fb3..d544559c1 100644 --- a/kernel/src/vtpm/tcgtpm/wrapper.rs +++ b/kernel/src/vtpm/tcgtpm/wrapper.rs @@ -16,6 +16,7 @@ use crate::{ }; use core::{ + alloc::Layout, ffi::{c_char, c_int, c_ulong, c_void}, ptr, slice::from_raw_parts, @@ -27,7 +28,13 @@ use alloc::alloc::{alloc, alloc_zeroed, dealloc, realloc as _realloc}; #[no_mangle] pub extern "C" fn malloc(size: c_ulong) -> *mut c_void { + if size == 0 { + return ptr::null_mut(); + } + if let Ok(layout) = layout_from_size(size as usize) { + // SAFETY: layout is guaranteed to be non-zero size. Memory may not be + // initiatlized, but that's what the caller expects. return unsafe { alloc(layout).cast() }; } ptr::null_mut() @@ -36,7 +43,12 @@ pub extern "C" fn malloc(size: c_ulong) -> *mut c_void { #[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) { + if new_size == 0 { + return ptr::null_mut(); + } + if let Ok(layout) = layout_from_size(new_size as usize) { + // SAFETY: layout is guaranteed to be non-zero size. return unsafe { alloc_zeroed(layout).cast() }; } } @@ -61,6 +73,15 @@ pub unsafe extern "C" fn realloc(p: *mut c_void, size: c_ulong) -> *mut c_void { 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. return unsafe { _realloc(ptr, layout, new_size).cast() }; } ptr::null_mut() @@ -73,12 +94,17 @@ pub unsafe extern "C" fn free(p: *mut c_void) { } let ptr = p as *mut u8; if let Some(layout) = layout_from_ptr(ptr.cast()) { + // 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)); From 834ae811320d7d14905172f9a12fdc2a817fed91 Mon Sep 17 00:00:00 2001 From: Stefano Garzarella Date: Mon, 16 Dec 2024 16:35:27 +0100 Subject: [PATCH 4/5] kernel/vtpm: add missing safety comments Add SAFETY comments around FFI calls and some checks for return values. Signed-off-by: Stefano Garzarella --- kernel/src/vtpm/tcgtpm/mod.rs | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/kernel/src/vtpm/tcgtpm/mod.rs b/kernel/src/vtpm/tcgtpm/mod.rs index 1d7833eb4..3fbdb053f 100644 --- a/kernel/src/vtpm/tcgtpm/mod.rs +++ b/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 { + // 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::(), 0) }; + // SAFETY: FFI call. Parameters and return values are checked. + let mut rc = unsafe { _plat__NVEnable(VirtAddr::null().as_mut_ptr::(), 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()); } From bd82a9952daf1862bb35419bfc900fcdfc6c9fff Mon Sep 17 00:00:00 2001 From: Stefano Garzarella Date: Tue, 7 Jan 2025 17:57:55 +0100 Subject: [PATCH 5/5] kernel/vtpm/wrapper: reduce indentation in some wrappers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In several places we use `if let Ok(foo) = func() {` creating large indented blocks, let's replace with `let Ok(foo) = func() else {` by immediately returning a null pointer and reducing the indentation to make the code more readable. Suggested-by: Carlos López Signed-off-by: Stefano Garzarella --- kernel/src/vtpm/tcgtpm/wrapper.rs | 90 ++++++++++++++++--------------- 1 file changed, 48 insertions(+), 42 deletions(-) diff --git a/kernel/src/vtpm/tcgtpm/wrapper.rs b/kernel/src/vtpm/tcgtpm/wrapper.rs index d544559c1..022311432 100644 --- a/kernel/src/vtpm/tcgtpm/wrapper.rs +++ b/kernel/src/vtpm/tcgtpm/wrapper.rs @@ -32,27 +32,31 @@ pub extern "C" fn malloc(size: c_ulong) -> *mut c_void { return ptr::null_mut(); } - if let Ok(layout) = layout_from_size(size as usize) { - // SAFETY: layout is guaranteed to be non-zero size. Memory may not be - // initiatlized, but that's what the caller expects. - return unsafe { alloc(layout).cast() }; - } - 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) { - if new_size == 0 { - return ptr::null_mut(); - } - - if let Ok(layout) = layout_from_size(new_size as usize) { - // SAFETY: layout is guaranteed to be non-zero size. - 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] @@ -64,27 +68,28 @@ pub unsafe extern "C" fn realloc(p: *mut c_void, size: c_ulong) -> *mut c_void { return malloc(size); } - if let Some(layout) = layout_from_ptr(ptr) { - 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. - return unsafe { _realloc(ptr, layout, new_size).cast() }; + 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(); } - 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] @@ -93,12 +98,13 @@ 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()) { - // 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) } - } + 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]