CVE-2023-50711: Out-of-bounds Write (CWE-787)
value: vmm-sys-util is a collection of modules that provides helpers and utilities used by multiple rust-vmm components. Starting in version 0.5.0 and prior to version 0.12.0, an issue in the `FamStructWrapper::deserialize` implementation provided by the crate for `vmm_sys_util::fam::FamStructWrapper` can lead to out of bounds memory accesses. The deserialization does not check that the length stored in the header matches the flexible array length. Mismatch in the lengths might allow out of bounds memory access through Rust-safe methods. The issue was corrected in version 0.12.0 by inserting a check that verifies the lengths of compared flexible arrays are equal for any deserialized header and aborting deserialization otherwise. Moreover, the API was changed so that header length can only be modified through Rust-unsafe code. This ensures that users cannot trigger out-of-bounds memory access from Rust-safe code.
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 9.8
Published: 2024-01-02T20:15Z
Repository: https://github.com/rust-vmm/vmm-sys-util
Language: Rust
Files changed: Cargo.toml, src/fam.rs
Commit date: 2024-01-02 11:02:43 +0000
Commit message: fix: deserialization issue of FamStructWrapper with serde

An issue was discovered in the Serde::deserialize implementation of
the FamStructWrapper that can lead to out-of-bounds memory access via
safe Rust code.

When dserializing a FamStructWrapper we reconstruct the header of the
type from the saved state and then reconstruct the flexible array part
in a separate step. The header includes information about the length of
the flexible array part. However, during deserialization, we do not
check that the length included in the header matches with the length of
the deserialized flexible array. The safety of FamStructWrapper methods
accessing the underlying memory of the flexible array depends on the
header length reflects the memory size of the flexible array.

If the saved state was malformed in way that this condition is not true,
and even worse, the header length implies a flexible array buffer bigger
than what we allocated memory for, a user can trigger out-bounds access
via Rust-safe code.

This commit introduces a check that the header length matches the length
of the flexible array deserialized from the saved state. If it doesn't,
deserialization returns an Error.

Moreover, we mark the method that can change the header length as unsafe
and make the as_mut_fam_struct method private, so that it is not
possible for a consumer of the library to break this invariant from safe
code.

Signed-off-by: Babis Chalios <bchalios@amazon.es>

Diff: diff --git a/Cargo.toml b/Cargo.toml
index fe4e7ea..997cf2a 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,8 +1,8 @@
 [package]
 name = "vmm-sys-util"
-version = "0.11.2"
+version = "0.12.0"
 authors = ["Intel Virtualization Team <vmm-maintainers@intel.com>"]
 description = "A system utility set"
 repository = "https://github.com/rust-vmm/vmm-sys-util"
 readme = "README.md"
 keywords = ["utils"]
@@ -24,5 +24,6 @@ serde_derive = { version = "1.0.27", optional = true }
 [target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies]
 bitflags = "1.0"
 
 [dev-dependencies]
 serde_json = "1.0.9"
