Fix unsoundness in our representation of the MADT

acpi/src/handler.rs

@@ -1,4 +1,4 @@
-use core::{fmt, ops::Deref, ptr::NonNull};
+use core::{fmt, ops::Deref, pin::Pin, ptr::NonNull};
 
 /// Describes a physical mapping created by `AcpiHandler::map_physical_region` and unmapped by
 /// `AcpiHandler::unmap_physical_region`. The region mapped must be at least `size_of::<T>()`
@@ -65,6 +65,10 @@ where
         self.virtual_start
     }
 
+    pub fn get(&self) -> Pin<&T> {
+        unsafe { Pin::new_unchecked(self.virtual_start.as_ref()) }
+    }
+
     pub fn region_length(&self) -> usize {
         self.region_length
     }
@@ -82,6 +86,7 @@ unsafe impl<H: AcpiHandler + Send, T: Send> Send for PhysicalMapping<H, T> {}
 
 impl<H, T> Deref for PhysicalMapping<H, T>
 where
+    T: Unpin,
     H: AcpiHandler,
 {
     type Target = T;

acpi/src/madt.rs

@@ -3,7 +3,11 @@ use crate::{
     AcpiTable,
 };
 use bit_field::BitField;
-use core::{marker::PhantomData, mem};
+use core::{
+    marker::{PhantomData, PhantomPinned},
+    mem,
+    pin::Pin,
+};
 
 #[cfg(feature = "allocator_api")]
 use crate::{
@@ -27,16 +31,22 @@ pub enum MadtError {
 /// to read each entry from it.
 ///
 /// In modern versions of ACPI, the MADT can detail one of four interrupt models:
-///     * The ancient dual-i8259 legacy PIC model
-///     * The Advanced Programmable Interrupt Controller (APIC) model
-///     * The Streamlined Advanced Programmable Interrupt Controller (SAPIC) model (for Itanium systems)
-///     * The Generic Interrupt Controller (GIC) model (for ARM systems)
-#[repr(C, packed)]
-#[derive(Debug, Clone, Copy)]
+/// - The ancient dual-i8259 legacy PIC model
+/// - The Advanced Programmable Interrupt Controller (APIC) model
+/// - The Streamlined Advanced Programmable Interrupt Controller (SAPIC) model (for Itanium systems)
+/// - The Generic Interrupt Controller (GIC) model (for ARM systems)
+///
+/// The MADT is a variable-sized structure consisting of a static header and then a variable number of entries.
+/// This type only contains the static portion, and then uses pointer arithmetic to parse the following entries.
+/// To make this sound, this type is `!Unpin` - this prevents you from getting anything other than a `Pin<&Madt>`
+/// out of a `PhysicalMapping`, thereby preventing a `Madt` from being moved before [`Madt::entries`] is called.
+#[repr(C)]
+#[derive(Debug)]
 pub struct Madt {
     pub header: SdtHeader,
     pub local_apic_address: u32,
     pub flags: u32,
+    _pinned: PhantomPinned,
 }
 
 /// ### Safety: Implementation properly represents a valid MADT.
@@ -51,7 +61,7 @@ unsafe impl AcpiTable for Madt {
 impl Madt {
     #[cfg(feature = "allocator_api")]
     pub fn parse_interrupt_model_in<'a, A>(
-        &self,
+        self: Pin<&Self>,
         allocator: A,
     ) -> AcpiResult<(InterruptModel<'a, A>, Option<ProcessorInfo<'a, A>>)>
     where
@@ -96,7 +106,7 @@ impl Madt {
 
     #[cfg(feature = "allocator_api")]
     fn parse_apic_model_in<'a, A>(
-        &self,
+        self: Pin<&Self>,
         allocator: A,
     ) -> AcpiResult<(InterruptModel<'a, A>, Option<ProcessorInfo<'a, A>>)>
     where
@@ -301,9 +311,10 @@ impl Madt {
         ))
     }
 
-    pub fn entries(&self) -> MadtEntryIter {
+    pub fn entries(self: Pin<&Self>) -> MadtEntryIter<'_> {
+        let ptr = unsafe { Pin::into_inner_unchecked(self) as *const Madt as *const u8 };
         MadtEntryIter {
-            pointer: unsafe { (self as *const Madt as *const u8).add(mem::size_of::<Madt>()) },
+            pointer: unsafe { ptr.add(mem::size_of::<Madt>()) },
             remaining_length: self.header.length - mem::size_of::<Madt>() as u32,
             _phantom: PhantomData,
         }

acpi/src/platform/mod.rs

@@ -124,7 +124,7 @@ where
 
         let madt = tables.find_table::<Madt>();
         let (interrupt_model, processor_info) = match madt {
-            Ok(madt) => madt.parse_interrupt_model_in(allocator)?,
+            Ok(madt) => madt.get().parse_interrupt_model_in(allocator)?,
             Err(_) => (InterruptModel::Unknown, None),
         };
         let pm_timer = PmTimer::new(&fadt)?;

acpi/src/sdt.rs

@@ -197,7 +197,7 @@ impl SdtHeader {
 
         // This is usually redundant compared to simply calling `validate_checksum` but respects custom
         // `AcpiTable::validate` implementations.
-        table_mapping.validate()?;
+        table_mapping.get().validate()?;
 
         Ok(table_mapping)
     }