diff --git a/multiboot2-common/CHANGELOG.md b/multiboot2-common/CHANGELOG.md index d7ead3a3..879c9539 100644 --- a/multiboot2-common/CHANGELOG.md +++ b/multiboot2-common/CHANGELOG.md @@ -2,6 +2,14 @@ ## Unreleased +- **Breaking:** Fixed undefined behavior when serializing stack-constructed + structures with implicit trailing padding: `MaybeDynSized::as_bytes` now + returns a plain `&[u8]` that covers exactly the structure size reported in + the header (clamped to the allocation) instead of a `BytesRef` over the + whole allocation, whose trailing padding is uninitialized memory for + stack-constructed values. `payload()` follows suit. As a side effect, + `clone_dyn` now preserves the reported size exactly instead of growing it + to the padded allocation size. - Added the `raw_type!` macro that generates an ABI-safe `#[repr(transparent)]` newtype plus a corresponding high-level open-set enum, including all conversions between them and the underlying integer. diff --git a/multiboot2-common/src/boxed.rs b/multiboot2-common/src/boxed.rs index 0b925467..4cdbccc6 100644 --- a/multiboot2-common/src/boxed.rs +++ b/multiboot2-common/src/boxed.rs @@ -109,6 +109,7 @@ mod tests { use super::*; use crate::Tag; use crate::test_utils::{DummyDstTag, DummyTestHeader}; + use core::slice; #[test] fn test_new_boxed() { @@ -126,23 +127,35 @@ mod tests { #[test] fn test_new_boxed_zeroes_padding() { - // A payload of 1 byte yields a 9-byte tag, padded to 16 bytes. The - // 7 trailing padding bytes must be initialized (to zero); otherwise - // reading them via `as_bytes()` is UB (caught by Miri). + // A payload of 1 byte yields a 9-byte tag in a 16-byte allocation. + // `as_bytes()` must exclude the 7 trailing padding bytes, while the + // allocation itself must be zeroed there (guaranteed by + // `alloc_zeroed`), as the Multiboot2 spec mandates zeroed padding + // between tags. let header = DummyTestHeader::new(DummyDstTag::ID, 0); let tag = new_boxed::(header, &[&[0xff]]); - let bytes = tag.as_bytes(); - assert_eq!(bytes.len(), 16); - assert_eq!(&bytes[9..16], &[0, 0, 0, 0, 0, 0, 0]); + assert_eq!(tag.as_bytes().len(), 9); + let ptr = (&raw const *tag).cast::(); + // SAFETY: The allocation spans `size_of_val` bytes and is fully + // initialized by `new_boxed` (zeroed allocation). + let all_bytes = unsafe { slice::from_raw_parts(ptr, size_of_val(&*tag)) }; + assert_eq!(all_bytes.len(), 16); + assert_eq!(&all_bytes[9..16], &[0, 0, 0, 0, 0, 0, 0]); } #[test] fn test_clone_tag() { + // A 5-byte payload, so that the reported tag size (13) is no + // multiple of the alignment. let header = DummyTestHeader::new(DummyDstTag::ID, 0); - let tag = new_boxed::(header, &[&[0, 1, 2, 3]]); + let tag = new_boxed::(header, &[&[0, 1, 2, 3, 4]]); assert_eq!(tag.header().typ(), 42); - assert_eq!(tag.payload(), &[0, 1, 2, 3]); + assert_eq!(tag.payload(), &[0, 1, 2, 3, 4]); - let _cloned = clone_dyn(tag.as_ref()); + let cloned = clone_dyn(tag.as_ref()); + // The clone must round-trip exactly; especially, the reported size + // must not grow to the padded allocation size. + assert_eq!(cloned.header(), tag.header()); + assert_eq!(cloned.payload(), tag.payload()); } } diff --git a/multiboot2-common/src/tag.rs b/multiboot2-common/src/tag.rs index 459d333a..6ffadd5b 100644 --- a/multiboot2-common/src/tag.rs +++ b/multiboot2-common/src/tag.rs @@ -1,6 +1,6 @@ //! Module for the traits [`MaybeDynSized`] and [`Tag`]. -use crate::{BytesRef, DynSizedStructure, Header}; +use crate::{DynSizedStructure, Header}; use core::slice; use ptr_meta::Pointee; @@ -69,25 +69,31 @@ pub unsafe trait MaybeDynSized: Pointee { } /// Returns the payload, i.e., all memory that is not occupied by the - /// [`Header`] of the type. + /// [`Header`] of the type. Implicit trailing padding beyond the + /// structure size reported in the header is not part of the payload. + /// + /// # Panics + /// Panics if the size reported in the header is smaller than the size of + /// the [`Header`] itself, which can only happen for oddly formed values. fn payload(&self) -> &[u8] { let from = size_of::(); &self.as_bytes()[from..] } - /// Returns the whole allocated bytes for this structure encapsulated in - /// [`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> { + /// Returns the bytes of the structure, i.e., the header and the payload, + /// up to the structure size reported by [`Self::header`]. + /// + /// Implicit trailing padding that the Rust memory layout might add beyond + /// that size is excluded, as it may be uninitialized for stack-constructed + /// values and must never be read. + fn as_bytes(&self) -> &[u8] { 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::(), size) }; - // Unwrap is fine as this type can't exist without the underlying memory - // guarantees. - BytesRef::try_from(slice).unwrap() + // Clamp to the allocation: a corrupt header size must never cause an + // out-of-bounds slice. + let size = self.header().total_size().min(size_of_val(self)); + // SAFETY: `ptr` points to `self`'s allocation, `size` is in bounds, + // and the first `total_size()` bytes of a value are initialized. + unsafe { slice::from_raw_parts(ptr.cast::(), size) } } /// Returns a pointer to this structure. diff --git a/multiboot2-header/CHANGELOG.md b/multiboot2-header/CHANGELOG.md index bde97dd7..e31cdcd7 100644 --- a/multiboot2-header/CHANGELOG.md +++ b/multiboot2-header/CHANGELOG.md @@ -2,6 +2,14 @@ ## Unreleased +- Fixed undefined behavior when serializing stack-constructed sized tags with + trailing struct padding (`ConsoleHeaderTag`, `EntryAddressHeaderTag`, + `EntryEfi32HeaderTag`, `EntryEfi64HeaderTag`, `FramebufferHeaderTag`): the + padding is uninitialized memory that `as_bytes()` - and thus + `Builder::build()` - exposed. `as_bytes()` now covers exactly the reported + tag size, and `Builder::build()` writes explicit zeroed padding between + tags. The built header is byte-wise identical, except that its inter-tag + padding is now guaranteed to be zeroed. - **Breaking:** `InformationRequestHeaderTag::new()` now takes `&[MbiTagType]` and `requests()` returns an iterator over `MbiTagType`. The `MbiTagTypeId` re-export was renamed to `MbiTagTypeRaw`. diff --git a/multiboot2-header/src/builder.rs b/multiboot2-header/src/builder.rs index 183f2d35..b1ac7647 100644 --- a/multiboot2-header/src/builder.rs +++ b/multiboot2-header/src/builder.rs @@ -8,7 +8,7 @@ use crate::{ }; use alloc::boxed::Box; use alloc::vec::Vec; -use multiboot2_common::{DynSizedStructure, MaybeDynSized, new_boxed}; +use multiboot2_common::{DynSizedStructure, MaybeDynSized, increase_to_alignment, new_boxed}; /// Builder for a Multiboot2 header. #[derive(Debug)] @@ -123,42 +123,62 @@ impl Builder { /// Multiboot2 header structure. #[must_use] pub fn build(self) -> Box> { + fn fill_zeroes_to_alignment(vec: &mut Vec) { + let pad = increase_to_alignment(vec.len()) - vec.len(); + for _ in 0..pad { + vec.push(0); + } + } + let header = Multiboot2BasicHeader::new(self.arch, 0); - let mut byte_refs = Vec::new(); + // Vec to be filled with the serialized tags, each zero-padded to the + // alignment. + let mut buffer_tags = Vec::::new(); if let Some(tag) = self.information_request_tag.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.address_tag.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.entry_tag.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.console_tag.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.framebuffer_tag.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.module_align_tag.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.efi_bs_tag.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.efi_32_tag.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.efi_64_tag.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.relocatable_tag.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } // TODO add support for custom tags once someone requests it. let end_tag = EndHeaderTag::new(); - byte_refs.push(end_tag.as_bytes().as_ref()); - new_boxed(header, byte_refs.as_slice()) + + buffer_tags.extend_from_slice(end_tag.as_bytes()); + new_boxed(header, &[buffer_tags.as_slice()]) } } @@ -216,10 +236,17 @@ mod tests { let structure = builder.build(); + #[cfg(miri)] + let _all_initialized = structure + .as_bytes() + .iter() + .map(|&byte| byte as u64) + .sum::(); + let header = { // SAFETY: The builder emits a fully formed, aligned header // buffer with a valid end tag. - unsafe { Header::load(structure.as_bytes().as_ref().as_ptr().cast()) }.unwrap() + unsafe { Header::load(structure.as_bytes().as_ptr().cast()) }.unwrap() }; assert_eq!(header.verify_checksum(), Ok(())); diff --git a/multiboot2/CHANGELOG.md b/multiboot2/CHANGELOG.md index 2c195afb..2f2d1bc0 100644 --- a/multiboot2/CHANGELOG.md +++ b/multiboot2/CHANGELOG.md @@ -2,6 +2,14 @@ ## Unreleased +- Fixed undefined behavior when serializing stack-constructed sized tags with + trailing struct padding (`BootdevTag`, `ApmTag`, `RsdpV1Tag`, `RsdpV2Tag`, + `EFISdt32Tag`, `EFIImageHandle32Tag`, `ImageLoadPhysAddrTag`): the padding + is uninitialized memory that `as_bytes()` - and thus `Builder::build()` - + exposed. `as_bytes()` now covers exactly the reported tag size, and + `Builder::build()` writes explicit zeroed padding between tags. The built + boot information is byte-wise identical, except that its inter-tag padding + is now guaranteed to be zeroed. - **Breaking:** Renamed `TagTypeId` to `TagTypeRaw`; it is now generated by the `raw_type!` macro from `multiboot2-common` and gained more conversions. `TagHeader::new()` now takes `impl Into`. diff --git a/multiboot2/src/apm.rs b/multiboot2/src/apm.rs index 8edebf0f..f9feab18 100644 --- a/multiboot2/src/apm.rs +++ b/multiboot2/src/apm.rs @@ -136,4 +136,20 @@ mod tests { let tag = ApmTag::new(1, 2, 3, 4, 5, 6, 7, 8, 9); assert_eq!(tag.header.size, 28); } + + /// Representative test for all stack-constructed sized tags: the bytes + /// exposed for a tag built via its constructor must cover exactly the + /// reported tag size and exclude the implicit trailing padding of the + /// Rust type layout, which is uninitialized memory that must never be + /// read (verified by Miri). + #[test] + fn as_bytes_covers_exactly_the_reported_tag_size() { + let tag = ApmTag::new(1, 2, 3, 4, 5, 6, 7, 8, 9); + let bytes = tag.as_bytes(); + assert_eq!(bytes.len(), 28); + assert!(bytes.len() < size_of::()); + // Reading every single byte must be defined behavior. + let byte_sum = bytes.iter().map(|&byte| byte as u64).sum::(); + assert!(byte_sum > 0); + } } diff --git a/multiboot2/src/boot_loader_name.rs b/multiboot2/src/boot_loader_name.rs index ccd90dff..e1cf898f 100644 --- a/multiboot2/src/boot_loader_name.rs +++ b/multiboot2/src/boot_loader_name.rs @@ -126,15 +126,13 @@ mod tests { #[cfg(feature = "builder")] fn test_build_str() { let tag = BootLoaderNameTag::new("hello"); - let bytes = tag.as_bytes().as_ref(); - let bytes = &bytes[..tag.header.size as usize]; + let bytes = tag.as_bytes(); assert_eq!(bytes, &get_bytes()[..tag.header.size as usize]); assert_eq!(tag.name(), Ok("hello")); // With terminating null. let tag = BootLoaderNameTag::new("hello\0"); - let bytes = tag.as_bytes().as_ref(); - let bytes = &bytes[..tag.header.size as usize]; + let bytes = tag.as_bytes(); assert_eq!(bytes, &get_bytes()[..tag.header.size as usize]); assert_eq!(tag.name(), Ok("hello")); diff --git a/multiboot2/src/builder.rs b/multiboot2/src/builder.rs index 41525b78..9e406786 100644 --- a/multiboot2/src/builder.rs +++ b/multiboot2/src/builder.rs @@ -11,7 +11,7 @@ use crate::{ }; use alloc::boxed::Box; use alloc::vec::Vec; -use multiboot2_common::{DynSizedStructure, MaybeDynSized, new_boxed}; +use multiboot2_common::{DynSizedStructure, MaybeDynSized, increase_to_alignment, new_boxed}; /// Builder for Multiboot2 boot information (MBI). #[derive(Debug)] @@ -251,77 +251,110 @@ impl Builder { /// Multiboot2 boot information structure. #[must_use] pub fn build(self) -> Box> { + fn fill_zeroes_to_alignment(vec: &mut Vec) { + let pad = increase_to_alignment(vec.len()) - vec.len(); + for _ in 0..pad { + vec.push(0); + } + } + let header = BootInformationHeader::new(0); - let mut byte_refs = Vec::new(); + // Vec to be filled with the serialized tags, each zero-padded to the + // alignment. + let mut buffer_tags = Vec::::new(); if let Some(tag) = self.cmdline.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.bootloader.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } - for i in &self.modules { - byte_refs.push(i.as_bytes().as_ref()); + for tag in &self.modules { + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.meminfo.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.bootdev.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.mmap.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.vbe.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.framebuffer.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.elf_sections.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.apm.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.efi32.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.efi64.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } - for i in &self.smbios { - byte_refs.push(i.as_bytes().as_ref()); + for tag in &self.smbios { + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.rsdpv1.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.rsdpv2.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } for tag in &self.network { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.efi_mmap.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.efi_bs.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.efi32_ih.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.efi64_ih.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } if let Some(tag) = self.image_load_addr.as_ref() { - byte_refs.push(tag.as_bytes().as_ref()); + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } - for i in &self.custom_tags { - byte_refs.push(i.as_bytes().as_ref()); + for tag in &self.custom_tags { + buffer_tags.extend_from_slice(tag.as_bytes()); + fill_zeroes_to_alignment(&mut buffer_tags); } let end_tag = EndTag::default(); - byte_refs.push(end_tag.as_bytes().as_ref()); - new_boxed(header, byte_refs.as_slice()) + + buffer_tags.extend_from_slice(end_tag.as_bytes()); + + new_boxed(header, &[buffer_tags.as_slice()]) } } @@ -388,6 +421,13 @@ mod tests { let structure = builder.build(); + #[cfg(miri)] + let _all_initialized = structure + .as_bytes() + .iter() + .map(|&byte| byte as u64) + .sum::(); + // SAFETY: The builder constructs a complete, aligned MBI with // a valid end tag. let info = unsafe { BootInformation::load(structure.as_bytes().as_ptr().cast()) }.unwrap(); diff --git a/multiboot2/src/command_line.rs b/multiboot2/src/command_line.rs index 724e4482..6ecebd07 100644 --- a/multiboot2/src/command_line.rs +++ b/multiboot2/src/command_line.rs @@ -120,15 +120,13 @@ mod tests { #[cfg(feature = "builder")] fn test_build_str() { let tag = CommandLineTag::new("hello"); - let bytes = tag.as_bytes().as_ref(); - let bytes = &bytes[..tag.header.size as usize]; + let bytes = tag.as_bytes(); assert_eq!(bytes, &get_bytes()[..tag.header().size as usize]); assert_eq!(tag.cmdline(), Ok("hello")); // With terminating null. let tag = CommandLineTag::new("hello\0"); - let bytes = tag.as_bytes().as_ref(); - let bytes = &bytes[..tag.header.size as usize]; + let bytes = tag.as_bytes(); assert_eq!(bytes, &get_bytes()[..tag.header().size as usize]); assert_eq!(tag.cmdline(), Ok("hello")); diff --git a/multiboot2/src/module.rs b/multiboot2/src/module.rs index dca1061f..3c2aa975 100644 --- a/multiboot2/src/module.rs +++ b/multiboot2/src/module.rs @@ -170,15 +170,13 @@ mod tests { #[cfg(feature = "builder")] fn test_build_str() { let tag = ModuleTag::new(0xff00, 0xffff, "hello"); - let bytes = tag.as_bytes().as_ref(); - let bytes = &bytes[..tag.header.size as usize]; + let bytes = tag.as_bytes(); assert_eq!(bytes, &get_bytes()[..tag.header().size as usize]); assert_eq!(tag.cmdline(), Ok("hello")); // With terminating null. let tag = ModuleTag::new(0xff00, 0xffff, "hello\0"); - let bytes = tag.as_bytes().as_ref(); - let bytes = &bytes[..tag.header.size as usize]; + let bytes = tag.as_bytes(); assert_eq!(bytes, &get_bytes()[..tag.header().size as usize]); assert_eq!(tag.cmdline(), Ok("hello")); diff --git a/multiboot2/src/smbios.rs b/multiboot2/src/smbios.rs index fb73dbc9..d0786512 100644 --- a/multiboot2/src/smbios.rs +++ b/multiboot2/src/smbios.rs @@ -119,8 +119,7 @@ mod tests { #[cfg(feature = "builder")] fn test_build() { let tag = SmbiosTag::new(7, 42, &[0, 1, 2, 3, 4, 5, 6, 7, 8]); - let bytes = tag.as_bytes().as_ref(); - let bytes = &bytes[..tag.header.size as usize]; + let bytes = tag.as_bytes(); assert_eq!(bytes, &get_bytes()[..tag.header.size as usize]); } }