Various UB Fixes

multiboot2-common/CHANGELOG.md

@@ -2,6 +2,14 @@
 
 ## Unreleased
 
+- **Breaking:** Fixed undefined behavior when serializing stack-constructed
+  structures with implicit trailing padding: `MaybeDynSized::as_bytes` now
+  returns a plain `&[u8]` that covers exactly the structure size reported in
+  the header (clamped to the allocation) instead of a `BytesRef` over the
+  whole allocation, whose trailing padding is uninitialized memory for
+  stack-constructed values. `payload()` follows suit. As a side effect,
+  `clone_dyn` now preserves the reported size exactly instead of growing it
+  to the padded allocation size.
 - Added the `raw_type!` macro that generates an ABI-safe `#[repr(transparent)]`
   newtype plus a corresponding high-level open-set enum, including all
   conversions between them and the underlying integer.

multiboot2-common/src/boxed.rs

@@ -109,6 +109,7 @@ mod tests {
     use super::*;
     use crate::Tag;
     use crate::test_utils::{DummyDstTag, DummyTestHeader};
+    use core::slice;
 
     #[test]
     fn test_new_boxed() {
@@ -126,23 +127,35 @@ mod tests {
 
     #[test]
     fn test_new_boxed_zeroes_padding() {
-        // A payload of 1 byte yields a 9-byte tag, padded to 16 bytes. The
-        // 7 trailing padding bytes must be initialized (to zero); otherwise
-        // reading them via `as_bytes()` is UB (caught by Miri).
+        // A payload of 1 byte yields a 9-byte tag in a 16-byte allocation.
+        // `as_bytes()` must exclude the 7 trailing padding bytes, while the
+        // allocation itself must be zeroed there (guaranteed by
+        // `alloc_zeroed`), as the Multiboot2 spec mandates zeroed padding
+        // between tags.
         let header = DummyTestHeader::new(DummyDstTag::ID, 0);
         let tag = new_boxed::<DummyDstTag>(header, &[&[0xff]]);
-        let bytes = tag.as_bytes();
-        assert_eq!(bytes.len(), 16);
-        assert_eq!(&bytes[9..16], &[0, 0, 0, 0, 0, 0, 0]);
+        assert_eq!(tag.as_bytes().len(), 9);
+        let ptr = (&raw const *tag).cast::<u8>();
+        // SAFETY: The allocation spans `size_of_val` bytes and is fully
+        // initialized by `new_boxed` (zeroed allocation).
+        let all_bytes = unsafe { slice::from_raw_parts(ptr, size_of_val(&*tag)) };
+        assert_eq!(all_bytes.len(), 16);
+        assert_eq!(&all_bytes[9..16], &[0, 0, 0, 0, 0, 0, 0]);
     }
 
     #[test]
     fn test_clone_tag() {
+        // A 5-byte payload, so that the reported tag size (13) is no
+        // multiple of the alignment.
         let header = DummyTestHeader::new(DummyDstTag::ID, 0);
-        let tag = new_boxed::<DummyDstTag>(header, &[&[0, 1, 2, 3]]);
+        let tag = new_boxed::<DummyDstTag>(header, &[&[0, 1, 2, 3, 4]]);
         assert_eq!(tag.header().typ(), 42);
-        assert_eq!(tag.payload(), &[0, 1, 2, 3]);
+        assert_eq!(tag.payload(), &[0, 1, 2, 3, 4]);
 
-        let _cloned = clone_dyn(tag.as_ref());
+        let cloned = clone_dyn(tag.as_ref());
+        // The clone must round-trip exactly; especially, the reported size
+        // must not grow to the padded allocation size.
+        assert_eq!(cloned.header(), tag.header());
+        assert_eq!(cloned.payload(), tag.payload());
     }
 }

multiboot2-common/src/tag.rs

@@ -1,6 +1,6 @@
 //! Module for the traits [`MaybeDynSized`] and [`Tag`].
 
-use crate::{BytesRef, DynSizedStructure, Header};
+use crate::{DynSizedStructure, Header};
 use core::slice;
 use ptr_meta::Pointee;
 
@@ -69,25 +69,31 @@ pub unsafe trait MaybeDynSized: Pointee {
     }
 
     /// Returns the payload, i.e., all memory that is not occupied by the
-    /// [`Header`] of the type.
+    /// [`Header`] of the type. Implicit trailing padding beyond the
+    /// structure size reported in the header is not part of the payload.
+    ///
+    /// # Panics
+    /// Panics if the size reported in the header is smaller than the size of
+    /// the [`Header`] itself, which can only happen for oddly formed values.
     fn payload(&self) -> &[u8] {
         let from = size_of::<Self::Header>();
         &self.as_bytes()[from..]
     }
 
-    /// Returns the whole allocated bytes for this structure encapsulated in
-    /// [`BytesRef`]. This includes padding bytes. To only get the "true" tag
-    /// data, read the tag size from [`Self::header`] and create a sub slice.
-    fn as_bytes(&self) -> BytesRef<'_, Self::Header> {
+    /// Returns the bytes of the structure, i.e., the header and the payload,
+    /// up to the structure size reported by [`Self::header`].
+    ///
+    /// Implicit trailing padding that the Rust memory layout might add beyond
+    /// that size is excluded, as it may be uninitialized for stack-constructed
+    /// values and must never be read.
+    fn as_bytes(&self) -> &[u8] {
         let ptr = &raw const *self;
-        // Actual tag size with optional terminating padding.
-        let size = size_of_val(self);
-        // SAFETY: `ptr` points to `self`'s allocation and `size_of_val(self)`
-        // covers the initialized object representation, including padding.
-        let slice = unsafe { slice::from_raw_parts(ptr.cast::<u8>(), size) };
-        // Unwrap is fine as this type can't exist without the underlying memory
-        // guarantees.
-        BytesRef::try_from(slice).unwrap()
+        // Clamp to the allocation: a corrupt header size must never cause an
+        // out-of-bounds slice.
+        let size = self.header().total_size().min(size_of_val(self));
+        // SAFETY: `ptr` points to `self`'s allocation, `size` is in bounds,
+        // and the first `total_size()` bytes of a value are initialized.
+        unsafe { slice::from_raw_parts(ptr.cast::<u8>(), size) }
     }
 
     /// Returns a pointer to this structure.

multiboot2-header/CHANGELOG.md

@@ -2,6 +2,14 @@
 
 ## Unreleased
 
+- Fixed undefined behavior when serializing stack-constructed sized tags with
+  trailing struct padding (`ConsoleHeaderTag`, `EntryAddressHeaderTag`,
+  `EntryEfi32HeaderTag`, `EntryEfi64HeaderTag`, `FramebufferHeaderTag`): the
+  padding is uninitialized memory that `as_bytes()` - and thus
+  `Builder::build()` - exposed. `as_bytes()` now covers exactly the reported
+  tag size, and `Builder::build()` writes explicit zeroed padding between
+  tags. The built header is byte-wise identical, except that its inter-tag
+  padding is now guaranteed to be zeroed.
 - **Breaking:** `InformationRequestHeaderTag::new()` now takes
   `&[MbiTagType]` and `requests()` returns an iterator over `MbiTagType`.
   The `MbiTagTypeId` re-export was renamed to `MbiTagTypeRaw`.

multiboot2-header/src/builder.rs

@@ -8,7 +8,7 @@ use crate::{
 };
 use alloc::boxed::Box;
 use alloc::vec::Vec;
-use multiboot2_common::{DynSizedStructure, MaybeDynSized, new_boxed};
+use multiboot2_common::{DynSizedStructure, MaybeDynSized, increase_to_alignment, new_boxed};
 
 /// Builder for a Multiboot2 header.
 #[derive(Debug)]
@@ -123,42 +123,62 @@ impl Builder {
     /// Multiboot2 header structure.
     #[must_use]
     pub fn build(self) -> Box<DynSizedStructure<Multiboot2BasicHeader>> {
+        fn fill_zeroes_to_alignment(vec: &mut Vec<u8>) {
+            let pad = increase_to_alignment(vec.len()) - vec.len();
+            for _ in 0..pad {
+                vec.push(0);
+            }
+        }
+
         let header = Multiboot2BasicHeader::new(self.arch, 0);
-        let mut byte_refs = Vec::new();
+        // Vec to be filled with the serialized tags, each zero-padded to the
+        // alignment.
+        let mut buffer_tags = Vec::<u8>::new();
         if let Some(tag) = self.information_request_tag.as_ref() {
-            byte_refs.push(tag.as_bytes().as_ref());
+            buffer_tags.extend_from_slice(tag.as_bytes());
+            fill_zeroes_to_alignment(&mut buffer_tags);
         }
         if let Some(tag) = self.address_tag.as_ref() {
-            byte_refs.push(tag.as_bytes().as_ref());
+            buffer_tags.extend_from_slice(tag.as_bytes());
+            fill_zeroes_to_alignment(&mut buffer_tags);
         }
         if let Some(tag) = self.entry_tag.as_ref() {
-            byte_refs.push(tag.as_bytes().as_ref());
+            buffer_tags.extend_from_slice(tag.as_bytes());
+            fill_zeroes_to_alignment(&mut buffer_tags);
         }
         if let Some(tag) = self.console_tag.as_ref() {
-            byte_refs.push(tag.as_bytes().as_ref());
+            buffer_tags.extend_from_slice(tag.as_bytes());
+            fill_zeroes_to_alignment(&mut buffer_tags);
         }
         if let Some(tag) = self.framebuffer_tag.as_ref() {
-            byte_refs.push(tag.as_bytes().as_ref());
+            buffer_tags.extend_from_slice(tag.as_bytes());
+            fill_zeroes_to_alignment(&mut buffer_tags);
         }
         if let Some(tag) = self.module_align_tag.as_ref() {
-            byte_refs.push(tag.as_bytes().as_ref());
+            buffer_tags.extend_from_slice(tag.as_bytes());
+            fill_zeroes_to_alignment(&mut buffer_tags);
         }
         if let Some(tag) = self.efi_bs_tag.as_ref() {
-            byte_refs.push(tag.as_bytes().as_ref());
+            buffer_tags.extend_from_slice(tag.as_bytes());
+            fill_zeroes_to_alignment(&mut buffer_tags);
         }
         if let Some(tag) = self.efi_32_tag.as_ref() {
-            byte_refs.push(tag.as_bytes().as_ref());
+            buffer_tags.extend_from_slice(tag.as_bytes());
+            fill_zeroes_to_alignment(&mut buffer_tags);
         }
         if let Some(tag) = self.efi_64_tag.as_ref() {
-            byte_refs.push(tag.as_bytes().as_ref());
+            buffer_tags.extend_from_slice(tag.as_bytes());
+            fill_zeroes_to_alignment(&mut buffer_tags);
         }
         if let Some(tag) = self.relocatable_tag.as_ref() {
-            byte_refs.push(tag.as_bytes().as_ref());
+            buffer_tags.extend_from_slice(tag.as_bytes());
+            fill_zeroes_to_alignment(&mut buffer_tags);
         }
         // TODO add support for custom tags once someone requests it.
         let end_tag = EndHeaderTag::new();
-        byte_refs.push(end_tag.as_bytes().as_ref());
-        new_boxed(header, byte_refs.as_slice())
+
+        buffer_tags.extend_from_slice(end_tag.as_bytes());
+        new_boxed(header, &[buffer_tags.as_slice()])
     }
 }
 
@@ -216,10 +236,17 @@ mod tests {
 
         let structure = builder.build();
 
+        #[cfg(miri)]
+        let _all_initialized = structure
+            .as_bytes()
+            .iter()
+            .map(|&byte| byte as u64)
+            .sum::<u64>();
+
         let header = {
             // SAFETY: The builder emits a fully formed, aligned header
             // buffer with a valid end tag.
-            unsafe { Header::load(structure.as_bytes().as_ref().as_ptr().cast()) }.unwrap()
+            unsafe { Header::load(structure.as_bytes().as_ptr().cast()) }.unwrap()
         };
 
         assert_eq!(header.verify_checksum(), Ok(()));

multiboot2/CHANGELOG.md

@@ -2,6 +2,14 @@
 
 ## Unreleased
 
+- Fixed undefined behavior when serializing stack-constructed sized tags with
+  trailing struct padding (`BootdevTag`, `ApmTag`, `RsdpV1Tag`, `RsdpV2Tag`,
+  `EFISdt32Tag`, `EFIImageHandle32Tag`, `ImageLoadPhysAddrTag`): the padding
+  is uninitialized memory that `as_bytes()` - and thus `Builder::build()` -
+  exposed. `as_bytes()` now covers exactly the reported tag size, and
+  `Builder::build()` writes explicit zeroed padding between tags. The built
+  boot information is byte-wise identical, except that its inter-tag padding
+  is now guaranteed to be zeroed.
 - **Breaking:** Renamed `TagTypeId` to `TagTypeRaw`; it is now generated by the
   `raw_type!` macro from `multiboot2-common` and gained more conversions.
   `TagHeader::new()` now takes `impl Into<TagType>`.

multiboot2/src/apm.rs

@@ -136,4 +136,20 @@ mod tests {
         let tag = ApmTag::new(1, 2, 3, 4, 5, 6, 7, 8, 9);
         assert_eq!(tag.header.size, 28);
     }
+
+    /// Representative test for all stack-constructed sized tags: the bytes
+    /// exposed for a tag built via its constructor must cover exactly the
+    /// reported tag size and exclude the implicit trailing padding of the
+    /// Rust type layout, which is uninitialized memory that must never be
+    /// read (verified by Miri).
+    #[test]
+    fn as_bytes_covers_exactly_the_reported_tag_size() {
+        let tag = ApmTag::new(1, 2, 3, 4, 5, 6, 7, 8, 9);
+        let bytes = tag.as_bytes();
+        assert_eq!(bytes.len(), 28);
+        assert!(bytes.len() < size_of::<ApmTag>());
+        // Reading every single byte must be defined behavior.
+        let byte_sum = bytes.iter().map(|&byte| byte as u64).sum::<u64>();
+        assert!(byte_sum > 0);
+    }
 }

multiboot2/src/boot_loader_name.rs

@@ -126,15 +126,13 @@ mod tests {
     #[cfg(feature = "builder")]
     fn test_build_str() {
         let tag = BootLoaderNameTag::new("hello");
-        let bytes = tag.as_bytes().as_ref();
-        let bytes = &bytes[..tag.header.size as usize];
+        let bytes = tag.as_bytes();
         assert_eq!(bytes, &get_bytes()[..tag.header.size as usize]);
         assert_eq!(tag.name(), Ok("hello"));
 
         // With terminating null.
         let tag = BootLoaderNameTag::new("hello\0");
-        let bytes = tag.as_bytes().as_ref();
-        let bytes = &bytes[..tag.header.size as usize];
+        let bytes = tag.as_bytes();
         assert_eq!(bytes, &get_bytes()[..tag.header.size as usize]);
         assert_eq!(tag.name(), Ok("hello"));