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 <[email protected]>

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "vmm-sys-util"
-version = "0.11.2"
+version = "0.12.0"
 authors = ["Intel Virtualization Team <[email protected]>"]
 description = "A system utility set"
 repository = "https://github.com/rust-vmm/vmm-sys-util"
@@ -26,3 +26,4 @@ bitflags = "1.0"
 
 [dev-dependencies]
 serde_json = "1.0.9"
+bincode = "1.3.3"

src/fam.rs

@@ -99,7 +99,7 @@ impl fmt::Display for Error {
 ///         self.len as usize
 ///     }
 ///
-///     fn set_len(&mut self, len: usize) {
+///     unsafe fn set_len(&mut self, len: usize) {
 ///         self.len = len as u32
 ///     }
 ///
@@ -135,7 +135,12 @@ pub unsafe trait FamStruct {
     ///
     /// 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
     ///
@@ -220,7 +225,11 @@ impl<T: Default + FamStruct> FamStructWrapper<T> {
             // 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 })
     }
@@ -276,8 +285,8 @@ impl<T: Default + FamStruct> FamStructWrapper<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]
     }
 
@@ -395,7 +404,11 @@ impl<T: Default + FamStruct> FamStructWrapper<T> {
             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 {
@@ -540,13 +553,23 @@ where
             {
                 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;
@@ -570,7 +593,7 @@ macro_rules! generate_fam_struct_impl {
                 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;
             }
 
@@ -603,7 +626,7 @@ mod tests {
     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]
@@ -1078,4 +1101,29 @@ mod tests {
         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)")
+        );
+    }
 }