Merge pull request #226 from rust-osdev/tag-casting

multiboot2: massive refactoring, removed UB, Miri passes all tests

Author: phip1611

.github/workflows/_build-rust.yml

@@ -98,8 +98,11 @@ jobs:
         run: cargo test --verbose
       - name: Unit Test with Miri
         if: inputs.do-miri
-        # "--tests" so that the doctests are skipped. Currently, the doctest
-        # in miri fails.
         run: |
           rustup component add miri
-          cargo miri test --tests
+          # Run with stack-borrow model
+          # XXX Temporarily, just for multiboot2 crate.
+          cargo miri test -p multiboot2
+          # Run with tree-borrow model
+          # XXX Temporarily, just for multiboot2 crate.
+          MIRIFLAGS=-Zmiri-tree-borrows cargo +nightly miri test -p multiboot2

.github/workflows/rust.yml

@@ -24,7 +24,7 @@ jobs:
     name: build (msrv)
     uses: ./.github/workflows/_build-rust.yml
     with:
-      rust-version: 1.75.0 # MSRV
+      rust-version: 1.70.0 # MSRV
       do-style-check: false
       features: builder
 
@@ -50,7 +50,7 @@ jobs:
     needs: build_msrv
     uses: ./.github/workflows/_build-rust.yml
     with:
-      rust-version: 1.75.0 # MSRV
+      rust-version: 1.70.0 # MSRV
       do-style-check: false
       rust-target: thumbv7em-none-eabihf
       features: builder
@@ -107,7 +107,7 @@ jobs:
     needs: build_msrv
     uses: ./.github/workflows/_build-rust.yml
     with:
-      rust-version: 1.75.0 # MSRV
+      rust-version: 1.70.0 # MSRV
       do-style-check: true
       do-test: false
       features: builder

Cargo.lock

