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.
rust-osdev · Oct 6, 2024 · 451716a · 451716a
1 parent a5b35f7
commit 451716a
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 2 deletions.
diff --git 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
@@ -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::<u8>();
+            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
@@ -432,20 +432,23 @@ mod tests {
     const BASE_MMAP_UNSORTED: [MemoryDescriptor; 3] = [
         MemoryDescriptor {
             ty: MemoryType::CONVENTIONAL,
+            padding0: 0,
             phys_start: 0x3000,
             virt_start: 0x3000,
             page_count: 1,
             att: MemoryAttribute::WRITE_BACK,
         },
         MemoryDescriptor {
             ty: MemoryType::CONVENTIONAL,
+            padding0: 0,
             phys_start: 0x2000,
             virt_start: 0x2000,
             page_count: 1,
             att: MemoryAttribute::WRITE_BACK,
         },
         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
@@ -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
@@ -12,20 +12,23 @@ fn parse_boot_information_efi_mmap() {
             virt_start: 0x3000,
             page_count: 1,
             att: MemoryAttribute::WRITE_BACK,
+            ..Default::default()
         },
         MemoryDescriptor {
             ty: MemoryType::CONVENTIONAL,
             phys_start: 0x2000,
             virt_start: 0x2000,
             page_count: 1,
             att: MemoryAttribute::WRITE_BACK,
+            ..Default::default()
         },
         MemoryDescriptor {
             ty: MemoryType::CONVENTIONAL,
             phys_start: 0x1000,
             virt_start: 0x1000,
             page_count: 1,
             att: MemoryAttribute::WRITE_BACK,
+            ..Default::default()
         },
     ];
     let map_size = mmap_source.len() * desc_size;