Various improvements and fixes

integration-test/bins/multiboot2_chainloader/src/loader.rs

@@ -20,11 +20,8 @@ pub fn load_module(mut modules: multiboot::information::ModuleIter) -> ! {
 
     // Check if a header is present.
     {
-        let hdr = multiboot2_header::Multiboot2Header::find_header(elf_bytes)
-            .unwrap()
+        let (hdr, _) = multiboot2_header::Multiboot2Header::find_header(elf_bytes)
             .expect("Should have Multiboot2 header");
-        let hdr =
-            unsafe { multiboot2_header::Multiboot2Header::load(hdr.0.as_ptr().cast()) }.unwrap();
         log::info!("Multiboot2 header:\n{hdr:#?}");
     }
 
@@ -91,9 +88,6 @@ fn map_memory(ph: ProgramHeaderEntry) {
     let dest_ptr = unsafe { dest_ptr.add(ph.filesz() as usize) };
 
     // Zero .bss memory
-    for _ in 0..(ph.memsz() - ph.filesz()) {
-        unsafe {
-            core::ptr::write(dest_ptr, 0);
-        }
-    }
+    assert!(ph.memsz() >= ph.filesz());
+    unsafe { core::ptr::write_bytes(dest_ptr, 0, (ph.memsz() - ph.filesz()) as usize) };
 }

multiboot2-common/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+- Added validation for complete padded tag sequences.
+- Added size details to memory validation errors.
+- Fixed validation for dynamically sized structures whose reported total size
+  exceeds the available buffer.
 - Small code improvements
 
 

multiboot2-common/src/boxed.rs

@@ -3,7 +3,6 @@
 use crate::{ALIGNMENT, Header, MaybeDynSized, increase_to_alignment};
 use alloc::boxed::Box;
 use core::alloc::Layout;
-use core::mem;
 use core::ops::Deref;
 use core::ptr;
 
@@ -31,7 +30,7 @@ pub fn new_boxed<T: MaybeDynSized<Metadata = usize> + ?Sized>(
         .map(|b| b.len())
         .sum::<usize>();
 
-    let tag_size = mem::size_of::<T::Header>() + additional_size;
+    let tag_size = size_of::<T::Header>() + additional_size;
     header.set_size(tag_size);
 
     // Allocation size is multiple of alignment.
@@ -43,7 +42,7 @@ pub fn new_boxed<T: MaybeDynSized<Metadata = usize> + ?Sized>(
 
     // write header
     {
-        let len = mem::size_of::<T::Header>();
+        let len = size_of::<T::Header>();
         let ptr = core::ptr::addr_of!(header);
         unsafe {
             ptr::copy_nonoverlapping(ptr.cast::<u8>(), heap_ptr, len);
@@ -52,7 +51,7 @@ pub fn new_boxed<T: MaybeDynSized<Metadata = usize> + ?Sized>(
 
     // write body
     {
-        let mut write_offset = mem::size_of::<T::Header>();
+        let mut write_offset = size_of::<T::Header>();
         for &bytes in additional_bytes_slices {
             let len = bytes.len();
             let src = bytes.as_ptr();
@@ -71,7 +70,7 @@ pub fn new_boxed<T: MaybeDynSized<Metadata = usize> + ?Sized>(
     // If this panic triggers, there is a fundamental flaw in my logic. This is
     // not the fault of an API user.
     assert_eq!(
-        mem::size_of_val(reference.deref()),
+        size_of_val(reference.deref()),
         alloc_size,
         "Allocation should match Rusts expectation"
     );

multiboot2-common/src/bytes_ref.rs

@@ -2,7 +2,6 @@
 
 use crate::{ALIGNMENT, Header, MemoryError};
 use core::marker::PhantomData;
-use core::mem;
 use core::ops::Deref;
 
 /// Wraps a byte slice representing a Multiboot2 structure including an optional
@@ -25,7 +24,7 @@ impl<'a, H: Header> TryFrom<&'a [u8]> for BytesRef<'a, H> {
     type Error = MemoryError;
 
     fn try_from(bytes: &'a [u8]) -> Result<Self, Self::Error> {
-        if bytes.len() < mem::size_of::<H>() {
+        if bytes.len() < size_of::<H>() {
             return Err(MemoryError::ShorterThanHeader);
         }
         // Doesn't work as expected: if align_of_val(&value[0]) < ALIGNMENT {

multiboot2-common/src/iter.rs

@@ -4,7 +4,6 @@
 
 use crate::{ALIGNMENT, DynSizedStructure, Header, increase_to_alignment};
 use core::marker::PhantomData;
-use core::mem;
 
 /// Iterates over the tags (modelled by [`DynSizedStructure`]) of the underlying
 /// byte slice. Each tag is expected to have the same common [`Header`] with
@@ -64,7 +63,7 @@ impl<'a, H: Header + 'a> Iterator for TagIter<'a, H> {
         // See <https://doc.rust-lang.org/reference/type-layout.html>.
         let slice = {
             let from = self.next_tag_offset;
-            let len = mem::size_of::<H>() + tag_hdr.payload_len();
+            let len = size_of::<H>() + tag_hdr.payload_len();
             let to = from + len;
 
             // The size of (the allocation for) a value is always a multiple of

multiboot2-common/src/lib.rs

@@ -256,7 +256,6 @@ pub use iter::TagIter;
 pub use tag::{MaybeDynSized, Tag};
 
 use core::fmt::Debug;
-use core::mem;
 use core::ptr;
 use core::ptr::NonNull;
 use core::slice;
@@ -284,7 +283,7 @@ pub trait Header: Clone + Sized + PartialEq + Eq + Debug {
     /// plus [`Header::payload_len`].
     #[must_use]
     fn total_size(&self) -> usize {
-        mem::size_of::<Self>() + self.payload_len()
+        size_of::<Self>() + self.payload_len()
     }
 
     /// Updates the header with the given `total_size`.
@@ -334,15 +333,20 @@ impl<H: Header> DynSizedStructure<H> {
         let ptr = bytes.as_ptr().cast::<H>();
         let hdr = unsafe { &*ptr };
 
-        if hdr.payload_len() > bytes.len() {
-            return Err(MemoryError::InvalidReportedTotalSize);
+        let payload_len = hdr.payload_len();
+        let total_size = size_of::<H>() + payload_len;
+        if total_size > bytes.len() {
+            return Err(MemoryError::InvalidReportedTotalSize(
+                total_size,
+                bytes.len(),
+            ));
         }
 
         // At this point we know that the memory slice fulfills the base
         // assumptions and requirements. Now, we safety can create the fat
         // pointer.
 
-        let dst_size = hdr.payload_len();
+        let dst_size = payload_len;
         // Create fat pointer for the DST.
         let ptr = ptr_meta::from_raw_parts(ptr.cast(), dst_size);
         let reference = unsafe { &*ptr };
@@ -401,28 +405,86 @@ impl<H: Header> DynSizedStructure<H> {
     /// # Panics
     /// This panics if there is a size mismatch. However, this should never be
     /// the case if all types follow their documented requirements.
-    ///
-    /// [`size_of_val`]: mem::size_of_val
     pub fn cast<T: MaybeDynSized<Header = H> + ?Sized>(&self) -> &T {
         // Thin or fat pointer, depending on type.
         // However, only thin ptr is needed.
         let base_ptr = ptr::addr_of!(*self);
 
         // This should be a compile-time assertion. However, this is the best
         // location to place it for now.
-        assert!(T::BASE_SIZE >= mem::size_of::<H>());
+        assert!(T::BASE_SIZE >= size_of::<H>());
 
         let t_dst_size = T::dst_len(self.header());
         // Creates thin or fat pointer, depending on type.
         let t_ptr = ptr_meta::from_raw_parts(base_ptr.cast(), t_dst_size);
         let t_ref = unsafe { &*t_ptr };
 
-        assert_eq!(mem::size_of_val(self), mem::size_of_val(t_ref));
+        assert_eq!(size_of_val(self), size_of_val(t_ref));
 
         t_ref
     }
 }
 
+/// Validates a sequence of padded Multiboot2 (header) tags.
+///
+/// Both Multiboot2 information tags and Multiboot2 header tags use an 8-byte
+/// tag header with the reported tag size stored in bytes 4..8. The reported
+/// size excludes alignment padding, but each following tag starts at the next
+/// 8-byte boundary.
+///
+/// Returns `Ok(true)` when a valid end tag is present exactly at the end of the
+/// provided byte range, and `Ok(false)` when the byte range ends without an end
+/// tag.
+pub fn validate_tag_sequence(
+    bytes: &[u8],
+    mut is_end_tag: impl FnMut(&[u8]) -> bool,
+) -> Result<bool, MemoryError> {
+    // Common header property for Multiboot2 and Multiboot2 header tags:
+    // The `size` property is always at offset 4..8 (the second u32).
+    const TAG_HEADER_SIZE: usize = size_of::<u32>() * 2;
+
+    if bytes.as_ptr().align_offset(ALIGNMENT) != 0 {
+        return Err(MemoryError::WrongAlignment);
+    }
+
+    let mut offset = 0;
+    while offset < bytes.len() {
+        let remaining = bytes.len() - offset;
+        if remaining < TAG_HEADER_SIZE {
+            return Err(MemoryError::ShorterThanHeader);
+        }
+
+        let tag = &bytes[offset..];
+        let total_size =
+            u32::from_ne_bytes(tag[4..8].try_into().expect("slice has exactly 4 bytes")) as usize;
+
+        if total_size < TAG_HEADER_SIZE {
+            return Err(MemoryError::SizeInsufficient(total_size, TAG_HEADER_SIZE));
+        }
+
+        let padded_size = total_size
+            .checked_add(ALIGNMENT - 1)
+            .map(|size| size & !(ALIGNMENT - 1))
+            .ok_or(MemoryError::InvalidReportedTotalSize(total_size, remaining))?;
+        if padded_size > remaining {
+            return Err(MemoryError::InvalidReportedTotalSize(
+                padded_size,
+                remaining,
+            ));
+        }
+
+        offset += padded_size;
+        if is_end_tag(&tag[..total_size]) {
+            if offset == bytes.len() {
+                return Ok(true);
+            }
+            return Err(MemoryError::InvalidReportedTotalSize(offset, bytes.len()));
+        }
+    }
+
+    Ok(false)
+}
+
 /// Errors that may occur when working with memory.
 #[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq, Hash, Error)]
 pub enum MemoryError {
@@ -436,15 +498,20 @@ pub enum MemoryError {
     /// type.
     #[error("memory range is shorter than the size of the header structure")]
     ShorterThanHeader,
+    /// The size is insufficient to contain at least a valid minimal structure.
+    #[error("memory range is shorter than the size of the header structure")]
+    SizeInsufficient(usize /* actual */, usize /* expected */),
     /// The buffer misses the terminating padding to the next alignment
     /// boundary. The padding is relevant to satisfy Rustc/Miri, but also the
     /// spec mandates that the padding is added.
     #[error("memory is missing required padding")]
     MissingPadding,
     /// The size-property has an illegal value that can't be fulfilled with the
     /// given bytes.
-    #[error("the header reports an invalid total size")]
-    InvalidReportedTotalSize,
+    #[error(
+        "header reports an invalid total size of 0x{0:x} while only 0x{1:x} bytes are available"
+    )]
+    InvalidReportedTotalSize(usize /* actual */, usize /* expected */),
 }
 
 /// Increases the given size to the next alignment boundary, if it is not a
@@ -487,7 +554,7 @@ mod tests {
         impl MaybeDynSized for CustomSizedTag {
             type Header = DummyTestHeader;
 
-            const BASE_SIZE: usize = mem::size_of::<Self>();
+            const BASE_SIZE: usize = size_of::<Self>();
 
             fn dst_len(_header: &DummyTestHeader) -> Self::Metadata {}
         }
@@ -502,7 +569,7 @@ mod tests {
         let tag = DynSizedStructure::ref_from_slice(bytes.borrow()).unwrap();
         let custom_tag = tag.cast::<CustomSizedTag>();
 
-        assert_eq!(mem::size_of_val(custom_tag), 16);
+        assert_eq!(size_of_val(custom_tag), 16);
         assert_eq!(custom_tag.a, 0xdead_beef);
         assert_eq!(custom_tag.b, 0x1337_1337);
     }
@@ -530,4 +597,24 @@ mod tests {
         assert_eq!(tag.header().typ(), 0x1337);
         assert_eq!(tag.header().size(), 18);
     }
+
+    #[test]
+    fn test_ref_from_slice_rejects_oversized_header() {
+        #[rustfmt::skip]
+        let bytes = AlignedBytes::new(
+            [
+                0x37, 0x13, 0, 0,
+                /* Tag size */
+                24, 0, 0, 0,
+                /* Only 8 bytes payload plus padding are available. */
+                0, 1, 2, 3,
+                4, 5, 6, 7,
+            ],
+        );
+
+        assert_eq!(
+            DynSizedStructure::<DummyTestHeader>::ref_from_slice(bytes.borrow()),
+            Err(MemoryError::InvalidReportedTotalSize(24, 16))
+        );
+    }
 }

multiboot2-common/src/tag.rs

@@ -1,7 +1,6 @@
 //! Module for the traits [`MaybeDynSized`] and [`Tag`].
 
 use crate::{BytesRef, DynSizedStructure, Header};
-use core::mem;
 use core::slice;
 use ptr_meta::Pointee;
 
@@ -55,7 +54,7 @@ pub trait MaybeDynSized: Pointee {
     /// Returns the payload, i.e., all memory that is not occupied by the
     /// [`Header`] of the type.
     fn payload(&self) -> &[u8] {
-        let from = mem::size_of::<Self::Header>();
+        let from = size_of::<Self::Header>();
         &self.as_bytes()[from..]
     }
 
@@ -65,7 +64,7 @@ pub trait MaybeDynSized: Pointee {
     fn as_bytes(&self) -> BytesRef<'_, Self::Header> {
         let ptr = core::ptr::addr_of!(*self);
         // Actual tag size with optional terminating padding.
-        let size = mem::size_of_val(self);
+        let size = size_of_val(self);
         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.
@@ -95,7 +94,7 @@ pub trait Tag: MaybeDynSized {
 impl<H: Header> MaybeDynSized for DynSizedStructure<H> {
     type Header = H;
 
-    const BASE_SIZE: usize = mem::size_of::<H>();
+    const BASE_SIZE: usize = size_of::<H>();
 
     fn dst_len(header: &Self::Header) -> Self::Metadata {
         header.payload_len()

multiboot2-common/src/test_utils.rs

@@ -4,7 +4,6 @@
 
 use crate::{Header, MaybeDynSized, Tag};
 use core::borrow::Borrow;
-use core::mem;
 use core::ops::Deref;
 
 /// Helper to 8-byte align the underlying bytes, as mandated in the Multiboot2
@@ -71,7 +70,7 @@ impl DummyTestHeader {
 
 impl Header for DummyTestHeader {
     fn payload_len(&self) -> usize {
-        self.size as usize - mem::size_of::<Self>()
+        self.size as usize - size_of::<Self>()
     }
 
     fn set_size(&mut self, total_size: usize) {
@@ -87,7 +86,7 @@ pub struct DummyDstTag {
 }
 
 impl DummyDstTag {
-    const BASE_SIZE: usize = mem::size_of::<DummyTestHeader>();
+    const BASE_SIZE: usize = size_of::<DummyTestHeader>();
 
     #[must_use]
     pub const fn header(&self) -> &DummyTestHeader {
@@ -103,7 +102,7 @@ impl DummyDstTag {
 impl MaybeDynSized for DummyDstTag {
     type Header = DummyTestHeader;
 
-    const BASE_SIZE: usize = mem::size_of::<DummyTestHeader>();
+    const BASE_SIZE: usize = size_of::<DummyTestHeader>();
 
     fn dst_len(header: &Self::Header) -> Self::Metadata {
         header.size as usize - Self::BASE_SIZE
@@ -117,7 +116,6 @@ impl Tag for DummyDstTag {
 
 #[cfg(test)]
 mod tests {
-    use core::mem;
     use core::ptr::addr_of;
 
     use crate::ALIGNMENT;
@@ -126,7 +124,7 @@ mod tests {
 
     #[test]
     fn abi() {
-        assert_eq!(mem::align_of::<AlignedBytes<0>>(), ALIGNMENT);
+        assert_eq!(align_of::<AlignedBytes<0>>(), ALIGNMENT);
 
         let bytes = AlignedBytes([0]);
         assert_eq!(bytes.as_ptr().align_offset(8), 0);