From 12b74ed7a0ff23f42d8bdccc6d7723d681fde50a Mon Sep 17 00:00:00 2001 From: Niklas Sombert Date: Wed, 15 Mar 2023 21:59:31 +0100 Subject: [PATCH 1/4] multiboot2: Get a mutable reference to the memory map --- .../src/verify/chainloader.rs | 6 +- .../multiboot2_payload/src/verify/grub.rs | 6 +- .../bins/multiboot2_payload/src/verify/mod.rs | 10 +- multiboot2/src/lib.rs | 93 ++++++++++++++++--- multiboot2/src/memory_map.rs | 7 ++ multiboot2/src/tag.rs | 57 ++++++++++++ multiboot2/src/tag_trait.rs | 13 +++ 7 files changed, 170 insertions(+), 22 deletions(-) diff --git a/integration-test/bins/multiboot2_payload/src/verify/chainloader.rs b/integration-test/bins/multiboot2_payload/src/verify/chainloader.rs index ef2ab7ed..a0c753a9 100644 --- a/integration-test/bins/multiboot2_payload/src/verify/chainloader.rs +++ b/integration-test/bins/multiboot2_payload/src/verify/chainloader.rs @@ -1,7 +1,7 @@ use crate::verify::{print_memory_map, print_module_info}; -use multiboot2::BootInformation; +use multiboot2::{BootInformation, BootInformationInner}; -pub fn run(mbi: &BootInformation) -> anyhow::Result<()> { +pub fn run>(mbi: &BootInformation) -> anyhow::Result<()> { basic_sanity_checks(mbi)?; print_memory_map(mbi)?; print_module_info(mbi)?; @@ -9,7 +9,7 @@ pub fn run(mbi: &BootInformation) -> anyhow::Result<()> { Ok(()) } -fn basic_sanity_checks(mbi: &BootInformation) -> anyhow::Result<()> { +fn basic_sanity_checks>(mbi: &BootInformation) -> anyhow::Result<()> { // Some basic sanity checks let bootloader_name = mbi .boot_loader_name_tag() diff --git a/integration-test/bins/multiboot2_payload/src/verify/grub.rs b/integration-test/bins/multiboot2_payload/src/verify/grub.rs index 9954e835..fd18a496 100644 --- a/integration-test/bins/multiboot2_payload/src/verify/grub.rs +++ b/integration-test/bins/multiboot2_payload/src/verify/grub.rs @@ -1,7 +1,7 @@ use crate::verify::{print_elf_info, print_memory_map, print_module_info}; -use multiboot2::BootInformation; +use multiboot2::{BootInformation, BootInformationInner}; -pub fn run(mbi: &BootInformation) -> anyhow::Result<()> { +pub fn run>(mbi: &BootInformation) -> anyhow::Result<()> { basic_sanity_checks(mbi)?; print_memory_map(mbi)?; print_module_info(mbi)?; @@ -9,7 +9,7 @@ pub fn run(mbi: &BootInformation) -> anyhow::Result<()> { Ok(()) } -fn basic_sanity_checks(mbi: &BootInformation) -> anyhow::Result<()> { +fn basic_sanity_checks>(mbi: &BootInformation) -> anyhow::Result<()> { // Some basic sanity checks let bootloader_name = mbi .boot_loader_name_tag() diff --git a/integration-test/bins/multiboot2_payload/src/verify/mod.rs b/integration-test/bins/multiboot2_payload/src/verify/mod.rs index 01f82269..681f2f7e 100644 --- a/integration-test/bins/multiboot2_payload/src/verify/mod.rs +++ b/integration-test/bins/multiboot2_payload/src/verify/mod.rs @@ -3,9 +3,9 @@ mod grub; use alloc::format; use alloc::vec::Vec; -use multiboot2::BootInformation; +use multiboot2::{BootInformation, BootInformationInner}; -pub fn run(mbi: &BootInformation) -> anyhow::Result<()> { +pub fn run>(mbi: &BootInformation) -> anyhow::Result<()> { println!("{mbi:#x?}"); println!(); @@ -27,7 +27,7 @@ pub fn run(mbi: &BootInformation) -> anyhow::Result<()> { Ok(()) } -pub(self) fn print_memory_map(mbi: &BootInformation) -> anyhow::Result<()> { +pub(self) fn print_memory_map>(mbi: &BootInformation) -> anyhow::Result<()> { let memmap = mbi .memory_map_tag() .ok_or("Should have memory map") @@ -46,7 +46,7 @@ pub(self) fn print_memory_map(mbi: &BootInformation) -> anyhow::Result<()> { Ok(()) } -pub(self) fn print_elf_info(mbi: &BootInformation) -> anyhow::Result<()> { +pub(self) fn print_elf_info>(mbi: &BootInformation) -> anyhow::Result<()> { let sections_iter = mbi .elf_sections() .ok_or("Should have elf sections") @@ -71,7 +71,7 @@ pub(self) fn print_elf_info(mbi: &BootInformation) -> anyhow::Result<()> { Ok(()) } -pub(self) fn print_module_info(mbi: &BootInformation) -> anyhow::Result<()> { +pub(self) fn print_module_info>(mbi: &BootInformation) -> anyhow::Result<()> { let modules = mbi.module_tags().collect::>(); if modules.len() != 1 { Err(anyhow::Error::msg("Should have exactly one boot module"))? diff --git a/multiboot2/src/lib.rs b/multiboot2/src/lib.rs index 4f8b05cf..dcdbf1bf 100644 --- a/multiboot2/src/lib.rs +++ b/multiboot2/src/lib.rs @@ -99,7 +99,7 @@ use derive_more::Display; #[cfg(feature = "builder")] use crate::builder::AsBytes; use crate::framebuffer::UnknownFramebufferType; -use tag::TagIter; +use tag::{TagIter, TagIterMut}; /// Magic number that a Multiboot2-compliant boot loader will use to identify /// the handoff. The location depends on the architecture and the targeted @@ -152,9 +152,9 @@ 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)] +#[derive(ptr_meta::Pointee, Debug)] #[repr(C)] -struct BootInformationInner { +pub struct BootInformationInner { header: BootInformationHeader, tags: [u8], } @@ -178,11 +178,22 @@ impl BootInformationInner { } } +impl AsRef for BootInformationInner { + fn as_ref(&self) -> &BootInformationInner { + self + } +} + +impl AsMut for BootInformationInner { + fn as_mut(&mut self) -> &mut BootInformationInner { + self + } +} /// A Multiboot 2 Boot Information (MBI) accessor. #[repr(transparent)] -pub struct BootInformation<'a>(&'a BootInformationInner); +pub struct BootInformation>(T); -impl<'a> BootInformation<'a> { +impl BootInformation<&BootInformationInner> { /// Loads the [`BootInformation`] from a pointer. The pointer must be valid /// and aligned to an 8-byte boundary, as defined by the spec. /// @@ -231,7 +242,42 @@ impl<'a> BootInformation<'a> { Ok(Self(mbi)) } +} + +impl BootInformation<&mut BootInformationInner> { + /// `BootInformation::load`, but mutably. + /// + /// # Safety + /// The same considerations that apply to `load` also apply here, but the + /// memory can be modified (through the `_mut` methods). + pub unsafe fn load_mut(ptr: *mut BootInformationHeader) -> Result { + // null or not aligned + if ptr.is_null() || ptr.align_offset(8) != 0 { + return Err(MbiLoadError::IllegalAddress); + } + + // mbi: reference to basic header + let mbi = &*ptr; + + // Check if total size is not 0 and a multiple of 8. + if mbi.total_size == 0 || mbi.total_size & 0b111 != 0 { + return Err(MbiLoadError::IllegalTotalSize(mbi.total_size)); + } + let slice_size = mbi.total_size as usize - size_of::(); + // mbi: reference to full mbi + let mbi = ptr_meta::from_raw_parts_mut::(ptr.cast(), slice_size); + let mbi = &mut *mbi; + + if !mbi.has_valid_end_tag() { + return Err(MbiLoadError::NoEndTag); + } + + Ok(Self(mbi)) + } +} + +impl> BootInformation { /// Get the start address of the boot info. pub fn start_address(&self) -> usize { self.as_ptr() as usize @@ -239,7 +285,7 @@ impl<'a> BootInformation<'a> { /// Get the start address of the boot info as pointer. pub fn as_ptr(&self) -> *const () { - core::ptr::addr_of!(*self.0).cast() + core::ptr::addr_of!(*self.0.as_ref()).cast() } /// Get the end address of the boot info. @@ -258,7 +304,7 @@ impl<'a> BootInformation<'a> { /// Get the total size of the boot info struct. pub fn total_size(&self) -> usize { - self.0.header.total_size as usize + self.0.as_ref().header.total_size as usize } // ###################################################### @@ -458,7 +504,7 @@ impl<'a> BootInformation<'a> { /// .unwrap(); /// assert_eq!(tag.name(), Ok("name")); /// ``` - pub fn get_tag(&'a self) -> Option<&'a TagT> { + pub fn get_tag(&self) -> Option<&TagT> { self.tags() .find(|tag| tag.typ == TagT::ID) .map(|tag| tag.cast_tag::()) @@ -466,11 +512,36 @@ impl<'a> BootInformation<'a> { /// Returns an iterator over all tags. fn tags(&self) -> TagIter { - TagIter::new(&self.0.tags) + TagIter::new(&self.0.as_ref().tags) } } -impl fmt::Debug for BootInformation<'_> { +impl + AsMut> BootInformation { + /// Search for the Memory map tag, return a mutable reference. + pub fn memory_map_tag_mut(&mut self) -> Option<&mut MemoryMapTag> { + self.get_tag_mut::(TagType::Mmap) + } + + fn get_tag_mut>( + &mut self, + typ: TagType, + ) -> Option<&mut TagT> { + let typ = typ.into(); + self.tags_mut() + .find(|tag| tag.typ == typ) + .map(|tag| tag.cast_tag_mut::()) + } + + fn tags_mut(&mut self) -> TagIterMut { + TagIterMut::new(&mut self.0.as_mut().tags) + } +} + +// SAFETY: BootInformation contains a const ptr to memory that is never mutated. +// Sending this pointer to other threads is sound. +unsafe impl> Send for BootInformation {} + +impl> fmt::Debug for BootInformation { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { /// Limit how many Elf-Sections should be debug-formatted. /// Can be thousands of sections for a Rust binary => this is useless output. @@ -1235,7 +1306,7 @@ mod tests { /// Helper for [`grub2`]. fn test_grub2_boot_info( - bi: &BootInformation, + bi: &BootInformation<&BootInformationInner>, addr: usize, string_addr: u64, bytes: &[u8], diff --git a/multiboot2/src/memory_map.rs b/multiboot2/src/memory_map.rs index a5197ec5..5cc8cb68 100644 --- a/multiboot2/src/memory_map.rs +++ b/multiboot2/src/memory_map.rs @@ -61,6 +61,13 @@ impl MemoryMapTag { assert_eq!(self.entry_size as usize, mem::size_of::()); &self.areas } + + /// Return a mutable slice with all memory areas. + pub fn all_memory_areas_mut(&mut self) -> &mut [MemoryArea] { + // If this ever fails, we need to model this differently in this crate. + assert_eq!(self.entry_size as usize, mem::size_of::()); + &mut self.areas + } } impl TagTrait for MemoryMapTag { diff --git a/multiboot2/src/tag.rs b/multiboot2/src/tag.rs index 0002d59f..d087e13e 100644 --- a/multiboot2/src/tag.rs +++ b/multiboot2/src/tag.rs @@ -64,6 +64,14 @@ impl Tag { unsafe { TagTrait::from_base_tag(self) } } + /// Casts the base tag to the specific tag type, but mutably. + pub fn cast_tag_mut<'a, T: TagTrait + ?Sized + 'a>(&'a mut self) -> &'a mut T { + assert_eq!(self.typ, T::ID); + // Safety: At this point, we trust that "self.size" and the size hint + // for DST tags are sane. + unsafe { TagTrait::from_base_tag_mut(self) } + } + /// Parses the provided byte sequence as Multiboot string, which maps to a /// [`str`]. pub fn parse_slice_as_string(bytes: &[u8]) -> Result<&str, StringError> { @@ -139,6 +147,55 @@ impl<'a> Iterator for TagIter<'a> { } } +/// Iterates the MBI's tags from the first tag to the end tag. +#[derive(Clone, Debug)] +pub struct TagIterMut<'a> { + /// Pointer to the next tag. Updated in each iteration. + pub current: *mut Tag, + /// The pointer right after the MBI. Used for additional bounds checking. + end_ptr_exclusive: *mut u8, + /// Lifetime capture of the MBI's memory. + _mem: PhantomData<&'a mut ()>, +} + +impl<'a> TagIterMut<'a> { + /// Creates a new iterator + pub fn new(mem: &'a mut [u8]) -> Self { + assert_eq!(mem.as_ptr().align_offset(8), 0); + TagIterMut { + current: mem.as_mut_ptr().cast(), + end_ptr_exclusive: unsafe { mem.as_mut_ptr().add(mem.len()) }, + _mem: PhantomData, + } + } +} + +impl<'a> Iterator for TagIterMut<'a> { + type Item = &'a mut Tag; + + fn next(&mut self) -> Option<&'a mut Tag> { + // This never failed so far. But better be safe. + assert!(self.current.cast::() < self.end_ptr_exclusive); + + let tag = unsafe { &mut *self.current }; + match tag.typ() { + TagType::End => None, // end 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 + self.current = unsafe { self.current.cast::().add(ptr_offset).cast::() }; + + Some(tag) + } + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/multiboot2/src/tag_trait.rs b/multiboot2/src/tag_trait.rs index 5c42e8a8..d7ddf501 100644 --- a/multiboot2/src/tag_trait.rs +++ b/multiboot2/src/tag_trait.rs @@ -56,4 +56,17 @@ pub trait TagTrait: Pointee { let ptr = ptr_meta::from_raw_parts(ptr.cast(), Self::dst_size(tag)); &*ptr } + + /// Creates a reference to a (dynamically sized) tag type in a safe way. + /// DST tags need to implement a proper [`Self::dst_size`] implementation. + /// + /// # Safety + /// Callers must be sure that the "size" field of the provided [`Tag`] is + /// sane and the underlying memory valid. The implementation of this trait + /// **must have** a correct [`Self::dst_size`] implementation. + unsafe fn from_base_tag_mut<'a>(tag: &mut Tag) -> &'a mut Self { + let ptr = core::ptr::addr_of_mut!(*tag); + let ptr = ptr_meta::from_raw_parts_mut(ptr.cast(), Self::dst_size(tag)); + &mut *ptr + } } From 42b30599a947e174cfa174da2db22cfc6a9df2e0 Mon Sep 17 00:00:00 2001 From: Niklas Sombert Date: Wed, 15 Mar 2023 22:14:05 +0100 Subject: [PATCH 2/4] multiboot2: Get a mutable reference to the basic memory information tag --- multiboot2/src/lib.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/multiboot2/src/lib.rs b/multiboot2/src/lib.rs index dcdbf1bf..deb5a878 100644 --- a/multiboot2/src/lib.rs +++ b/multiboot2/src/lib.rs @@ -522,6 +522,11 @@ impl + AsMut> BootInformati self.get_tag_mut::(TagType::Mmap) } + /// Search for the basic memory info tag, return a mutable reference. + pub fn basic_memory_info_tag_mut(&mut self) -> Option<&mut BasicMemoryInfoTag> { + self.get_tag_mut::(TagType::BasicMeminfo) + } + fn get_tag_mut>( &mut self, typ: TagType, From 5f3e1f99d3eaf8042e8a4730c92aadef38f18503 Mon Sep 17 00:00:00 2001 From: Niklas Sombert Date: Mon, 3 Apr 2023 21:41:37 +0200 Subject: [PATCH 3/4] multiboot2: Get a mutable reference to the EFI memory map --- multiboot2/src/lib.rs | 5 +++++ multiboot2/src/memory_map.rs | 38 ++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/multiboot2/src/lib.rs b/multiboot2/src/lib.rs index deb5a878..d5c9344d 100644 --- a/multiboot2/src/lib.rs +++ b/multiboot2/src/lib.rs @@ -527,6 +527,11 @@ impl + AsMut> BootInformati self.get_tag_mut::(TagType::BasicMeminfo) } + /// Search for the EFI Memory map tag, return a mutable reference. + pub fn efi_memory_map_tag_mut(&mut self) -> Option<&mut EFIMemoryMapTag> { + self.get_tag_mut::(TagType::EfiMmap) + } + fn get_tag_mut>( &mut self, typ: TagType, diff --git a/multiboot2/src/memory_map.rs b/multiboot2/src/memory_map.rs index 5cc8cb68..e966322c 100644 --- a/multiboot2/src/memory_map.rs +++ b/multiboot2/src/memory_map.rs @@ -330,6 +330,22 @@ impl EFIMemoryMapTag { phantom: PhantomData, } } + + /// Return an iterator over ALL marked memory areas, mutably. + /// + /// This differs from `MemoryMapTag` as for UEFI, the OS needs some non- + /// available memory areas for tables and such. + pub fn memory_areas_mut(&mut self) -> EFIMemoryAreaIterMut { + let self_ptr = self as *mut EFIMemoryMapTag; + let start_area = (&mut self.descs[0]) as *mut EFIMemoryDesc; + EFIMemoryAreaIterMut { + 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 *mut () as u64 + self.size as u64), + entry_size: self.desc_size, + phantom: PhantomData, + } + } } impl TagTrait for EFIMemoryMapTag { @@ -364,3 +380,25 @@ impl<'a> Iterator for EFIMemoryAreaIter<'a> { } } } + +/// An iterator over ALL EFI memory areas, mutably. +#[derive(Clone, Debug)] +pub struct EFIMemoryAreaIterMut<'a> { + current_area: u64, + last_area: u64, + entry_size: u32, + phantom: PhantomData<&'a mut EFIMemoryDesc>, +} + +impl<'a> Iterator for EFIMemoryAreaIterMut<'a> { + type Item = &'a mut EFIMemoryDesc; + fn next(&mut self) -> Option<&'a mut EFIMemoryDesc> { + if self.current_area > self.last_area { + None + } else { + let area = unsafe { &mut *(self.current_area as *mut EFIMemoryDesc) }; + self.current_area += self.entry_size as u64; + Some(area) + } + } +} From 1c2fb82b138b8a2706c98ba79ca16e0ab22aa19c Mon Sep 17 00:00:00 2001 From: Niklas Sombert Date: Sun, 9 Jul 2023 12:40:56 +0200 Subject: [PATCH 4/4] multiboot2: Combine EFIMemoryAreaIter(Mut) --- multiboot2/src/memory_map.rs | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/multiboot2/src/memory_map.rs b/multiboot2/src/memory_map.rs index e966322c..dd2e02c4 100644 --- a/multiboot2/src/memory_map.rs +++ b/multiboot2/src/memory_map.rs @@ -319,7 +319,7 @@ impl EFIMemoryMapTag { /// /// This differs from `MemoryMapTag` as for UEFI, the OS needs some non- /// available memory areas for tables and such. - pub fn memory_areas(&self) -> EFIMemoryAreaIter { + pub fn memory_areas(&self) -> EFIMemoryAreaIter<&EFIMemoryDesc> { let self_ptr = self as *const EFIMemoryMapTag; let start_area = (&self.descs[0]) as *const EFIMemoryDesc; EFIMemoryAreaIter { @@ -335,10 +335,10 @@ impl EFIMemoryMapTag { /// /// This differs from `MemoryMapTag` as for UEFI, the OS needs some non- /// available memory areas for tables and such. - pub fn memory_areas_mut(&mut self) -> EFIMemoryAreaIterMut { + pub fn memory_areas_mut(&mut self) -> EFIMemoryAreaIter<&mut EFIMemoryDesc> { let self_ptr = self as *mut EFIMemoryMapTag; let start_area = (&mut self.descs[0]) as *mut EFIMemoryDesc; - EFIMemoryAreaIterMut { + EFIMemoryAreaIter { 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 *mut () as u64 + self.size as u64), @@ -361,14 +361,14 @@ impl TagTrait for EFIMemoryMapTag { /// An iterator over ALL EFI memory areas. #[derive(Clone, Debug)] -pub struct EFIMemoryAreaIter<'a> { +pub struct EFIMemoryAreaIter { current_area: u64, last_area: u64, entry_size: u32, - phantom: PhantomData<&'a EFIMemoryDesc>, + phantom: PhantomData, } -impl<'a> Iterator for EFIMemoryAreaIter<'a> { +impl<'a> Iterator for EFIMemoryAreaIter<&'a EFIMemoryDesc> { type Item = &'a EFIMemoryDesc; fn next(&mut self) -> Option<&'a EFIMemoryDesc> { if self.current_area > self.last_area { @@ -381,16 +381,7 @@ impl<'a> Iterator for EFIMemoryAreaIter<'a> { } } -/// An iterator over ALL EFI memory areas, mutably. -#[derive(Clone, Debug)] -pub struct EFIMemoryAreaIterMut<'a> { - current_area: u64, - last_area: u64, - entry_size: u32, - phantom: PhantomData<&'a mut EFIMemoryDesc>, -} - -impl<'a> Iterator for EFIMemoryAreaIterMut<'a> { +impl<'a> Iterator for EFIMemoryAreaIter<&'a mut EFIMemoryDesc> { type Item = &'a mut EFIMemoryDesc; fn next(&mut self) -> Option<&'a mut EFIMemoryDesc> { if self.current_area > self.last_area {