`serde` deserialization for `FamStructWrapper` lacks bound checks that could potentially lead to out-of-bounds memory access

@@ -1,6 +1,6 @@
 [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"
@@ -26,3 +26,4 @@ bitflags = "1.0"
 
 [dev-dependencies]
 serde_json = "1.0.9"
+bincode = "1.3.3"

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