various safety and correctness improvements

multiboot2-common/CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- **Breaking:** `Header` now requires `total_size()` and derives
+  `payload_len()` from it.
 - Added validation for complete padded tag sequences.
 - Added size details to memory validation errors.
 - Fixed validation for dynamically sized structures whose reported total size

multiboot2-common/src/boxed.rs

@@ -37,13 +37,16 @@ pub fn new_boxed<T: MaybeDynSized<Metadata = usize> + ?Sized>(
     // See <https://doc.rust-lang.org/reference/type-layout.html>
     let alloc_size = increase_to_alignment(tag_size);
     let layout = Layout::from_size_align(alloc_size, ALIGNMENT).unwrap();
+    // SAFETY: `layout` matches the requested allocation size and alignment.
     let heap_ptr = unsafe { alloc::alloc::alloc(layout) };
     assert!(!heap_ptr.is_null());
 
     // write header
     {
         let len = size_of::<T::Header>();
         let ptr = core::ptr::addr_of!(header);
+        // SAFETY: `header` is a fully initialized stack value and `heap_ptr`
+        // points into the freshly allocated destination buffer.
         unsafe {
             ptr::copy_nonoverlapping(ptr.cast::<u8>(), heap_ptr, len);
         }
@@ -55,16 +58,21 @@ pub fn new_boxed<T: MaybeDynSized<Metadata = usize> + ?Sized>(
         for &bytes in additional_bytes_slices {
             let len = bytes.len();
             let src = bytes.as_ptr();
+            let dst = heap_ptr.wrapping_add(write_offset);
+            // SAFETY: `src` is a valid slice and `dst` stays inside the
+            // allocated object without overlapping `src`.
             unsafe {
-                let dst = heap_ptr.add(write_offset);
                 ptr::copy_nonoverlapping(src, dst, len);
-                write_offset += len;
             }
+            write_offset += len;
         }
     }
 
     // This is a fat pointer for DSTs and a thin pointer for sized `T`s.
+    // SAFETY: The allocation was sized for `T` and all bytes up to the
+    // reported dynamic length were initialized above.
     let ptr: *mut T = ptr_meta::from_raw_parts_mut(heap_ptr.cast(), T::dst_len(&header));
+    // SAFETY: `ptr` points to the initialized allocation described above.
     let reference = unsafe { Box::from_raw(ptr) };
 
     // If this panic triggers, there is a fundamental flaw in my logic. This is

multiboot2-common/src/iter.rs

@@ -31,10 +31,15 @@ pub struct TagIter<'a, H: Header> {
 
 impl<'a, H: Header> TagIter<'a, H> {
     /// Creates a new iterator.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that the whole chain of tags (with their reported
+    /// sizes) is valid and fits within the memory slice.
     #[must_use]
     // TODO we could take a BytesRef here, but the surrounding code should be
     //  bullet-proof enough.
-    pub fn new(mem: &'a [u8]) -> Self {
+    pub unsafe fn new(mem: &'a [u8]) -> Self {
         // Assert alignment.
         assert_eq!(mem.as_ptr().align_offset(ALIGNMENT), 0);
 
@@ -55,15 +60,22 @@ impl<'a, H: Header + 'a> Iterator for TagIter<'a, H> {
         }
         assert!(self.next_tag_offset < self.buffer.len());
 
-        let ptr = unsafe { self.buffer.as_ptr().add(self.next_tag_offset) }.cast::<H>();
+        let ptr = self
+            .buffer
+            .as_ptr()
+            .wrapping_add(self.next_tag_offset)
+            .cast::<H>();
+        // SAFETY: `new()` requires a validated, aligned tag chain and the
+        // current offset is checked to stay within the buffer before this
+        // dereference.
         let tag_hdr = unsafe { &*ptr };
 
         // Get relevant byte portion for the next tag. This includes padding
         // bytes to fulfill Rust memory guarantees. Otherwise, Miri complains.
         // See <https://doc.rust-lang.org/reference/type-layout.html>.
         let slice = {
             let from = self.next_tag_offset;
-            let len = size_of::<H>() + tag_hdr.payload_len();
+            let len = tag_hdr.total_size();
             let to = from + len;
 
             // The size of (the allocation for) a value is always a multiple of
@@ -78,6 +90,7 @@ impl<'a, H: Header + 'a> Iterator for TagIter<'a, H> {
         };
 
         // unwrap: We should not fail at this point.
+        // In any ::load() before, we already validated the whole chain of tags.
         let tag = DynSizedStructure::ref_from_slice(slice).unwrap();
         Some(tag)
     }
@@ -108,7 +121,8 @@ mod tests {
                 8, 0, 0, 0,
             ],
         );
-        let mut iter = TagIter::<DummyTestHeader>::new(bytes.borrow());
+        // SAFETY: Chain of tags is valid.
+        let mut iter = unsafe { TagIter::<DummyTestHeader>::new(bytes.borrow()) };
         let first = iter.next().unwrap();
         assert_eq!(first.header().typ(), 0xff);
         assert_eq!(first.header().size(), 8);

multiboot2-common/src/lib.rs

@@ -63,7 +63,7 @@
 //! indicates the total size of the structure. This is roughly translated to the
 //! following rusty base type:
 //!
-//! ```ignore
+//! ```rust,ignore
 //! #[repr(C, align(8))]
 //! struct DynStructure {
 //!     header: MyHeader,
@@ -91,7 +91,7 @@
 //!
 //! Note that we also have structures (tags) in Multiboot2 that looks like this:
 //!
-//! ```ignore
+//! ```rust,ignore
 //! #[repr(C, align(8))]
 //! struct DynStructure {
 //!     header: MyHeader,
@@ -102,7 +102,7 @@
 //!
 //! or
 //!
-//! ```ignore
+//! ```rust,ignore
 //! #[repr(C, align(8))]
 //! struct CommandLineTag {
 //!     header: TagHeader,
@@ -224,8 +224,9 @@
 #![deny(
     clippy::all,
     clippy::cargo,
-    clippy::must_use_candidate,
     clippy::nursery,
+    clippy::must_use_candidate,
+    clippy::undocumented_unsafe_blocks,
     missing_debug_implementations,
     missing_docs,
     rustdoc::all
@@ -274,16 +275,18 @@ pub const ALIGNMENT: usize = 8;
 /// The alignment of implementors **must** be the compatible with the demands
 /// for the corresponding structure, which typically is [`ALIGNMENT`].
 pub trait Header: Clone + Sized + PartialEq + Eq + Debug {
-    /// Returns the length of the payload, i.e., the bytes that are additional
-    /// to the header. The value is measured in bytes.
+    /// Returns the total size of the structure in bytes, including the fixed
+    /// header and any dynamic payload.
     #[must_use]
-    fn payload_len(&self) -> usize;
+    fn total_size(&self) -> usize;
 
-    /// Returns the total size of the struct, thus the size of the header itself
-    /// plus [`Header::payload_len`].
+    /// Returns the length of the payload, i.e., the bytes that are additional
+    /// to the header. The value is measured in bytes.
     #[must_use]
-    fn total_size(&self) -> usize {
-        size_of::<Self>() + self.payload_len()
+    fn payload_len(&self) -> usize {
+        let total_size = self.total_size();
+        assert!(total_size >= size_of::<Self>());
+        total_size - size_of::<Self>()
     }
 
     /// Updates the header with the given `total_size`.
@@ -294,7 +297,7 @@ pub trait Header: Clone + Sized + PartialEq + Eq + Debug {
 /// and a dynamic amount of bytes without hidden implicit padding.
 ///
 /// This structures combines a [`Header`] with the logically owned data by
-/// that header according to the reported [`Header::payload_len`]. Instances
+/// that header according to the reported [`Header::total_size`]. Instances
 /// guarantees that the memory requirements promised in the crates description
 /// are respected.
 ///
@@ -331,16 +334,22 @@ impl<H: Header> DynSizedStructure<H> {
     /// from the given [`BytesRef`].
     pub fn ref_from_bytes(bytes: BytesRef<'_, H>) -> Result<&Self, MemoryError> {
         let ptr = bytes.as_ptr().cast::<H>();
+        // SAFETY: `BytesRef` guarantees alignment and that the buffer covers
+        // at least the fixed header size.
         let hdr = unsafe { &*ptr };
 
-        let payload_len = hdr.payload_len();
-        let total_size = size_of::<H>() + payload_len;
+        let total_size = hdr.total_size();
+        let header_size = size_of::<H>();
+        if total_size < header_size {
+            return Err(MemoryError::SizeInsufficient(total_size, header_size));
+        }
         if total_size > bytes.len() {
             return Err(MemoryError::InvalidReportedTotalSize(
                 total_size,
                 bytes.len(),
             ));
         }
+        let payload_len = total_size - header_size;
 
         // At this point we know that the memory slice fulfills the base
         // assumptions and requirements. Now, we safety can create the fat
@@ -349,6 +358,8 @@ impl<H: Header> DynSizedStructure<H> {
         let dst_size = payload_len;
         // Create fat pointer for the DST.
         let ptr = ptr_meta::from_raw_parts(ptr.cast(), dst_size);
+        // SAFETY: The allocation was sized from the validated reported total
+        // size, so the fat pointer refers to initialized memory.
         let reference = unsafe { &*ptr };
         Ok(reference)
     }
@@ -368,9 +379,18 @@ impl<H: Header> DynSizedStructure<H> {
     /// The caller must ensure that the function operates on valid memory.
     pub unsafe fn ref_from_ptr<'a>(ptr: NonNull<H>) -> Result<&'a Self, MemoryError> {
         let ptr = ptr.as_ptr().cast_const();
+        // SAFETY: `ptr` came from a valid pointer to the header; we only read
+        // the reported total size and immediately re-slice that range.
         let hdr = unsafe { &*ptr };
+        let total_size = hdr.total_size();
+        let header_size = size_of::<H>();
+        if total_size < header_size {
+            return Err(MemoryError::SizeInsufficient(total_size, header_size));
+        }
 
-        let slice = unsafe { slice::from_raw_parts(ptr.cast::<u8>(), hdr.total_size()) };
+        // SAFETY: `total_size` came from the validated header and matches the
+        // readable byte range for the structure.
+        let slice = unsafe { slice::from_raw_parts(ptr.cast::<u8>(), total_size) };
         Self::ref_from_slice(slice)
     }
 
@@ -417,6 +437,8 @@ impl<H: Header> DynSizedStructure<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);
+        // SAFETY: `self` is a valid reference and the cast keeps the same
+        // allocation; `T::dst_len` determines the matching tail length.
         let t_ref = unsafe { &*t_ptr };
 
         assert_eq!(size_of_val(self), size_of_val(t_ref));
@@ -456,7 +478,7 @@ pub fn validate_tag_sequence(
 
         let tag = &bytes[offset..];
         let total_size =
-            u32::from_ne_bytes(tag[4..8].try_into().expect("slice has exactly 4 bytes")) as usize;
+            u32::from_le_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));
@@ -617,4 +639,24 @@ mod tests {
             Err(MemoryError::InvalidReportedTotalSize(24, 16))
         );
     }
+
+    #[test]
+    fn test_ref_from_slice_rejects_too_small_reported_size() {
+        #[rustfmt::skip]
+        let bytes = AlignedBytes::new(
+            [
+                0x37, 0x13, 0, 0,
+                /* Tag size */
+                4, 0, 0, 0,
+                /* Remaining bytes are irrelevant. */
+                0, 1, 2, 3,
+                0, 0, 0, 0,
+            ],
+        );
+
+        assert_eq!(
+            DynSizedStructure::<DummyTestHeader>::ref_from_slice(bytes.borrow()),
+            Err(MemoryError::SizeInsufficient(4, 8))
+        );
+    }
 }

multiboot2-common/src/tag.rs

@@ -48,6 +48,8 @@ pub trait MaybeDynSized: Pointee {
     /// Returns the corresponding [`Header`].
     fn header(&self) -> &Self::Header {
         let ptr = core::ptr::addr_of!(*self);
+        // SAFETY: `self` is a valid reference and `Self::Header` is the
+        // prefix of this `repr(C)` structure at the same address.
         unsafe { &*ptr.cast::<Self::Header>() }
     }
 
@@ -65,6 +67,8 @@ pub trait MaybeDynSized: Pointee {
         let ptr = core::ptr::addr_of!(*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.

multiboot2-common/src/test_utils.rs

@@ -69,8 +69,8 @@ impl DummyTestHeader {
 }
 
 impl Header for DummyTestHeader {
-    fn payload_len(&self) -> usize {
-        self.size as usize - size_of::<Self>()
+    fn total_size(&self) -> usize {
+        self.size as usize
     }
 
     fn set_size(&mut self, total_size: usize) {

multiboot2-header/src/header.rs

@@ -50,6 +50,8 @@ impl<'a> Multiboot2Header<'a> {
     ///   program may observe unsynchronized mutation.
     pub unsafe fn load(ptr: *const Multiboot2BasicHeader) -> Result<Self, LoadError> {
         let ptr = NonNull::new(ptr.cast_mut()).ok_or(LoadError::Memory(MemoryError::Null))?;
+        // SAFETY: `ptr` was checked for null and `ref_from_ptr` validates the
+        // reported total size before constructing the DST reference.
         let inner = unsafe { DynSizedStructure::ref_from_ptr(ptr).map_err(LoadError::Memory)? };
         let this = Self(inner);
 
@@ -69,9 +71,9 @@ impl<'a> Multiboot2Header<'a> {
     /// Checks whether the header has a valid, complete tag sequence.
     fn has_valid_tag_sequence(&self) -> Result<bool, MemoryError> {
         validate_tag_sequence(self.0.payload(), |tag| {
-            let typ = u16::from_ne_bytes(tag[0..2].try_into().unwrap());
-            let flags = u16::from_ne_bytes(tag[2..4].try_into().unwrap());
-            let size = u32::from_ne_bytes(tag[4..8].try_into().unwrap()) as usize;
+            let typ = u16::from_le_bytes(tag[0..2].try_into().unwrap());
+            let flags = u16::from_le_bytes(tag[2..4].try_into().unwrap());
+            let size = u32::from_le_bytes(tag[4..8].try_into().unwrap()) as usize;
 
             typ == HeaderTagType::End as u16
                 && flags == crate::HeaderTagFlag::Required as u16
@@ -163,14 +165,17 @@ impl<'a> Multiboot2Header<'a> {
             .wrapping_add(magic_begin_idx)
             .cast::<Multiboot2BasicHeader>();
 
+        // SAFETY: `ptr` points into `buffer`, which was checked for
+        // alignment and search-window bounds above.
         let header = unsafe { Self::load(ptr)? };
         Ok((header, magic_begin_idx))
     }
 
     /// Returns a [`TagIter`].
     #[must_use]
     pub fn iter(&self) -> TagIter<'_> {
-        TagIter::new(self.0.payload())
+        // SAFETY: We validated the chain of tags beforehand.
+        unsafe { TagIter::new(self.0.payload()) }
     }
 
     /// Wrapper around [`Multiboot2BasicHeader::verify_checksum`].
@@ -393,9 +398,8 @@ impl Multiboot2BasicHeader {
 }
 
 impl Header for Multiboot2BasicHeader {
-    fn payload_len(&self) -> usize {
-        assert!(self.length as usize >= size_of::<Self>());
-        self.length as usize - size_of::<Self>()
+    fn total_size(&self) -> usize {
+        self.length as usize
     }
 
     fn set_size(&mut self, total_size: usize) {

multiboot2-header/src/information_request.rs

@@ -26,6 +26,7 @@ impl InformationRequestHeaderTag {
     #[must_use]
     pub fn new(flags: HeaderTagFlag, requests: &[MbiTagTypeId]) -> Box<Self> {
         let header = HeaderTagHeader::new(HeaderTagType::InformationRequest, flags, 0);
+        // SAFETY: The memory we are using is valid.
         let requests = unsafe {
             let ptr = ptr::addr_of!(*requests);
             slice::from_raw_parts(ptr.cast::<u8>(), size_of_val(requests))

multiboot2-header/src/lib.rs

@@ -31,6 +31,7 @@
     clippy::all,
     clippy::cargo,
     clippy::nursery,
+    clippy::undocumented_unsafe_blocks,
     clippy::must_use_candidate,
     // clippy::restriction,
     // clippy::pedantic

multiboot2-header/src/tags.rs

@@ -106,8 +106,8 @@ impl HeaderTagHeader {
 }
 
 impl Header for HeaderTagHeader {
-    fn payload_len(&self) -> usize {
-        self.size as usize - size_of::<Self>()
+    fn total_size(&self) -> usize {
+        self.size as usize
     }
 
     fn set_size(&mut self, total_size: usize) {

multiboot2/CHANGELOG.md

@@ -13,6 +13,8 @@
 - Added UserDefined section to `ElfSectionType`.
 - Added equality implementations for `BootInformation`.
 - Fixed `BootInformation::load` to validate the complete padded tag sequence.
+- Fixed RSDP tag constructors to compute their checksums automatically and
+  tightened checksum validation to avoid reading past malformed lengths.
 - Fixed indexed framebuffer parsing to reject palette metadata that exceeds
   the tag payload.
 - Fixed EFI memory map parsing to reject descriptor sizes that cannot safely