From 753a89a4eaf42dfea1808e20e8f0ea665e72d9fc Mon Sep 17 00:00:00 2001 From: Isaac Woods Date: Sun, 6 Oct 2024 21:16:19 +0100 Subject: [PATCH 1/2] Prevent getting a raw reference from `PhysicalMapping` if `T: !Unpin` --- acpi/src/handler.rs | 11 ++++++----- acpi/src/platform/mod.rs | 2 +- acpi/src/sdt.rs | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/acpi/src/handler.rs b/acpi/src/handler.rs index 46a65328..6a93d057 100644 --- a/acpi/src/handler.rs +++ b/acpi/src/handler.rs @@ -1,8 +1,4 @@ -use core::{ - fmt, - ops::{Deref, DerefMut}, - 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::()` @@ -73,6 +69,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 } @@ -90,6 +90,7 @@ unsafe impl Send for PhysicalMapping {} impl Deref for PhysicalMapping where + T: Unpin, H: AcpiHandler, { type Target = T; diff --git a/acpi/src/platform/mod.rs b/acpi/src/platform/mod.rs index 1e8df4c8..ea8d1624 100644 --- a/acpi/src/platform/mod.rs +++ b/acpi/src/platform/mod.rs @@ -124,7 +124,7 @@ where let madt = tables.find_table::(); 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)?; diff --git a/acpi/src/sdt.rs b/acpi/src/sdt.rs index 9ea72f33..9d92624a 100644 --- a/acpi/src/sdt.rs +++ b/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) } From 915e81655a647026f6a097a6b1ef6b3fce69f61a Mon Sep 17 00:00:00 2001 From: Isaac Woods Date: Sun, 6 Oct 2024 21:18:44 +0100 Subject: [PATCH 2/2] Make the MADT `!Unpin` to prevent unsoundness This prevents an `Madt` being moved away from its following, but not represented, dynamic entries. This would previously have caused unsoundness as arbitrary memory would be read from after wherever the `Madt` had ended up. By prevening the user from getting anything other than a `Pin<&Madt>`, this is prevented. --- acpi/src/madt.rs | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/acpi/src/madt.rs b/acpi/src/madt.rs index c89a6731..4f65c17b 100644 --- a/acpi/src/madt.rs +++ b/acpi/src/madt.rs @@ -4,7 +4,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::{ @@ -29,16 +33,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)] +/// - 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. @@ -62,7 +72,7 @@ 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>)> where @@ -107,7 +117,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>)> where @@ -311,9 +321,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::()) }, + pointer: unsafe { ptr.add(mem::size_of::()) }, remaining_length: self.header.length - mem::size_of::() as u32, _phantom: PhantomData, }