From 74c981b4c37fae6210220262257d1a2e3121a6d1 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 17 Aug 2024 08:29:00 +0200 Subject: [PATCH 1/2] treewide: allow std in tests This allows to use `dbg!` for example. Having this is beneficial in all no_std crates. There are no disadvantages or negative implications. --- uefi-raw/src/lib.rs | 5 +++++ uefi/src/lib.rs | 7 ++++++- xtask/src/check_raw.rs | 8 +++++++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/uefi-raw/src/lib.rs b/uefi-raw/src/lib.rs index 78d320bfc..faf6155b1 100644 --- a/uefi-raw/src/lib.rs +++ b/uefi-raw/src/lib.rs @@ -20,6 +20,11 @@ unused )] +// Useful for convenience in tests, such as `dbg!` +#[cfg_attr(test, macro_use)] +#[cfg(test)] +extern crate std; + #[macro_use] mod enums; diff --git a/uefi/src/lib.rs b/uefi/src/lib.rs index 7a7ce8a65..1ecb938b3 100644 --- a/uefi/src/lib.rs +++ b/uefi/src/lib.rs @@ -24,7 +24,7 @@ //! //! Status::SUCCESS //! } -//! # extern crate std; +//! # extern crate std; // For panic handler //! ``` //! //! Please find more info in our [Rust UEFI Book]. @@ -235,6 +235,11 @@ extern crate self as uefi; #[macro_use] extern crate uefi_raw; +// Useful for convenience in tests, such as `dbg!` +#[cfg_attr(test, macro_use)] +#[cfg(test)] +extern crate std; + #[macro_use] pub mod data_types; pub mod allocator; diff --git a/xtask/src/check_raw.rs b/xtask/src/check_raw.rs index b6887afc6..c466d7250 100644 --- a/xtask/src/check_raw.rs +++ b/xtask/src/check_raw.rs @@ -354,7 +354,13 @@ fn check_item(item: &Item, src: &Path) -> Result<(), Error> { // Allow. } item => { - return Err(Error::new(ErrorKind::ForbiddenItemKind, src, item)); + let allowed_exception = match item { + Item::ExternCrate(x) => x.ident == "std", + _ => false, + }; + if !allowed_exception { + return Err(Error::new(ErrorKind::ForbiddenItemKind, src, item)); + } } } From ee2e9a176c2f2ac8de3bd9b4486995ee3a08cf79 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 17 Aug 2024 08:30:27 +0200 Subject: [PATCH 2/2] uefi-raw: Add explicit padding field for MemoryDescriptor + add test By convention, structs having a C representation should be explicit with their in-between padding fields. This allows users memory-safe trivial serialization and deserialization by accessing the underlying memory as byte slice. Without this fields being explicit, Miri complains about uninitialized memory accesses, which is UB. uefi-raw: add test for MemoryDescriptor to catch Miri problems The underlying issue is that if someone wants to cast a slice of descriptors to a byte slice, the uninitialized **implicit** padding bytes are UB. Miri complains about that. --- uefi-raw/CHANGELOG.md | 3 +++ uefi-raw/src/table/boot.rs | 38 ++++++++++++++++++++++++++++++-- uefi/src/mem/memory_map/impl_.rs | 3 +++ uefi/src/mem/memory_map/mod.rs | 12 ++++++++++ uefi/tests/memory_map.rs | 3 +++ 5 files changed, 57 insertions(+), 2 deletions(-) diff --git a/uefi-raw/CHANGELOG.md b/uefi-raw/CHANGELOG.md index 9bf48d15b..8ecae3391 100644 --- a/uefi-raw/CHANGELOG.md +++ b/uefi-raw/CHANGELOG.md @@ -1,5 +1,8 @@ # uefi-raw - [Unreleased] +## Changed +- `MemoryDescriptor` now has an explicit padding field, allowing trivial + memory-safe serialization and deserialization # uefi-raw - 0.8.0 (2024-09-09) diff --git a/uefi-raw/src/table/boot.rs b/uefi-raw/src/table/boot.rs index 8559a4c28..82cfc5de0 100644 --- a/uefi-raw/src/table/boot.rs +++ b/uefi-raw/src/table/boot.rs @@ -350,7 +350,8 @@ bitflags! { pub struct MemoryDescriptor { /// Type of memory occupying this range. pub ty: MemoryType, - // Implicit 32-bit padding. + /// Padding field. Ignore. + pub padding0: u32, /// Starting physical address. pub phys_start: PhysicalAddress, /// Starting virtual address. @@ -370,6 +371,7 @@ impl Default for MemoryDescriptor { fn default() -> Self { Self { ty: MemoryType::RESERVED, + padding0: 0, phys_start: 0, virt_start: 0, page_count: 0, @@ -379,7 +381,7 @@ impl Default for MemoryDescriptor { } newtype_enum! { -/// The type of a memory range. +/// The type of memory range. /// /// UEFI allows firmwares and operating systems to introduce new memory types /// in the `0x7000_0000..=0xFFFF_FFFF` range. Therefore, we don't know the full set @@ -484,3 +486,35 @@ pub enum Tpl: usize => { /// Note that this is not necessarily the processor's page size. The UEFI page /// size is always 4 KiB. pub const PAGE_SIZE: usize = 4096; + +#[cfg(test)] +mod tests { + use core::{mem, slice}; + use super::*; + + // This tests succeeds if Miri doesn't complain about uninitialized memory. + #[test] + fn memory_descriptor_trivial_serialization() { + let descs = [ + MemoryDescriptor { + ty: MemoryType::CONVENTIONAL, + padding0: 0, + phys_start: 0x1000, + virt_start: 0x1000, + page_count: 1, + att: Default::default(), + }, + ]; + + + let raw_bytes: &[u8] = { + let ptr = descs.as_ptr().cast::(); + let len = mem::size_of_val(&descs); + unsafe { slice::from_raw_parts(ptr.cast(), len) } + }; + + // Test succeeds if Miri doesn't complain about initialized memory + // (implicit padding fields). + dbg!(&raw_bytes); + } +} diff --git a/uefi/src/mem/memory_map/impl_.rs b/uefi/src/mem/memory_map/impl_.rs index 21814cae5..25761678c 100644 --- a/uefi/src/mem/memory_map/impl_.rs +++ b/uefi/src/mem/memory_map/impl_.rs @@ -432,6 +432,7 @@ mod tests { const BASE_MMAP_UNSORTED: [MemoryDescriptor; 3] = [ MemoryDescriptor { ty: MemoryType::CONVENTIONAL, + padding0: 0, phys_start: 0x3000, virt_start: 0x3000, page_count: 1, @@ -439,6 +440,7 @@ mod tests { }, MemoryDescriptor { ty: MemoryType::CONVENTIONAL, + padding0: 0, phys_start: 0x2000, virt_start: 0x2000, page_count: 1, @@ -446,6 +448,7 @@ mod tests { }, MemoryDescriptor { ty: MemoryType::CONVENTIONAL, + padding0: 0, phys_start: 0x1000, virt_start: 0x1000, page_count: 1, diff --git a/uefi/src/mem/memory_map/mod.rs b/uefi/src/mem/memory_map/mod.rs index 46cd82903..799b82dec 100644 --- a/uefi/src/mem/memory_map/mod.rs +++ b/uefi/src/mem/memory_map/mod.rs @@ -131,6 +131,7 @@ mod tests_mmap_artificial { const BASE: MemoryDescriptor = MemoryDescriptor { ty: TY, + padding0: 0, phys_start: 0, virt_start: 0, page_count: 0, @@ -169,6 +170,7 @@ mod tests_mmap_artificial { const BASE: MemoryDescriptor = MemoryDescriptor { ty: TY, + padding0: 0, phys_start: 0, virt_start: 0, page_count: 0, @@ -278,6 +280,7 @@ mod tests_mmap_real { | MemoryAttribute::WRITE_COMBINE | MemoryAttribute::WRITE_THROUGH | MemoryAttribute::WRITE_BACK, + ..Default::default() }, MemoryDescriptor { ty: MemoryType::CONVENTIONAL, @@ -288,6 +291,7 @@ mod tests_mmap_real { | MemoryAttribute::WRITE_COMBINE | MemoryAttribute::WRITE_THROUGH | MemoryAttribute::WRITE_BACK, + ..Default::default() }, MemoryDescriptor { ty: MemoryType::BOOT_SERVICES_DATA, @@ -298,6 +302,7 @@ mod tests_mmap_real { | MemoryAttribute::WRITE_COMBINE | MemoryAttribute::WRITE_THROUGH | MemoryAttribute::WRITE_BACK, + ..Default::default() }, MemoryDescriptor { ty: MemoryType::CONVENTIONAL, @@ -308,6 +313,7 @@ mod tests_mmap_real { | MemoryAttribute::WRITE_COMBINE | MemoryAttribute::WRITE_THROUGH | MemoryAttribute::WRITE_BACK, + ..Default::default() }, MemoryDescriptor { ty: MemoryType::CONVENTIONAL, @@ -318,6 +324,7 @@ mod tests_mmap_real { | MemoryAttribute::WRITE_COMBINE | MemoryAttribute::WRITE_THROUGH | MemoryAttribute::WRITE_BACK, + ..Default::default() }, MemoryDescriptor { ty: MemoryType::ACPI_NON_VOLATILE, @@ -328,6 +335,7 @@ mod tests_mmap_real { | MemoryAttribute::WRITE_COMBINE | MemoryAttribute::WRITE_THROUGH | MemoryAttribute::WRITE_BACK, + ..Default::default() }, MemoryDescriptor { ty: MemoryType::CONVENTIONAL, @@ -338,6 +346,7 @@ mod tests_mmap_real { | MemoryAttribute::WRITE_COMBINE | MemoryAttribute::WRITE_THROUGH | MemoryAttribute::WRITE_BACK, + ..Default::default() }, MemoryDescriptor { ty: MemoryType::ACPI_NON_VOLATILE, @@ -348,6 +357,7 @@ mod tests_mmap_real { | MemoryAttribute::WRITE_COMBINE | MemoryAttribute::WRITE_THROUGH | MemoryAttribute::WRITE_BACK, + ..Default::default() }, MemoryDescriptor { ty: MemoryType::CONVENTIONAL, @@ -358,6 +368,7 @@ mod tests_mmap_real { | MemoryAttribute::WRITE_COMBINE | MemoryAttribute::WRITE_THROUGH | MemoryAttribute::WRITE_BACK, + ..Default::default() }, MemoryDescriptor { ty: MemoryType::ACPI_NON_VOLATILE, @@ -368,6 +379,7 @@ mod tests_mmap_real { | MemoryAttribute::WRITE_COMBINE | MemoryAttribute::WRITE_THROUGH | MemoryAttribute::WRITE_BACK, + ..Default::default() }, ]; assert_eq!(entries.as_slice(), &expected); diff --git a/uefi/tests/memory_map.rs b/uefi/tests/memory_map.rs index 0d7be6191..c41cdd83f 100644 --- a/uefi/tests/memory_map.rs +++ b/uefi/tests/memory_map.rs @@ -12,6 +12,7 @@ fn parse_boot_information_efi_mmap() { virt_start: 0x3000, page_count: 1, att: MemoryAttribute::WRITE_BACK, + ..Default::default() }, MemoryDescriptor { ty: MemoryType::CONVENTIONAL, @@ -19,6 +20,7 @@ fn parse_boot_information_efi_mmap() { virt_start: 0x2000, page_count: 1, att: MemoryAttribute::WRITE_BACK, + ..Default::default() }, MemoryDescriptor { ty: MemoryType::CONVENTIONAL, @@ -26,6 +28,7 @@ fn parse_boot_information_efi_mmap() { virt_start: 0x1000, page_count: 1, att: MemoryAttribute::WRITE_BACK, + ..Default::default() }, ]; let map_size = mmap_source.len() * desc_size;