multiboot2: memory bug fix

Author: phip1611

multiboot2/Changelog.md

@@ -14,6 +14,8 @@
 - `EFIMemoryAreaType` was removed and is now an alias of
   `uefi_raw::table::boot::MemoryType`
 - MSRV is 1.68.0
+- **BREAKING** Removed `MemoryAreaIter` and `MemoryMapTag::available_memory_areas`
+- Added `MemoryMapTag::entry_size` and `MemoryMapTag::entry_version`
 
 ## 0.15.1 (2023-03-18)
 - **BREAKING** `MemoryMapTag::all_memory_areas()` was renamed to `memory_areas`

multiboot2/src/lib.rs

@@ -58,7 +58,7 @@ pub use framebuffer::{FramebufferColor, FramebufferField, FramebufferTag, Frameb
 pub use image_load_addr::ImageLoadPhysAddr;
 pub use memory_map::{
     BasicMemoryInfoTag, EFIBootServicesNotExited, EFIMemoryAreaType, EFIMemoryDesc,
-    EFIMemoryMapTag, MemoryArea, MemoryAreaIter, MemoryAreaType, MemoryMapTag,
+    EFIMemoryMapTag, MemoryArea, MemoryAreaType, MemoryMapTag,
 };
 pub use module::{ModuleIter, ModuleTag};
 pub use rsdp::{RsdpV1Tag, RsdpV2Tag};
@@ -514,7 +514,10 @@ impl fmt::Debug for BootInformation {
 /// This crate uses the [`Pointee`]-abstraction of the [`ptr_meta`] crate to
 /// create fat pointers.
 pub trait TagTrait: Pointee {
-    /// Returns
+    /// Returns the amount of items in the dynamically sized portion of the
+    /// DST. Note that this is not the amount of bytes. So if the dynamically
+    /// sized portion is 16 bytes in size and each element is 4 bytes big, then
+    /// this function must return 4.
     fn dst_size(base_tag: &Tag) -> Self::Metadata;
 
     /// Creates a reference to a (dynamically sized) tag type in a safe way.
@@ -706,16 +709,16 @@ mod tests {
             FramebufferType::RGB {
                 red: FramebufferField {
                     position: 16,
-                    size: 8
+                    size: 8,
                 },
                 green: FramebufferField {
                     position: 8,
-                    size: 8
+                    size: 8,
                 },
                 blue: FramebufferField {
                     position: 0,
-                    size: 8
-                }
+                    size: 8,
+                },
             }
         );
     }
@@ -767,22 +770,22 @@ mod tests {
                     FramebufferColor {
                         red: 255,
                         green: 0,
-                        blue: 0
+                        blue: 0,
                     },
                     FramebufferColor {
                         red: 0,
                         green: 255,
-                        blue: 0
+                        blue: 0,
                     },
                     FramebufferColor {
                         red: 0,
                         green: 0,
-                        blue: 255
+                        blue: 255,
                     },
                     FramebufferColor {
                         red: 0,
                         green: 0,
-                        blue: 0
+                        blue: 0,
                     }
                 ]
             ),
