Skip to content

Commit

Permalink
Make the MADT !Unpin to prevent unsoundness
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
IsaacWoods committed Oct 6, 2024
1 parent 37d4bb2 commit d2d8932
Showing 1 changed file with 21 additions and 10 deletions.
31 changes: 21 additions & 10 deletions acpi/src/madt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand All @@ -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)
/// - 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`, and so prevents a `Madt` from being moved before [`Madt::entries`] is called.
#[repr(C, packed)]
#[derive(Debug, Clone, Copy)]
#[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.
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
}
Expand Down

0 comments on commit d2d8932

Please sign in to comment.