+bincode = "1.3.3"
diff --git a/src/fam.rs b/src/fam.rs
index 3c0c03b..38a6ff3 100644
--- a/src/fam.rs
+++ b/src/fam.rs
@@ -97,11 +97,11 @@ impl fmt::Display for Error {
 ///
 ///     fn len(&self) -> usize {
 ///         self.len as usize
 ///     }
 ///
-///     fn set_len(&mut self, len: usize) {
+///     unsafe fn set_len(&mut self, len: usize) {
 ///         self.len = len as u32
 ///     }
 ///
 ///     fn max_len() -> usize {
 ///         100
@@ -133,11 +133,16 @@ pub unsafe trait FamStruct {
 
     /// Set the FAM length
     ///
     /// These type of structures contain a member that holds the FAM length.
     /// This method will set the value of that member.
-    fn set_len(&mut self, len: usize);
+    ///
+    /// # Safety
+    ///
+    /// The caller needs to ensure that `len` here reflects the correct number of entries of the
+    /// flexible array part of the struct.
+    unsafe fn set_len(&mut self, len: usize);
 
     /// Get max allowed FAM length
     ///
     /// This depends on each structure.
     /// For example a structure representing the cpuid can contain at most 80 entries.
@@ -218,11 +223,15 @@ impl<T: Default + FamStruct> FamStructWrapper<T> {
         mem_allocator.push(T::default());
         for _ in 1..required_mem_allocator_capacity {
             // SAFETY: Safe as long T follows the requirements of being POD.
             mem_allocator.push(unsafe { mem::zeroed() })
         }
-        mem_allocator[0].set_len(num_elements);
+        // SAFETY: The flexible array part of the struct has `num_elements` capacity. We just
+        // initialized this in `mem_allocator`.
+        unsafe {
+            mem_allocator[0].set_len(num_elements);
+        }
 
         Ok(FamStructWrapper { mem_allocator })
     }
 
     /// Create a new FamStructWrapper from a slice of elements.
@@ -274,12 +283,12 @@ impl<T: Default + FamStruct> FamStructWrapper<T> {
     /// Get a reference to the actual [`FamStruct`](trait.FamStruct.html) instance.
     pub fn as_fam_struct_ref(&self) -> &T {
         &self.mem_allocator[0]
     }
 
-    /// Get a mut reference to the actual [`FamStruct`](trait.FamStruct.html) instance.
-    pub fn as_mut_fam_struct(&mut self) -> &mut T {
+    // Get a mut reference to the actual [`FamStruct`](trait.FamStruct.html) instance.
+    fn as_mut_fam_struct(&mut self) -> &mut T {
         &mut self.mem_allocator[0]
     }
 
     /// Get a pointer to the [`FamStruct`](trait.FamStruct.html) instance.
     ///
@@ -393,11 +402,15 @@ impl<T: Default + FamStruct> FamStructWrapper<T> {
             // SAFETY: Safe as long as the trait is only implemented for POD. This is a requirement
             // for the trait implementation.
             self.mem_allocator[i] = unsafe { mem::zeroed() }
         }
         // Update the len of the underlying `FamStruct`.
-        self.as_mut_fam_struct().set_len(len);
+        // SAFETY: We just adjusted the memory for the underlying `mem_allocator` to hold `len`
+        // entries.
+        unsafe {
+            self.as_mut_fam_struct().set_len(len);
+        }
 
         // If the len needs to be decreased, deallocate unnecessary memory
         if additional_elements < 0 {
             self.mem_allocator.shrink_to_fit();
         }
@@ -538,17 +551,27 @@ where
             where
                 V: SeqAccess<'de>,
             {
                 use serde::de::Error;
 
-                let header = seq
+                let header: X = seq
                     .next_element()?
                     .ok_or_else(|| de::Error::invalid_length(0, &self))?;
                 let entries: Vec<X::Entry> = seq
                     .next_element()?
                     .ok_or_else(|| de::Error::invalid_length(1, &self))?;
 
+                if header.len() != entries.len() {
+                    let msg = format!(
+                        "Mismatch between length of FAM specified in FamStruct header ({}) \
+                         and actual size of FAM ({})",
+                        header.len(),
+                        entries.len()
+                    );
+                    return Err(V::Error::custom(msg));
+                }
+
                 let mut result: Self::Value = FamStructWrapper::from_entries(entries.as_slice())
                     .map_err(|e| V::Error::custom(format!("{:?}", e)))?;
                 result.mem_allocator[0] = header;
                 Ok(result)
             }
@@ -568,11 +591,11 @@ macro_rules! generate_fam_struct_impl {
 
             fn len(&self) -> usize {
                 self.$field_name as usize
             }
 
-            fn set_len(&mut self, len: usize) {
+            unsafe fn set_len(&mut self, len: usize) {
                 self.$field_name = len as $field_type;
             }
 
             fn max_len() -> usize {
                 $max
@@ -601,11 +624,11 @@ mod tests {
     use super::*;
 
     const MAX_LEN: usize = 100;
 
     #[repr(C)]
-    #[derive(Default, PartialEq, Eq)]
+    #[derive(Default, Debug, PartialEq, Eq)]
     pub struct __IncompleteArrayField<T>(::std::marker::PhantomData<T>, [T; 0]);
     impl<T> __IncompleteArrayField<T> {
         #[inline]
         pub fn new() -> Self {
             __IncompleteArrayField(::std::marker::PhantomData, [])
@@ -1076,6 +1099,31 @@ mod tests {
         assert_eq!(wrapper2.as_mut_fam_struct().index, 3);
         assert_eq!(wrapper2.as_mut_fam_struct().length, 8);
         assert_eq!(wrapper2.as_mut_fam_struct().flags, 2);
         assert_eq!(wrapper2.as_slice(), [0, 0, 0, 3, 14, 0, 0, 1]);
     }
+
+    #[cfg(feature = "with-serde")]
+    #[test]
+    fn test_bad_deserialize() {
+        #[repr(C)]
+        #[derive(Default, Debug, PartialEq, Serialize, Deserialize)]
+        struct Foo {
+            pub len: u32,
+            pub padding: u32,
+            pub entries: __IncompleteArrayField<u32>,
+        }
+
+        generate_fam_struct_impl!(Foo, u32, entries, u32, len, 100);
+
+        let state = FamStructWrapper::<Foo>::new(0).unwrap();
+        let mut bytes = bincode::serialize(&state).unwrap();
+
+        // The `len` field of the header is the first to be serialized.
+        // Writing at position 0 of the serialized data should change its value.
+        bytes[0] = 255;
+
+        assert!(
+            matches!(bincode::deserialize::<FamStructWrapper<Foo>>(&bytes).map_err(|boxed| *boxed), Err(bincode::ErrorKind::Custom(s)) if s == *"Mismatch between length of FAM specified in FamStruct header (255) and actual size of FAM (0)")
+        );
+    }
 }

Vulnerable code: version = "0.11.2"
///     fn set_len(&mut self, len: usize) {
    fn set_len(&mut self, len: usize);
        mem_allocator[0].set_len(num_elements);
    /// Get a mut reference to the actual [`FamStruct`](trait.FamStruct.html) instance.
    pub fn as_mut_fam_struct(&mut self) -> &mut T {
        self.as_mut_fam_struct().set_len(len);
                let header = seq
            fn set_len(&mut self, len: usize) {
    #[derive(Default, PartialEq, Eq)]
Fixed code: version = "0.12.0"
bincode = "1.3.3"
///     unsafe fn set_len(&mut self, len: usize) {
    ///
    /// # Safety
    ///
    /// The caller needs to ensure that `len` here reflects the correct number of entries of the
    /// flexible array part of the struct.
    unsafe fn set_len(&mut self, len: usize);
        // SAFETY: The flexible array part of the struct has `num_elements` capacity. We just
        // initialized this in `mem_allocator`.
        unsafe {
            mem_allocator[0].set_len(num_elements);
        }
    // Get a mut reference to the actual [`FamStruct`](trait.FamStruct.html) instance.
    fn as_mut_fam_struct(&mut self) -> &mut T {
        // SAFETY: We just adjusted the memory for the underlying `mem_allocator` to hold `len`
        // entries.
        unsafe {
            self.as_mut_fam_struct().set_len(len);
        }
                let header: X = seq
                if header.len() != entries.len() {
                    let msg = format!(
                        "Mismatch between length of FAM specified in FamStruct header ({}) \
                         and actual size of FAM ({})",
                        header.len(),
                        entries.len()
                    );
                    return Err(V::Error::custom(msg));
                }

            unsafe fn set_len(&mut self, len: usize) {
    #[derive(Default, Debug, PartialEq, Eq)]

    #[cfg(feature = "with-serde")]
    #[test]
    fn test_bad_deserialize() {
        #[repr(C)]
        #[derive(Default, Debug, PartialEq, Serialize, Deserialize)]
        struct Foo {
            pub len: u32,
            pub padding: u32,
            pub entries: __IncompleteArrayField<u32>,
        }

        generate_fam_struct_impl!(Foo, u32, entries, u32, len, 100);

        let state = FamStructWrapper::<Foo>::new(0).unwrap();
        let mut bytes = bincode::serialize(&state).unwrap();

        // The `len` field of the header is the first to be serialized.
        // Writing at position 0 of the serialized data should change its value.
        bytes[0] = 255;

        assert!(
            matches!(bincode::deserialize::<FamStructWrapper<Foo>>(&bytes).map_err(|boxed| *boxed), Err(bincode::ErrorKind::Custom(s)) if s == *"Mismatch between length of FAM specified in FamStruct header (255) and actual size of FAM (0)")
        );
    }