@@ -915,28 +918,28 @@ mod tests {
             vbe.mode_info.red_field,
             VBEField {
                 position: 16,
-                size: 8
+                size: 8,
             }
         );
         assert_eq!(
             vbe.mode_info.green_field,
             VBEField {
                 position: 8,
-                size: 8
+                size: 8,
             }
         );
         assert_eq!(
             vbe.mode_info.blue_field,
             VBEField {
                 position: 0,
-                size: 8
+                size: 8,
             }
         );
         assert_eq!(
             vbe.mode_info.reserved_field,
             VBEField {
                 position: 24,
-                size: 8
+                size: 8,
             }
         );
         assert_eq!(
@@ -1218,27 +1221,33 @@ mod tests {
         let addr = bytes.0.as_ptr() as usize;
         let bi = unsafe { load(addr) };
         let bi = bi.unwrap();
-        test_grub2_boot_info(bi, addr, string_addr, &bytes.0, &string_bytes.0);
+
+        test_grub2_boot_info(&bi, addr, string_addr, &bytes.0, &string_bytes.0);
         let bi = unsafe { load_with_offset(addr, 0) };
         let bi = bi.unwrap();
-        test_grub2_boot_info(bi, addr, string_addr, &bytes.0, &string_bytes.0);
+        test_grub2_boot_info(&bi, addr, string_addr, &bytes.0, &string_bytes.0);
         let offset = 8usize;
         for i in 0..8 {
             bytes.0[796 + i] = ((string_addr - offset as u64) >> (i * 8)) as u8;
         }
         let bi = unsafe { load_with_offset(addr - offset, offset) };
         let bi = bi.unwrap();
         test_grub2_boot_info(
-            bi,
+            &bi,
             addr,
             string_addr - offset as u64,
             &bytes.0,
             &string_bytes.0,
         );
+
+        // Check that the MBI's debug output can be printed without SEGFAULT.
+        // If this works, it is a good indicator than transitively a lot of
+        // stuff works.
+        println!("{bi:#?}");
     }
 
     fn test_grub2_boot_info(
-        bi: BootInformation,
+        bi: &BootInformation,
         addr: usize,
         string_addr: u64,
         bytes: &[u8],
@@ -1317,7 +1326,12 @@ mod tests {
         assert_eq!(ElfSectionFlags::empty(), s8.flags());
         assert_eq!(ElfSectionType::StringTable, s8.section_type());
         assert!(es.next().is_none());
-        let mut mm = bi.memory_map_tag().unwrap().available_memory_areas();
+        let mut mm = bi
+            .memory_map_tag()
+            .unwrap()
+            .memory_areas()
+            .iter()
+            .filter(|area| area.typ() == MemoryAreaType::Available);
         let mm1 = mm.next().unwrap();
         assert_eq!(0x00000000, mm1.start_address());
         assert_eq!(0x009_FC00, mm1.end_address());

multiboot2/src/memory_map.rs

@@ -3,6 +3,7 @@ use core::convert::TryInto;
 use core::fmt::Debug;
 use core::marker::PhantomData;
 use core::mem;
+use std::mem::size_of;
 
 pub use uefi_raw::table::boot::MemoryDescriptor as EFIMemoryDesc;
 pub use uefi_raw::table::boot::MemoryType as EFIMemoryAreaType;
@@ -44,31 +45,29 @@ impl MemoryMapTag {
         boxed_dst_tag(TagType::Mmap, bytes.as_slice())
     }
 
-    /// Return an iterator over all memory areas that have the type
-    /// [`MemoryAreaType::Available`].
-    pub fn available_memory_areas(&self) -> impl Iterator<Item = &MemoryArea> {
-        self.memory_areas()
-            .filter(|entry| matches!(entry.typ, MemoryAreaType::Available))
+    /// Returns the entry size.
+    pub fn entry_size(&self) -> u32 {
+        self.entry_size
     }
 
-    /// Return an iterator over all memory areas.
-    pub fn memory_areas(&self) -> MemoryAreaIter {
-        let self_ptr = self as *const MemoryMapTag;
-        let start_area = (&self.areas[0]) as *const MemoryArea;
-        MemoryAreaIter {
-            current_area: start_area as u64,
-            // NOTE: `last_area` is only a bound, it doesn't necessarily point exactly to the last element
-            last_area: (self_ptr as *const () as u64 + (self.size - self.entry_size) as u64),
-            entry_size: self.entry_size,
-            phantom: PhantomData,
-        }
+    /// Returns the entry version.
+    pub fn entry_version(&self) -> u32 {
+        self.entry_version
+    }
+
+    /// Return the slice with all memory areas.
+    pub fn memory_areas(&self) -> &[MemoryArea] {
+        // If this ever fails, we need to model this differently in this crate.
+        assert_eq!(self.entry_size as usize, size_of::<MemoryArea>());
+        &self.areas
     }
 }
 
 impl TagTrait for MemoryMapTag {
     fn dst_size(base_tag: &Tag) -> usize {
         assert!(base_tag.size as usize >= METADATA_SIZE);
-        base_tag.size as usize - METADATA_SIZE
+        let size = base_tag.size as usize - METADATA_SIZE;
+        size / size_of::<MemoryArea>()
     }
 }
 
@@ -152,28 +151,6 @@ pub enum MemoryAreaType {
     Defective = 5,
 }
 
-/// An iterator over all memory areas
-#[derive(Clone, Debug)]
-pub struct MemoryAreaIter<'a> {
-    current_area: u64,
-    last_area: u64,
-    entry_size: u32,
-    phantom: PhantomData<&'a MemoryArea>,
-}
-
-impl<'a> Iterator for MemoryAreaIter<'a> {
-    type Item = &'a MemoryArea;
-    fn next(&mut self) -> Option<&'a MemoryArea> {
-        if self.current_area > self.last_area {
-            None
-        } else {
-            let area = unsafe { &*(self.current_area as *const MemoryArea) };
-            self.current_area += self.entry_size as u64;
-            Some(area)
-        }
-    }
-}
-
 /// Basic memory info
 ///
 /// This tag includes "basic memory information".
@@ -279,7 +256,8 @@ impl EFIMemoryMapTag {
 impl TagTrait for EFIMemoryMapTag {
     fn dst_size(base_tag: &Tag) -> usize {
         assert!(base_tag.size as usize >= EFI_METADATA_SIZE);
-        base_tag.size as usize - EFI_METADATA_SIZE
+        let size = base_tag.size as usize - EFI_METADATA_SIZE;
+        size / size_of::<EFIMemoryDesc>()
     }
 }