From 1afeb95c4e089bb1b166c8029999c8a72f3c70d8 Mon Sep 17 00:00:00 2001 From: Brennan Tracy Date: Tue, 10 Nov 2026 18:16:27 +0000 Subject: [PATCH 1/2] guest: handle small certificate buffers Check the firmware error status after each report request. Allocate a larger buffer and retry when the firmware needs more space. Confirm that a certificate buffer exists before reading its entries. Parse the certificate table from the allocated buffer. Some kernels do not fail the ioctl when the certificate buffer is too small. On those kernels, the code read the certificate table from an invalid placeholder address. Signed-off-by: Brennan Tracy --- src/firmware/guest/mod.rs | 75 ++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 41 deletions(-) diff --git a/src/firmware/guest/mod.rs b/src/firmware/guest/mod.rs index efddffe7..fd01c0e0 100644 --- a/src/firmware/guest/mod.rs +++ b/src/firmware/guest/mod.rs @@ -15,10 +15,7 @@ use crate::error::*; #[cfg(target_os = "linux")] use crate::firmware::{ host::CertTableEntry, - linux::{ - guest::{ioctl::*, types::*}, - host as HostFFI, - }, + linux::guest::{ioctl::*, types::*}, }; #[cfg(target_os = "linux")] @@ -126,7 +123,7 @@ impl Firmware { let mut report_response = ReportRsp::default(); // Define a buffer to store the certificates in. - let mut certificate_bytes: Vec; + let mut certificate_bytes: Vec = vec![]; // Due to the complex buffer allocation, we will take the ReportReq // provided by the caller, and create an extended report request object @@ -143,36 +140,32 @@ impl Firmware { &mut report_response, ); - // KEEP for Kernels before 47894e0f (5.19), as userspace broke at that hash. - if SNP_GET_EXT_REPORT - .ioctl(&mut self.0, &mut guest_request) - .is_err() - { - match guest_request.fw_err.into() { - // The kernel patch by pgonda@google.com in kernel hash 47894e0f - // changed the ioctl return to succeed instead of returning an - // error when encountering an invalid certificate length. This was - // done to keep the cryptography safe, so we will now just check - // the guest_request.fw_err for a new value. - // - // Check to see if the buffer needs to be resized. If it does, the - // we need to resize the buffer to the correct size, and - // re-request for the certificates. - VmmError::InvalidCertificatePageLength => { - certificate_bytes = vec![0u8; ext_report_request.certs_len as usize]; - ext_report_request.certs_address = certificate_bytes.as_mut_ptr() as u64; - let mut guest_request_retry: GuestRequest = - GuestRequest::new( - message_version, - &mut ext_report_request, - &mut report_response, - ); - SNP_GET_EXT_REPORT - .ioctl(&mut self.0, &mut guest_request_retry) - .map_err(|_| map_fw_err(guest_request_retry.fw_err.into()))?; + let result = SNP_GET_EXT_REPORT.ioctl(&mut self.0, &mut guest_request); + + // Depending on the kernel version, a certificate buffer that is too + // small either fails the ioctl or returns success. Both cases set + // fw_err, and this code checks it in either case. + match guest_request.fw_err.into() { + // Resize the buffer to the length the kernel asked for and + // request the certificates again. + VmmError::InvalidCertificatePageLength => { + certificate_bytes = vec![0u8; ext_report_request.certs_len as usize]; + ext_report_request.certs_address = certificate_bytes.as_mut_ptr() as u64; + let mut guest_request_retry: GuestRequest = + GuestRequest::new( + message_version, + &mut ext_report_request, + &mut report_response, + ); + let retry_result = SNP_GET_EXT_REPORT.ioctl(&mut self.0, &mut guest_request_retry); + if retry_result.is_err() || guest_request_retry.fw_err != 0 { + Err(map_fw_err(guest_request_retry.fw_err.into()))? } - _ => Err(map_fw_err(guest_request.fw_err.into()))?, } + _ if result.is_err() || guest_request.fw_err != 0 => { + Err(map_fw_err(guest_request.fw_err.into()))? + } + _ => {} } // Make sure response status is successful @@ -184,16 +177,16 @@ impl Firmware { return Ok((report_response.report.to_vec(), None)); } - let mut certificates: Vec; - - unsafe { - let entries = (ext_report_request.certs_address as *mut HostFFI::types::CertTableEntry) - .as_mut() - .ok_or(CertError::EmptyCertBuffer)?; - certificates = HostFFI::types::CertTableEntry::parse_table(entries)?; - certificates.sort(); + // The kernel writes certificates only into a buffer that this + // function allocated. Without one, certs_address still holds the + // u64::MAX placeholder from ExtReportReq::new. + if certificate_bytes.is_empty() { + Err(CertError::EmptyCertBuffer)? } + let mut certificates = CertTableEntry::vec_bytes_to_cert_table(&mut certificate_bytes)?; + certificates.sort(); + // Return both the Attestation Report, as well as the Cert Table. Ok((report_response.report.to_vec(), Some(certificates))) } From 5876667395b93858ad76af6e741b71c2958bfb54 Mon Sep 17 00:00:00 2001 From: Brennan Tracy Date: Tue, 10 Nov 2026 17:59:50 +0000 Subject: [PATCH 2/2] snp: validate certificate table bounds Parse certificate tables from byte buffers instead of raw pointers. Check each entry offset and length against the buffer size. Return an error when an entry exceeds the buffer or when the table lacks a terminating entry. The certificate table parser trusted offsets and lengths from the input table. Malformed input caused out-of-bounds reads and panics in a safe public function. Signed-off-by: Brennan Tracy --- Cargo.lock | 2 +- src/firmware/host/types/snp.rs | 5 +- src/firmware/linux/host/types/snp.rs | 111 +++++++++++++++------------ 3 files changed, 65 insertions(+), 53 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9545c638..d5ae268a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1040,7 +1040,7 @@ dependencies = [ "num-integer", "num-iter", "num-traits", - "rand 0.8.5", + "rand 0.8.8", "smallvec", "zeroize", ] diff --git a/src/firmware/host/types/snp.rs b/src/firmware/host/types/snp.rs index ea827f30..4aba318d 100644 --- a/src/firmware/host/types/snp.rs +++ b/src/firmware/host/types/snp.rs @@ -253,10 +253,7 @@ impl CertTableEntry { /// Takes in bytes in kernel CertTable format and returns in user API CertTable format. #[cfg(target_os = "linux")] pub fn vec_bytes_to_cert_table(bytes: &mut [u8]) -> Result, CertError> { - let cert_bytes_ptr: *mut FFI::types::CertTableEntry = - bytes.as_mut_ptr() as *mut FFI::types::CertTableEntry; - - Ok(unsafe { FFI::types::CertTableEntry::parse_table(cert_bytes_ptr).unwrap() }) + FFI::types::CertTableEntry::parse_table(bytes) } } diff --git a/src/firmware/linux/host/types/snp.rs b/src/firmware/linux/host/types/snp.rs index 16657328..93a70d10 100644 --- a/src/firmware/linux/host/types/snp.rs +++ b/src/firmware/linux/host/types/snp.rs @@ -166,52 +166,42 @@ impl CertTableEntry { /// }; /// ``` /// + /// The table ends at the first entry whose GUID is all zeroes. Each entry's + /// `offset` is relative to the start of `bytes`, and `offset + length` must + /// not exceed `bytes.len()`. + /// + /// # Errors + /// + /// Returns [`CertError::BufferOverflow`] if the table has no terminating + /// entry inside `bytes`, or if an entry points outside `bytes`. #[cfg(target_os = "linux")] - pub unsafe fn parse_table( - mut data: *mut CertTableEntry, - ) -> Result, uuid::Error> { - // Helpful Constance for parsing the data - const ZERO_GUID: Uuid = Uuid::from_bytes([0x0; 16]); + pub fn parse_table(bytes: &[u8]) -> Result, CertError> { + const ENTRY_LEN: usize = std::mem::size_of::(); - // Pre-defined re-usable variables. - let table_ptr: *mut u8 = data as *mut u8; - - // Create a location to store the final data. let mut retval: Vec = vec![]; + let mut remaining = bytes; - // Start parsing the PSP data from the pointers. - let mut entry: CertTableEntry; - - loop { - // Dereference the pointer to parse the table data. - entry = *data; - let guid: Uuid = Uuid::from_slice(entry.guid.as_slice())?; - - // Once we find a zeroed GUID, we are done. - if guid == ZERO_GUID { - break; - } - - // Calculate the beginning and ending pointers of the raw certificate data. - let mut cert_bytes: Vec = vec![]; - let mut cert_addr: *mut u8 = table_ptr.offset(entry.offset as isize); - let cert_end: *mut u8 = cert_addr.add(entry.length as usize); + while let Some((entry, rest)) = remaining.split_first_chunk::() { + remaining = rest; - // Gather the certificate bytes. - while cert_addr != cert_end { - cert_bytes.push(*cert_addr); - cert_addr = cert_addr.add(1usize); + let [guid @ .., o0, o1, o2, o3, l0, l1, l2, l3] = *entry; + let guid = Uuid::from_bytes(guid); + if guid.is_nil() { + return Ok(retval); } - // Build the Rust-friendly structure and append vector to be returned when - // we are finished. - retval.push(UAPI::CertTableEntry::from_guid(&guid, cert_bytes.clone())?); + let start = u32::from_ne_bytes([o0, o1, o2, o3]) as usize; + let length = u32::from_ne_bytes([l0, l1, l2, l3]) as usize; + let end = start.checked_add(length).ok_or(CertError::BufferOverflow)?; + let data = bytes.get(start..end).ok_or(CertError::BufferOverflow)?; - // Move the pointer ahead to the next value. - data = data.offset(1isize); + retval.push( + UAPI::CertTableEntry::from_guid(&guid, data.to_vec()) + .map_err(|_| CertError::InvalidGUID)?, + ); } - Ok(retval) + Err(CertError::BufferOverflow) } } @@ -527,6 +517,7 @@ mod test { #[cfg(target_os = "linux")] mod cert_table_entry { + use crate::error::CertError; use crate::firmware::host as UAPI; use crate::firmware::linux::host::types::CertTableEntry; use uuid::Uuid; @@ -565,7 +556,7 @@ mod test { #[test] fn test_parse_table_regular() { - let mut cert_bytes: Vec = vec![ + let cert_bytes: Vec = vec![ 192, 180, 6, 164, 168, 3, 73, 82, 151, 67, 63, 182, 1, 76, 208, 174, 120, 0, 0, 0, 25, 0, 0, 0, 74, 183, 179, 121, 187, 172, 79, 228, 160, 47, 5, 174, 243, 39, 199, 130, 145, 0, 0, 0, 25, 0, 0, 0, 99, 218, 117, 141, 230, 100, 69, 100, 173, 197, @@ -577,11 +568,8 @@ mod test { 5, 5, 5, 7, 7, 7, 7, 7, 7, ]; - let cert_bytes_ptr: *mut CertTableEntry = - cert_bytes.as_mut_ptr() as *mut CertTableEntry; - let actual: Vec = - unsafe { CertTableEntry::parse_table(cert_bytes_ptr).unwrap() }; + CertTableEntry::parse_table(&cert_bytes).unwrap(); let expected: Vec = build_vec_uapi_cert_table(); @@ -589,9 +577,8 @@ mod test { } #[test] - #[should_panic] fn test_parse_table_offset_short() { - let mut cert_bytes: Vec = vec![ + let cert_bytes: Vec = vec![ 192, 180, 6, 164, 168, 3, 73, 82, 151, 67, 63, 182, 1, 76, 208, 174, 120, 0, 0, 0, 1, 0, 0, 0, 74, 183, 179, 121, 187, 172, 79, 228, 160, 47, 5, 174, 243, 39, 199, 130, 145, 0, 0, 0, 25, 0, 0, 0, 99, 218, 117, 141, 230, 100, 69, 100, 173, 197, @@ -603,18 +590,46 @@ mod test { 5, 5, 5, 7, 7, 7, 7, 7, 7, ]; - let cert_bytes_ptr: *mut CertTableEntry = - cert_bytes.as_mut_ptr() as *mut CertTableEntry; - let actual: Vec = - unsafe { CertTableEntry::parse_table(cert_bytes_ptr).unwrap() }; + CertTableEntry::parse_table(&cert_bytes).unwrap(); let expected: Vec = build_vec_uapi_cert_table(); - assert_eq!( + assert_ne!( expected, actual, "Invalid certificate offset encountered..." ); } + + #[test] + fn test_parse_table_entry_out_of_bounds() { + let mut cert_bytes: Vec = + CertTableEntry::uapi_to_vec_bytes(&build_vec_uapi_cert_table()).unwrap(); + + // Make the last certificate's length run one byte past the buffer. + cert_bytes[92..96].copy_from_slice(&7u32.to_ne_bytes()); + + assert_eq!( + CertTableEntry::parse_table(&cert_bytes), + Err(CertError::BufferOverflow) + ); + } + + #[test] + fn test_parse_table_empty() { + let cert_bytes: Vec = CertTableEntry::uapi_to_vec_bytes(&[]).unwrap(); + + assert_eq!(CertTableEntry::parse_table(&cert_bytes), Ok(vec![])); + } + + #[test] + fn test_parse_table_missing_terminator() { + let cert_bytes: Vec = CertTableEntry::uapi_to_vec_bytes(&[]).unwrap(); + + assert_eq!( + CertTableEntry::parse_table(&cert_bytes[..23]), + Err(CertError::BufferOverflow) + ); + } } }