Merge pull request #102 from Freax13/fix-physical-mapping-soundness

Fix physical mapping soundness

acpi/Cargo.toml

@@ -12,4 +12,4 @@ edition = "2018"
 [dependencies]
 log = "0.4"
 bit_field = "0.10"
-rsdp = "1"
+rsdp = { version = "1", path = "../rsdp" }

acpi/src/lib.rs

@@ -163,7 +163,7 @@ where
 
             let num_tables = (mapping.length as usize - mem::size_of::<SdtHeader>()) / mem::size_of::<u32>();
             let tables_base =
-                ((mapping.virtual_start.as_ptr() as usize) + mem::size_of::<SdtHeader>()) as *const u32;
+                ((mapping.virtual_start().as_ptr() as usize) + mem::size_of::<SdtHeader>()) as *const u32;
 
             for i in 0..num_tables {
                 result.process_sdt(unsafe { *tables_base.offset(i as isize) as usize })?;
@@ -176,7 +176,7 @@ where
 
             let num_tables = (mapping.length as usize - mem::size_of::<SdtHeader>()) / mem::size_of::<u64>();
             let tables_base =
-                ((mapping.virtual_start.as_ptr() as usize) + mem::size_of::<SdtHeader>()) as *const u64;
+                ((mapping.virtual_start().as_ptr() as usize) + mem::size_of::<SdtHeader>()) as *const u64;
 
             for i in 0..num_tables {
                 result.process_sdt(unsafe { *tables_base.offset(i as isize) as usize })?;

rsdp/src/handler.rs

@@ -7,11 +7,56 @@ pub struct PhysicalMapping<H, T>
 where
     H: AcpiHandler,
 {
-    pub physical_start: usize,
-    pub virtual_start: NonNull<T>,
-    pub region_length: usize, // Can be equal or larger than size_of::<T>()
-    pub mapped_length: usize, // Differs from `region_length` if padding is added for alignment
-    pub handler: H,
+    physical_start: usize,
+    virtual_start: NonNull<T>,
+    region_length: usize, // Can be equal or larger than size_of::<T>()
+    mapped_length: usize, // Differs from `region_length` if padding is added for alignment
+    handler: H,
+}
+
+impl<H, T> PhysicalMapping<H, T>
+where
+    H: AcpiHandler,
+{
+    /// Construct a new `PhysicalMapping`.
+    /// `mapped_length` may differ from `region_length` if padding is added for alignment.
+    ///
+    /// ## Safety
+    ///
+    /// This function must only be called by an `AcpiHandler` of type `H` to make sure that it's safe to unmap the mapping.
+    ///
+    /// - `virtual_start` must be a valid pointer.
+    /// - `region_length` must be equal to or larger than `size_of::<T>()`.
+    /// - `handler` must be the same `AcpiHandler` that created the mapping.
+    pub unsafe fn new(
+        physical_start: usize,
+        virtual_start: NonNull<T>,
+        region_length: usize,
+        mapped_length: usize,
+        handler: H,
+    ) -> Self {
+        Self { physical_start, virtual_start, region_length, mapped_length, handler }
+    }
+
+    pub fn physical_start(&self) -> usize {
+        self.physical_start
+    }
+
+    pub fn virtual_start(&self) -> NonNull<T> {
+        self.virtual_start
+    }
+
+    pub fn region_length(&self) -> usize {
+        self.region_length
+    }
+
+    pub fn mapped_length(&self) -> usize {
+        self.mapped_length
+    }
+
+    pub fn handler(&self) -> &H {
+        &self.handler
+    }
 }
 
 impl<H, T> Deref for PhysicalMapping<H, T>
@@ -30,7 +75,7 @@ where
     H: AcpiHandler,
 {
     fn drop(&mut self) {
-        self.handler.unmap_physical_region(self)
+        H::unmap_physical_region(self)
     }
 }
 
@@ -39,13 +84,20 @@ where
 /// however you please, as long as they conform to the documentation of each function. The handler is stored in
 /// every `PhysicalMapping` so it's able to unmap itself when dropped, so this type needs to be something you can
 /// clone/move about freely (e.g. a reference, wrapper over `Rc`, marker struct, etc.).
-pub trait AcpiHandler: Clone + Sized {
+pub trait AcpiHandler: Clone {
     /// Given a physical address and a size, map a region of physical memory that contains `T` (note: the passed
     /// size may be larger than `size_of::<T>()`). The address is not neccessarily page-aligned, so the
     /// implementation may need to map more than `size` bytes. The virtual address the region is mapped to does not
     /// matter, as long as it is accessible to `acpi`.
+    ///
+    /// ## Safety
+    ///
+    /// - `physical_address` must point to a valid `T` in physical memory.
+    /// - `size` must be at least `size_of::<T>()`.
     unsafe fn map_physical_region<T>(&self, physical_address: usize, size: usize) -> PhysicalMapping<Self, T>;
 
-    /// Unmap the given physical mapping. This is called when a `PhysicalMapping` is dropped.
-    fn unmap_physical_region<T>(&self, region: &PhysicalMapping<Self, T>);
+    /// Unmap the given physical mapping. This is called when a `PhysicalMapping` is dropped, you should **not** manually call this.
+    ///
+    /// Note: A reference to the handler used to construct `region` can be acquired by calling [`PhysicalMapping::handler`].
+    fn unmap_physical_region<T>(region: &PhysicalMapping<Self, T>);
 }

rsdp/src/lib.rs

@@ -83,7 +83,7 @@ impl Rsdp {
 
                 for address in area.clone().step_by(16) {
                     let ptr_in_mapping =
-                        unsafe { mapping.virtual_start.as_ptr().offset((address - area.start) as isize) };
+                        unsafe { mapping.virtual_start().as_ptr().offset((address - area.start) as isize) };
                     let signature = unsafe { *(ptr_in_mapping as *const [u8; 8]) };
 
                     if signature == *RSDP_SIGNATURE {