Merge pull request #155 from rust-osdev/dev7

multiboot2: model MBI as DST

Author: phip1611

multiboot2/src/builder/information.rs

@@ -1,7 +1,7 @@
 //! Exports item [`InformationBuilder`].
 use crate::builder::traits::StructAsBytes;
 use crate::{
-    BasicMemoryInfoTag, BootInformationInner, BootLoaderNameTag, CommandLineTag,
+    BasicMemoryInfoTag, BootInformationHeader, BootLoaderNameTag, CommandLineTag,
     EFIBootServicesNotExitedTag, EFIImageHandle32Tag, EFIImageHandle64Tag, EFIMemoryMapTag,
     EFISdt32Tag, EFISdt64Tag, ElfSectionsTag, EndTag, FramebufferTag, ImageLoadPhysAddrTag,
     MemoryMapTag, ModuleTag, RsdpV1Tag, RsdpV2Tag, SmbiosTag,
@@ -74,7 +74,7 @@ impl InformationBuilder {
     /// Returns the expected length of the Multiboot2 header,
     /// when the `build()`-method gets called.
     pub fn expected_len(&self) -> usize {
-        let base_len = size_of::<BootInformationInner>();
+        let base_len = size_of::<BootInformationHeader>();
         // size_or_up_aligned not required, because length is 16 and the
         // begin is 8 byte aligned => first tag automatically 8 byte aligned
         let mut len = Self::size_or_up_aligned(base_len);
@@ -156,7 +156,7 @@ impl InformationBuilder {
         Self::build_add_bytes(
             &mut data,
             // important that we write the correct expected length into the header!
-            &BootInformationInner::new(self.expected_len() as u32).struct_as_bytes(),
+            &BootInformationHeader::new(self.expected_len() as u32).struct_as_bytes(),
             false,
         );
 
@@ -327,7 +327,7 @@ mod tests {
         assert_eq!(builder.expected_len(), expected_len);
 
         let mb2i_data = builder.build();
-        let mb2i = unsafe { BootInformation::load(mb2i_data.as_ptr()) }
+        let mb2i = unsafe { BootInformation::load(mb2i_data.as_ptr().cast()) }
             .expect("the generated information to be readable");
         println!("{:#?}", mb2i);
         assert_eq!(mb2i.basic_memory_info_tag().unwrap().memory_lower(), 640);

multiboot2/src/lib.rs

@@ -22,10 +22,13 @@
 //! ## Example
 //!
 //! ```rust
-//! use multiboot2::BootInformation;
-//! fn kmain(multiboot_info_ptr: u32) {
-//!     let boot_info = unsafe { BootInformation::load(multiboot_info_ptr as *const u8).unwrap() };
-//!     println!("{:?}", boot_info);
+//! use multiboot2::{BootInformation, BootInformationHeader};
+//!
+//! fn kernel_entry(mb_magic: u32, mbi_ptr: u32) {
+//!     if mb_magic == multiboot2::MAGIC {
+//!         let boot_info = unsafe { BootInformation::load(mbi_ptr as *const BootInformationHeader).unwrap() };
+//!         let _cmd = boot_info.command_line_tag();
+//!     } else { /* Panic or use multiboot1 flow. */ }
 //! }
 //! ```
 //!
@@ -41,6 +44,7 @@ extern crate alloc;
 extern crate std;
 
 use core::fmt;
+use core::mem::size_of;
 use derive_more::Display;
 // Must be public so that custom tags can be DSTs.
 pub use ptr_meta::Pointee;
@@ -102,7 +106,7 @@ pub const MAGIC: u32 = 0x36d76289;
 /// Deprecated. Please use BootInformation::load() instead.
 #[deprecated = "Please use BootInformation::load() instead."]
 pub unsafe fn load<'a>(address: usize) -> Result<BootInformation<'a>, MbiLoadError> {
-    let ptr = address as *const u8;
+    let ptr = address as *const BootInformationHeader;
     BootInformation::load(ptr)
 }
 
@@ -139,58 +143,77 @@ pub enum MbiLoadError {
 #[cfg(feature = "unstable")]
 impl core::error::Error for MbiLoadError {}
 
+/// The basic header of a boot information.
+#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 #[repr(C, align(8))]
-struct BootInformationInner {
+pub struct BootInformationHeader {
+    // size is multiple of 8
     total_size: u32,
     _reserved: u32,
-    // followed by various, dynamically sized multiboot2 tags
-    tags: [Tag; 0],
+    // Followed by the boot information tags.
 }
 
-impl BootInformationInner {
+impl BootInformationHeader {
     #[cfg(feature = "builder")]
     fn new(total_size: u32) -> Self {
         Self {
             total_size,
             _reserved: 0,
-            tags: [],
         }
     }
+}
+
+#[cfg(feature = "builder")]
+impl StructAsBytes for BootInformationHeader {
+    fn byte_size(&self) -> usize {
+        core::mem::size_of::<Self>()
+    }
+}
 
+/// This type holds the whole data of the MBI. This helps to better satisfy miri
+/// when it checks for memory issues.
+#[derive(ptr_meta::Pointee)]
+#[repr(C)]
+struct BootInformationInner {
+    header: BootInformationHeader,
+    tags: [u8],
+}
+
+impl BootInformationInner {
+    /// Checks if the MBI has a valid end tag by checking the end of the mbi's
+    /// bytes.
     fn has_valid_end_tag(&self) -> bool {
         let end_tag_prototype = EndTag::default();
 
-        let self_ptr = self as *const _;
-        let end_tag_addr = self_ptr as usize + (self.total_size - end_tag_prototype.size) as usize;
-        let end_tag = unsafe { &*(end_tag_addr as *const Tag) };
+        let self_ptr = unsafe { self.tags.as_ptr().sub(size_of::<BootInformationHeader>()) };
 
-        end_tag.typ == end_tag_prototype.typ && end_tag.size == end_tag_prototype.size
-    }
-}
+        let end_tag_ptr = unsafe {
+            self_ptr
+                .add(self.header.total_size as usize)
+                .sub(size_of::<EndTag>())
+        };
+        let end_tag = unsafe { &*(end_tag_ptr as *const EndTag) };
 
-#[cfg(feature = "builder")]
-impl StructAsBytes for BootInformationInner {
-    fn byte_size(&self) -> usize {
-        core::mem::size_of::<Self>()
+        end_tag.typ == end_tag_prototype.typ && end_tag.size == end_tag_prototype.size
     }
 }
 
 /// A Multiboot 2 Boot Information (MBI) accessor.
 #[repr(transparent)]
 pub struct BootInformation<'a>(&'a BootInformationInner);
 
-impl BootInformation<'_> {
+impl<'a> BootInformation<'a> {
     /// Loads the [`BootInformation`] from a pointer. The pointer must be valid
     /// and aligned to an 8-byte boundary, as defined by the spec.
     ///
     /// ## Example
     ///
     /// ```rust
-    /// use multiboot2::BootInformation;
+    /// use multiboot2::{BootInformation, BootInformationHeader};
     ///
     /// fn kernel_entry(mb_magic: u32, mbi_ptr: u32) {
     ///     if mb_magic == multiboot2::MAGIC {
-    ///         let boot_info = unsafe { BootInformation::load(mbi_ptr as *const u8).unwrap() };
+    ///         let boot_info = unsafe { BootInformation::load(mbi_ptr as *const BootInformationHeader).unwrap() };
     ///         let _cmd = boot_info.command_line_tag();
     ///     } else { /* Panic or use multiboot1 flow. */ }
     /// }
@@ -203,20 +226,26 @@ impl BootInformation<'_> {
     ///   boot environments, such as UEFI.
     /// * The memory at `ptr` must not be modified after calling `load` or the
     ///   program may observe unsynchronized mutation.
-    pub unsafe fn load(ptr: *const u8) -> Result<Self, MbiLoadError> {
+    pub unsafe fn load(ptr: *const BootInformationHeader) -> Result<Self, MbiLoadError> {
         // null or not aligned
         if ptr.is_null() || ptr.align_offset(8) != 0 {
             return Err(MbiLoadError::IllegalAddress);
         }
 
-        let mbi = &*ptr.cast::<BootInformationInner>();
+        // mbi: reference to basic header
+        let mbi = &*ptr;
 
         // Check if total size is a multiple of 8.
         // See MbiLoadError::IllegalTotalSize for comments
         if mbi.total_size & 0b111 != 0 {
             return Err(MbiLoadError::IllegalTotalSize(mbi.total_size));
         }
 
+        let slice_size = mbi.total_size as usize - size_of::<BootInformationHeader>();
+        // mbi: reference to full mbi
+        let mbi = ptr_meta::from_raw_parts::<BootInformationInner>(ptr.cast(), slice_size);
+        let mbi = &*mbi;
+
         if !mbi.has_valid_end_tag() {
             return Err(MbiLoadError::NoEndTag);
         }
@@ -226,7 +255,7 @@ impl BootInformation<'_> {
 
     /// Get the start address of the boot info.
     pub fn start_address(&self) -> usize {
-        core::ptr::addr_of!(*self.0) as usize
+        self.as_ptr() as usize
     }
 
     /// Get the start address of the boot info as pointer.
@@ -248,7 +277,7 @@ impl BootInformation<'_> {
 
     /// Get the total size of the boot info struct.
     pub fn total_size(&self) -> usize {
-        self.0.total_size as usize
+        self.0.header.total_size as usize
     }
 
     /// Search for the basic memory info tag.
@@ -425,20 +454,19 @@ impl BootInformation<'_> {
     ///     .unwrap();
     /// assert_eq!(tag.name(), Ok("name"));
     /// ```
-    pub fn get_tag<TagT: TagTrait + ?Sized, TagType: Into<TagTypeId>>(
-        &self,
+    pub fn get_tag<TagT: TagTrait + ?Sized + 'a, TagType: Into<TagTypeId>>(
+        &'a self,
         typ: TagType,
-    ) -> Option<&TagT> {
+    ) -> Option<&'a TagT> {
         let typ = typ.into();
         self.tags()
             .find(|tag| tag.typ == typ)
             .map(|tag| tag.cast_tag::<TagT>())
     }
 
+    /// Returns an iterator over all tags.
     fn tags(&self) -> TagIter {
-        // The first tag starts 8 bytes after the begin of the boot info header
-        let ptr = core::ptr::addr_of!(self.0.tags).cast();
-        TagIter::new(ptr)
+        TagIter::new(&self.0.tags)
     }
 }
 
@@ -525,8 +553,8 @@ pub trait TagTrait: Pointee {
     /// sane and the underlying memory valid. The implementation of this trait
     /// **must have** a correct [`Self::dst_size`] implementation.
     unsafe fn from_base_tag<'a>(tag: &Tag) -> &'a Self {
-        let ptr = tag as *const _ as *const ();
-        let ptr = ptr_meta::from_raw_parts(ptr, Self::dst_size(tag));
+        let ptr = core::ptr::addr_of!(*tag);
+        let ptr = ptr_meta::from_raw_parts(ptr.cast(), Self::dst_size(tag));
         &*ptr
     }
 }
@@ -1520,7 +1548,7 @@ mod tests {
             0, 0, 0, 0, // end tag type.
             8, 0, 0, 0, // end tag size.
         ]);
-        let bi = unsafe { BootInformation::load(bytes2.0.as_ptr()) };
+        let bi = unsafe { BootInformation::load(bytes2.0.as_ptr().cast()) };
         let bi = bi.unwrap();
         let efi_mmap = bi.efi_memory_map_tag();
         assert!(efi_mmap.is_none());

multiboot2/src/tag_type.rs

@@ -297,14 +297,15 @@ mod partial_eq_impls {
 #[derive(Clone, Copy)]
 #[repr(C)]
 pub struct Tag {
-    pub typ: TagTypeId, // u32
+    // u32
+    pub typ: TagTypeId,
     pub size: u32,
     // additional, tag specific fields
 }
 
 impl Tag {
     /// Casts the base tag to the specific tag type.
-    pub fn cast_tag<'a, T: TagTrait + ?Sized>(&self) -> &'a T {
+    pub fn cast_tag<'a, T: TagTrait + ?Sized + 'a>(&'a self) -> &'a T {
         // Safety: At this point, we trust that "self.size" and the size hint
         // for DST tags are sane.
         unsafe { TagTrait::from_base_tag(self) }
@@ -377,17 +378,25 @@ impl StructAsBytes for EndTag {
     }
 }
 
+/// Iterates the MBI's tags from the first tag to the end tag.
 #[derive(Clone, Debug)]
 pub struct TagIter<'a> {
+    /// Pointer to the next tag. Updated in each iteration.
     pub current: *const Tag,
-    phantom: PhantomData<&'a Tag>,
+    /// The pointer right after the MBI. Used for additional bounds checking.
+    end_ptr_exclusive: *const u8,
+    /// Lifetime capture of the MBI's memory.
+    _mem: PhantomData<&'a ()>,
 }
 
 impl<'a> TagIter<'a> {
-    pub fn new(first: *const Tag) -> Self {
+    /// Creates a new iterator
+    pub fn new(mem: &'a [u8]) -> Self {
+        assert_eq!(mem.as_ptr().align_offset(8), 0);
         TagIter {
-            current: first,
-            phantom: PhantomData,
+            current: mem.as_ptr().cast(),
+            end_ptr_exclusive: unsafe { mem.as_ptr().add(mem.len()) },
+            _mem: PhantomData,
         }
     }
 }
@@ -396,17 +405,25 @@ impl<'a> Iterator for TagIter<'a> {
     type Item = &'a Tag;
 
     fn next(&mut self) -> Option<&'a Tag> {
-        match unsafe { &*self.current } {
+        // This never failed so far. But better be safe.
+        assert!(self.current.cast::<u8>() < self.end_ptr_exclusive);
+
+        let tag = unsafe { &*self.current };
+        match tag {
             &Tag {
                 // END-Tag
                 typ: TagTypeId(0),
                 size: 8,
             } => None, // end tag
             tag => {
+                // We return the tag and update self.current already to the next
+                // tag.
+
+                // next pointer (rounded up to 8-byte alignment)
+                let ptr_offset = (tag.size as usize + 7) & !7;
+
                 // go to next tag
-                let mut tag_addr = self.current as usize;
-                tag_addr += ((tag.size + 7) & !7) as usize; //align at 8 byte
-                self.current = tag_addr as *const _;
+                self.current = unsafe { self.current.cast::<u8>().add(ptr_offset).cast::<Tag>() };
 
                 Some(tag)
             }