various safety and correctness improvements

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

@@ -6,9 +6,6 @@ mod multiboot;
 
 extern crate alloc;
 
-#[macro_use]
-extern crate integration_test_util;
-
 use integration_test_util::init_environment;
 
 core::arch::global_asm!(include_str!("start.S"), options(att_syntax));

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

@@ -1,7 +1,6 @@
 //! Parsing the Multiboot information. Glue code for the [`multiboot`] code.
 
 use anyhow::anyhow;
-use core::ptr::addr_of_mut;
 use core::slice;
 use multiboot::information::{MemoryManagement, Multiboot, PAddr, SIGNATURE_EAX};
 
@@ -13,7 +12,7 @@ pub fn get_mbi<'a>(magic: u32, ptr: u32) -> anyhow::Result<Multiboot<'a, 'static
     if magic != SIGNATURE_EAX {
         return Err(anyhow!("Unknown Multiboot signature {magic:x}"));
     }
-    let mmgmt: &mut dyn MemoryManagement = unsafe { &mut *addr_of_mut!(MEMORY_MANAGEMENT) };
+    let mmgmt: &mut dyn MemoryManagement = unsafe { &mut *(&raw mut MEMORY_MANAGEMENT) };
     unsafe { Multiboot::from_ptr(ptr as u64, mmgmt) }.ok_or(anyhow!(
         "Can't read Multiboot boot information from pointer"
     ))

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);
+        let ptr = &raw const 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
@@ -256,7 +257,6 @@ pub use iter::TagIter;
 pub use tag::{MaybeDynSized, Tag};
 
 use core::fmt::Debug;
-use core::ptr;
 use core::ptr::NonNull;
 use core::slice;
 use thiserror::Error;
@@ -274,16 +274,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 +296,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 +333,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 +357,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 +378,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)
     }
 
@@ -405,10 +424,13 @@ 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.
-    pub fn cast<T: MaybeDynSized<Header = H> + ?Sized>(&self) -> &T {
+    pub fn cast<T: MaybeDynSized<Header = H> + ?Sized>(&self) -> &T
+    where
+        T::Metadata: Default,
+    {
         // Thin or fat pointer, depending on type.
         // However, only thin ptr is needed.
-        let base_ptr = ptr::addr_of!(*self);
+        let base_ptr = &raw const *self;
 
         // This should be a compile-time assertion. However, this is the best
         // location to place it for now.
@@ -417,6 +439,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 +480,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 +641,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

@@ -9,7 +9,10 @@ use ptr_meta::Pointee;
 /// [`DynSizedStructure::cast`].
 ///
 /// Structs that are a DST must provide a **correct** [`MaybeDynSized::dst_len`]
-/// implementation.
+/// implementation. The needed metadata type is either `()` for sized types or
+/// `usize` for dynamically sized types. For sized types, there is a default
+/// implementation. Only dynamically sized types need to implement
+/// [`MaybeDynSized::dst_len`].
 ///
 /// # ABI
 /// Implementors **must** use `#[repr(C)]`. As there might be padding necessary
@@ -43,11 +46,20 @@ pub trait MaybeDynSized: Pointee {
     ///
     /// For sized tags, this just returns `()`. For DSTs, this returns an
     /// `usize`.
-    fn dst_len(header: &Self::Header) -> Self::Metadata;
+    fn dst_len(header: &Self::Header) -> Self::Metadata
+    where
+        // Either `()` or `usize`, never something else
+        Self::Metadata: Default,
+    {
+        let _ = header;
+        Default::default()
+    }
 
     /// Returns the corresponding [`Header`].
     fn header(&self) -> &Self::Header {
-        let ptr = core::ptr::addr_of!(*self);
+        let ptr = &raw const *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>() }
     }
 
@@ -62,9 +74,11 @@ pub trait MaybeDynSized: Pointee {
     /// [`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> {
-        let ptr = core::ptr::addr_of!(*self);
+        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.