@@ -10,7 +10,7 @@ exclude = [
 
 [workspace.dependencies]
 bitflags = "2.6.0"
-derive_more = { version = "1.0.0", default-features = false, features = ["display"] }
+derive_more = { version = "~0.99.18", default-features = false, features = ["display"] }
 log = { version = "~0.4", default-features = false }
 
 # This way, the "multiboot2" dependency in the multiboot2-header crate can be

Cargo.toml

@@ -44,15 +44,15 @@ pub fn load_module(mut modules: multiboot::information::ModuleIter) -> ! {
 
     // build MBI
     let mbi = multiboot2::builder::InformationBuilder::new()
-        .bootloader_name_tag(BootLoaderNameTag::new("mb2_integrationtest_chainloader"))
-        .command_line_tag(CommandLineTag::new("chainloaded YEAH"))
+        .bootloader_name_tag(&BootLoaderNameTag::new("mb2_integrationtest_chainloader"))
+        .command_line_tag(&CommandLineTag::new("chainloaded YEAH"))
         // random non-sense memory map
-        .memory_map_tag(MemoryMapTag::new(&[MemoryArea::new(
+        .memory_map_tag(&MemoryMapTag::new(&[MemoryArea::new(
             0,
             0xffffffff,
             MemoryAreaType::Reserved,
         )]))
-        .add_module_tag(ModuleTag::new(
+        .add_module_tag(&ModuleTag::new(
             elf_mod.start as u32,
             elf_mod.end as u32,
             elf_mod.string.unwrap(),

integration-test/bins/Cargo.lock

@@ -26,7 +26,7 @@ readme = "README.md"
 homepage = "https://github.com/rust-osdev/multiboot2-header"
 repository = "https://github.com/rust-osdev/multiboot2"
 documentation = "https://docs.rs/multiboot2-header"
-rust-version = "1.75"
+rust-version = "1.70"
 
 [[example]]
 name = "minimal"

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

@@ -3,8 +3,6 @@
 ## Unreleased
 
 - **Breaking** All functions that returns something useful are now `#[must_use]`
-- updated dependencies
-- documentation enhancements
 
 ## 0.4.0 (2024-05-01)
 

multiboot2-header/Cargo.toml

@@ -77,7 +77,7 @@ bytes of the ELF. See Multiboot2 specification.
 
 ## MSRV
 
-The MSRV is 1.75.0 stable.
+The MSRV is 1.70.0 stable.
 
 ## License & Contribution
 

multiboot2-header/Changelog.md

@@ -255,6 +255,7 @@ impl HeaderBuilder {
     /// Adds information requests from the
     /// [`InformationRequestHeaderTagBuilder`] to the builder.
     #[must_use]
+    #[allow(clippy::missing_const_for_fn)] // only in Rust 1.70 necessary
     pub fn information_request_tag(
         mut self,
         information_request_tag: InformationRequestHeaderTagBuilder,

multiboot2-header/README.md

@@ -34,7 +34,7 @@
 //!
 //! ## MSRV
 //!
-//! The MSRV is 1.75.0 stable.
+//! The MSRV is 1.70.0 stable.
 
 #![no_std]
 #![cfg_attr(feature = "unstable", feature(error_in_core))]

multiboot2-header/src/builder/header.rs

@@ -31,7 +31,7 @@ readme = "README.md"
 homepage = "https://github.com/rust-osdev/multiboot2"
 repository = "https://github.com/rust-osdev/multiboot2"
 documentation = "https://docs.rs/multiboot2"
-rust-version = "1.75"
+rust-version = "1.70"
 
 [features]
 default = ["builder"]
@@ -45,12 +45,14 @@ bitflags.workspace = true
 derive_more.workspace = true
 log.workspace = true
 
+ptr_meta = { version = "~0.2", default-features = false }
 # We only use a very basic type definition from this crate. To prevent MSRV
 # bumps from uefi-raw, I restrict this here. Upstream users are likely to have
 # two versions of this library in it, which is no problem, as we only use the
 # type definition.
 uefi-raw = { version = "~0.5", default-features = false }
-ptr_meta = { version = "~0.2", default-features = false }
+
+[dev-dependencies]
 
 [package.metadata.docs.rs]
 all-features = true

multiboot2-header/src/lib.rs

@@ -2,14 +2,38 @@
 
 ## Unreleased
 
-- **Breaking** All functions that returns something useful are now `#[must_use]`
-- **Breaking** More public fields in tags were replaced by public getters, such
+-
+
+## 0.21.0 (2024-08-17)
+
+This release contains a massive refactoring of various internals. Now, **all
+unit tests pass Miri**, thus we removed lots of undefined behaviour and
+increased the memory safety! 🎉 Only a small part of these internal refactorings
+leak to the public interface. If you don't provide external custom tags, you
+should be fine.
+
+Please note that **all previous releases** must be considered unsafe, as they
+contain UB. However, it is never clear how UB results in immediate incorrect
+behaviour and it _might_ work. **Nevertheless, please migrate to the latest
+release and you'll be fine!**
+
+All previous releases on crates.io have been yanked.
+
+- **Breaking:** All functions that returns something useful are
+  now `#[must_use]`
+- **Breaking:** More public fields in tags were replaced by public getters, such
   as `SmbiosTag::major()`
-- updated dependencies
-- MSRV is 1.75
+- **Breaking:** Methods of `InformationBuilder` to add tags now consume
+  references instead of owned values
+- **Breaking:** The `BoxedDst` has been removed in favor of a normal Rust `Box`.
+  This only affects you if you use the `builder` feature.
+- **Breaking:** MSRV is 1.75
+- **Breaking:** Introduced new `TagHeader` type as replacement for the `Tag`
+  type that will be changed in the next step. `Tag` has been renamed to an
+  internal-only `GenericTag` type.
+- Added missing `InformationBuilder::vbe_info_tag`
 - documentation enhancements
-- Introduced new `TagHeader` type as replacement for the `Tag` type that will
-  be changed in the next step.
+- updated dependencies
 
 ## 0.20.2 (2024-05-26)
 

multiboot2/Cargo.toml

@@ -45,7 +45,7 @@ tag_, which is a tag of type `0` and size `8`.
 
 ## MSRV
 
-The MSRV is 1.75.0 stable.
+The MSRV is 1.70.0 stable.
 
 ## License & Contribution
 

multiboot2/Changelog.md

@@ -21,15 +21,15 @@ use derive_more::Display;
 pub enum MbiLoadError {
     /// The address is invalid. Make sure that the address is 8-byte aligned,
     /// according to the spec.
-    #[display("The address is invalid")]
+    #[display(fmt = "The address is invalid")]
     IllegalAddress,
     /// The total size of the multiboot2 information structure must be not zero
     /// and a multiple of 8.
-    #[display("The size of the MBI is unexpected")]
+    #[display(fmt = "The size of the MBI is unexpected")]
     IllegalTotalSize(u32),
     /// Missing end tag. Each multiboot2 boot information requires to have an
     /// end tag.
-    #[display("There is no end tag")]
+    #[display(fmt = "There is no end tag")]
     NoEndTag,
 }
 
@@ -38,7 +38,7 @@ impl core::error::Error for MbiLoadError {}
 
 /// The basic header of a [`BootInformation`] as sized Rust type.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
-#[repr(C)]
+#[repr(C, align(8))]
 pub struct BootInformationHeader {
     // size is multiple of 8
     total_size: u32,
@@ -68,7 +68,7 @@ impl AsBytes for BootInformationHeader {}
 /// 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)]
+#[repr(C, align(8))]
 struct BootInformationInner {
     header: BootInformationHeader,
     tags: [u8],
@@ -221,6 +221,8 @@ impl<'a> BootInformation<'a> {
     /// Otherwise, if the [`TagType::EfiBs`] tag is present, this returns `None`
     /// as it is strictly recommended to get the memory map from the `uefi`
     /// services.
+    ///
+    /// [`TagType::EfiBs`]: crate::TagType::EfiBs
     #[must_use]
     pub fn efi_memory_map_tag(&self) -> Option<&EFIMemoryMapTag> {
         // If the EFIBootServicesNotExited is present, then we should not use
@@ -277,8 +279,8 @@ impl<'a> BootInformation<'a> {
     pub fn elf_sections(&self) -> Option<ElfSectionIter> {
         let tag = self.get_tag::<ElfSectionsTag>();
         tag.map(|t| {
-            assert!((t.entry_size * t.shndx) <= t.size);
-            t.sections()
+            assert!((t.entry_size() * t.shndx()) <= t.size() as u32);
+            t.sections_iter()
         })
     }
 
@@ -359,7 +361,7 @@ impl<'a> BootInformation<'a> {
     /// special handling is required. This is reflected by code-comments.
     ///
     /// ```no_run
-    /// use multiboot2::{BootInformation, BootInformationHeader, StringError, Tag, TagTrait, TagType, TagTypeId};
+    /// use multiboot2::{BootInformation, BootInformationHeader, parse_slice_as_string, StringError, TagHeader, TagTrait, TagType, TagTypeId};
     ///
     /// #[repr(C)]
     /// #[derive(multiboot2::Pointee)] // Only needed for DSTs.
@@ -374,17 +376,17 @@ impl<'a> BootInformation<'a> {
     /// impl TagTrait for CustomTag {
     ///     const ID: TagType = TagType::Custom(0x1337);
     ///
-    ///     fn dst_size(base_tag: &Tag) -> usize {
+    ///     fn dst_len(header: &TagHeader) -> usize {
     ///         // The size of the sized portion of the custom tag.
     ///         let tag_base_size = 8; // id + size is 8 byte in size
-    ///         assert!(base_tag.size >= 8);
-    ///         base_tag.size as usize - tag_base_size
+    ///         assert!(header.size >= 8);
+    ///         header.size as usize - tag_base_size
     ///     }
     /// }
     ///
     /// impl CustomTag {
     ///     fn name(&self) -> Result<&str, StringError> {
-    ///         Tag::parse_slice_as_string(&self.name)
+    ///         parse_slice_as_string(&self.name)
     ///     }
     /// }
     /// let mbi_ptr = 0xdeadbeef as *const BootInformationHeader;
@@ -395,11 +397,13 @@ impl<'a> BootInformation<'a> {
     ///     .unwrap();
     /// assert_eq!(tag.name(), Ok("name"));
     /// ```
+    ///
+    /// [`TagType`]: crate::TagType
     #[must_use]
     pub fn get_tag<TagT: TagTrait + ?Sized + 'a>(&'a self) -> Option<&'a TagT> {
         self.tags()
-            .find(|tag| tag.typ == TagT::ID)
-            .map(|tag| tag.cast_tag::<TagT>())
+            .find(|tag| tag.header().typ == TagT::ID)
+            .map(|tag| tag.cast::<TagT>())
     }
 
     /// Returns an iterator over all tags.
@@ -440,7 +444,7 @@ impl fmt::Debug for BootInformation<'_> {
             if elf_sections_tag_entries_count > ELF_SECTIONS_LIMIT {
                 debug.field("elf_sections (count)", &elf_sections_tag_entries_count);
             } else {
-                debug.field("elf_sections", &self.elf_sections().unwrap_or_default());
+                debug.field("elf_sections", &self.elf_sections());
             }
         }