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/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))) } 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) + ); + } } }