From 2cefa7e5fa5c3804b97265df6bdd32e1fbad92e2 Mon Sep 17 00:00:00 2001 From: Ruihan Li Date: Wed, 26 Feb 2025 20:46:13 +0800 Subject: [PATCH] Remove lots of unsafe code in `acpi/` --- ostd/src/arch/x86/kernel/acpi/dmar.rs | 100 ++++++------- ostd/src/arch/x86/kernel/acpi/mod.rs | 21 +-- ostd/src/arch/x86/kernel/acpi/remapping.rs | 155 ++++++++++----------- 3 files changed, 138 insertions(+), 138 deletions(-) diff --git a/ostd/src/arch/x86/kernel/acpi/dmar.rs b/ostd/src/arch/x86/kernel/acpi/dmar.rs index ad22e138..e8944e5b 100644 --- a/ostd/src/arch/x86/kernel/acpi/dmar.rs +++ b/ostd/src/arch/x86/kernel/acpi/dmar.rs @@ -3,7 +3,7 @@ #![expect(dead_code)] use alloc::vec::Vec; -use core::{fmt::Debug, mem::size_of, slice::Iter}; +use core::{fmt::Debug, slice::Iter}; use acpi::{ sdt::{SdtHeader, Signature}, @@ -11,17 +11,22 @@ use acpi::{ }; use super::remapping::{Andd, Atsr, Drhd, Rhsa, Rmrr, Satc, Sidp}; -use crate::mm::paddr_to_vaddr; -/// DMA Remapping structure. When IOMMU is enabled, the structure should be present in the ACPI table, -/// and the user can use the DRHD table in this structure to obtain the register base addresses used to configure functions such as IOMMU. +/// DMA Remapping structure. +/// +/// When IOMMU is enabled, the structure should be present in the ACPI table, and the user can use +/// the DRHD table in this structure to obtain the register base addresses used to configure +/// functions such IOMMU. #[derive(Debug)] pub struct Dmar { header: DmarHeader, - /// Actual size is indicated by `length` in header - remapping_structures: Vec, // Followed by `n` entries with format `Remapping Structures` + // The actual size is indicated by `length` in `header`. + // Entries with the format of Remapping Structures are followed. + remapping_structures: Vec, } +/// Remapping Structures. +/// /// A DMAR structure contains serval remapping structures. Among these structures, /// one DRHD must exist, the others must not exist at all. #[derive(Debug)] @@ -57,6 +62,8 @@ struct DmarHeader { reserved: [u8; 10], } +// SAFETY: The `DmarHeader` is the header for the DMAR structure. All its fields are described in +// the Intel manual. unsafe impl AcpiTable for DmarHeader { const SIGNATURE: Signature = Signature::DMAR; fn header(&self) -> &acpi::sdt::SdtHeader { @@ -67,53 +74,52 @@ unsafe impl AcpiTable for DmarHeader { impl Dmar { /// Creates a instance from ACPI table. pub fn new() -> Option { - if !super::ACPI_TABLES.is_completed() { - return None; - } - let acpi_table_lock = super::ACPI_TABLES.get().unwrap().lock(); - // SAFETY: The DmarHeader is the header for the DMAR structure, it fits all the field described in Intel manual. + let acpi_table_lock = super::ACPI_TABLES.get()?.lock(); + let dmar_mapping = acpi_table_lock.find_table::().ok()?; - let physical_address = dmar_mapping.physical_start(); - let len = dmar_mapping.mapped_length(); - // SAFETY: The target address is the start of the remapping structures, - // and the length is valid since the value is read from the length field in SDTHeader minus the size of DMAR header. - let dmar_slice = unsafe { - core::slice::from_raw_parts_mut( - paddr_to_vaddr(physical_address + size_of::()) as *mut u8, - len - size_of::(), + let header = *dmar_mapping; + // SAFETY: `find_table` returns a region of memory that belongs to the ACPI table. This + // memory region is valid to read, properly initialized, lives for `'static`, and will + // never be mutated. + let slice = unsafe { + core::slice::from_raw_parts( + dmar_mapping + .virtual_start() + .as_ptr() + .cast::() + .cast_const(), + dmar_mapping.mapped_length(), ) }; + let mut index = core::mem::size_of::(); let mut remapping_structures = Vec::new(); - let mut index = 0; - let mut remain_length = len - size_of::(); - // SAFETY: Indexes and offsets are strictly followed by the manual. - unsafe { - while remain_length > 0 { - // Common header: type: u16, length: u16 - let length = *dmar_slice[index + 2..index + 4].as_ptr() as usize; - let typ = *dmar_slice[index..index + 2].as_ptr() as usize; - let bytes = &&dmar_slice[index..index + length]; - let remapping = match typ { - 0 => Remapping::Drhd(Drhd::from_bytes(bytes)), - 1 => Remapping::Rmrr(Rmrr::from_bytes(bytes)), - 2 => Remapping::Atsr(Atsr::from_bytes(bytes)), - 3 => Remapping::Rhsa(Rhsa::from_bytes(bytes)), - 4 => Remapping::Andd(Andd::from_bytes(bytes)), - 5 => Remapping::Satc(Satc::from_bytes(bytes)), - 6 => Remapping::Sidp(Sidp::from_bytes(bytes)), - _ => { - panic!("Unidentified remapping structure type"); - } - }; - // let temp = DeviceScope::from_bytes( - // &bytes[index as usize..index as usize + length], - // ); - remapping_structures.push(remapping); - index += length; - remain_length -= length; - } + while index != (header.header.length as usize) { + // CommonHeader { type: u16, length: u16 } + let typ = u16::from_ne_bytes(slice[index..index + 2].try_into().unwrap()); + let length = + u16::from_ne_bytes(slice[index + 2..index + 4].try_into().unwrap()) as usize; + + let bytes = &slice[index..index + length]; + let remapping = match typ { + 0 => Remapping::Drhd(Drhd::from_bytes(bytes)), + 1 => Remapping::Rmrr(Rmrr::from_bytes(bytes)), + 2 => Remapping::Atsr(Atsr::from_bytes(bytes)), + 3 => Remapping::Rhsa(Rhsa::from_bytes(bytes)), + 4 => Remapping::Andd(Andd::from_bytes(bytes)), + 5 => Remapping::Satc(Satc::from_bytes(bytes)), + 6 => Remapping::Sidp(Sidp::from_bytes(bytes)), + _ => { + panic!( + "the type of the remapping structure is invalid or not supported: {}", + typ + ); + } + }; + remapping_structures.push(remapping); + + index += length; } Some(Dmar { diff --git a/ostd/src/arch/x86/kernel/acpi/mod.rs b/ostd/src/arch/x86/kernel/acpi/mod.rs index 0d488609..ac0b93c0 100644 --- a/ostd/src/arch/x86/kernel/acpi/mod.rs +++ b/ostd/src/arch/x86/kernel/acpi/mod.rs @@ -1,7 +1,5 @@ // SPDX-License-Identifier: MPL-2.0 -#![expect(unused_variables)] - pub mod dmar; pub mod remapping; @@ -29,16 +27,19 @@ impl AcpiHandler for AcpiMemoryHandler { physical_address: usize, size: usize, ) -> acpi::PhysicalMapping { - acpi::PhysicalMapping::new( - physical_address, - NonNull::new(paddr_to_vaddr(physical_address) as *mut T).unwrap(), - size, - size, - self.clone(), - ) + let virtual_address = NonNull::new(paddr_to_vaddr(physical_address) as *mut T).unwrap(); + + // SAFETY: The caller should guarantee that `physical_address..physical_address + size` is + // part of the ACPI table. Then the memory region is mapped to `virtual_address` and is + // valid for read and immutable dereferencing. + // FIXME: The caller guarantee only holds if we trust the hardware to provide a valid ACPI + // table. Otherwise, if the table is corrupted, it may reference arbitrary memory regions. + unsafe { + acpi::PhysicalMapping::new(physical_address, virtual_address, size, size, self.clone()) + } } - fn unmap_physical_region(region: &acpi::PhysicalMapping) {} + fn unmap_physical_region(_region: &acpi::PhysicalMapping) {} } pub fn init() { diff --git a/ostd/src/arch/x86/kernel/acpi/remapping.rs b/ostd/src/arch/x86/kernel/acpi/remapping.rs index 2a0a4c52..39c56832 100644 --- a/ostd/src/arch/x86/kernel/acpi/remapping.rs +++ b/ostd/src/arch/x86/kernel/acpi/remapping.rs @@ -1,20 +1,23 @@ // SPDX-License-Identifier: MPL-2.0 #![expect(dead_code)] -#![expect(unused_variables)] //! Remapping structures of DMAR table. -//! This file defines these structures and provides a "Debug" implementation to see the value inside these structures. +//! +//! This file defines these structures and provides a `Debug` implementation to see the value +//! inside these structures. +//! //! Most of the introduction are copied from Intel vt-directed-io-specification. -use alloc::{string::String, vec::Vec}; -use core::{fmt::Debug, mem::size_of}; +use alloc::{borrow::ToOwned, string::String, vec::Vec}; +use core::fmt::Debug; + +use ostd_pod::Pod; /// DMA-remapping hardware unit definition (DRHD). /// /// A DRHD structure uniquely represents a remapping hardware unit present in the platform. -/// There must be at least one instance of this structure for each -/// PCI segment in the platform. +/// There must be at least one instance of this structure for each PCI segment in the platform. #[derive(Debug, Clone)] pub struct Drhd { header: DrhdHeader, @@ -28,7 +31,7 @@ impl Drhd { } #[repr(C)] -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Pod)] pub struct DrhdHeader { typ: u16, length: u16, @@ -50,7 +53,7 @@ pub struct Rmrr { } #[repr(C)] -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Pod)] pub struct RmrrHeader { typ: u16, length: u16, @@ -71,7 +74,7 @@ pub struct Atsr { } #[repr(C)] -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Pod)] pub struct AtsrHeader { typ: u16, length: u16, @@ -82,11 +85,12 @@ pub struct AtsrHeader { /// Remapping Hardware Status Affinity (RHSA). /// -/// It is applicable for platforms supporting non-uniform memory (NUMA), where Remapping hardware units spans across nodes. -/// This optional structure provides the association between each Remapping hardware unit (identified by its -/// espective Base Address) and the proximity domain to which that hardware unit belongs. +/// It is applicable for platforms supporting non-uniform memory (NUMA), +/// where Remapping hardware units spans across nodes. +/// This optional structure provides the association between each Remapping hardware unit (identified +/// by its espective Base Address) and the proximity domain to which that hardware unit belongs. #[repr(C)] -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Pod)] pub struct Rhsa { typ: u16, length: u16, @@ -106,7 +110,7 @@ pub struct Andd { } #[repr(C)] -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Pod)] pub struct AnddHeader { typ: u16, length: u16, @@ -125,7 +129,7 @@ pub struct Satc { } #[repr(C)] -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Pod)] pub struct SatcHeader { typ: u16, length: u16, @@ -139,7 +143,6 @@ pub struct SatcHeader { /// The (SIDP) reporting structure identifies devices that have special /// properties and that may put restrictions on how system software must configure remapping /// structures that govern such devices in a platform where remapping hardware is enabled. -/// #[derive(Debug, Clone)] pub struct Sidp { header: SidpHeader, @@ -147,7 +150,7 @@ pub struct Sidp { } #[repr(C)] -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Pod)] pub struct SidpHeader { typ: u16, length: u16, @@ -164,7 +167,7 @@ pub struct DeviceScope { } #[repr(C)] -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Pod)] pub struct DeviceScopeHeader { typ: u8, length: u8, @@ -175,35 +178,32 @@ pub struct DeviceScopeHeader { } macro_rules! impl_from_bytes { - ($(($struct:tt,$header_struct:tt,$dst_name:ident)),*) => { + ($(($struct:tt, $header_struct:tt),)*) => { $(impl $struct { - /// Creates instance from bytes + #[doc = concat!("Parses a [`", stringify!($struct), "`] from bytes.")] /// - /// # Safety + /// # Panics /// - /// User must ensure the bytes is valid. - /// - pub unsafe fn from_bytes(bytes: &[u8]) -> Self { - let length = u16_from_slice(&bytes[2..4]) as usize; - debug_assert_eq!(length, bytes.len()); + #[doc = concat!( + "This method may panic if the bytes do not represent a valid [`", + stringify!($struct), + "`].", + )] + pub fn from_bytes(bytes: &[u8]) -> Self { + let header = $header_struct::from_bytes(bytes); + debug_assert_eq!(header.length as usize, bytes.len()); let mut index = core::mem::size_of::<$header_struct>(); - let mut remain_length = length - core::mem::size_of::<$header_struct>(); - let mut $dst_name = Vec::new(); - while remain_length > 0 { - let length = *bytes[index + 1..index + 2].as_ptr() as usize; - let temp = DeviceScope::from_bytes( - &bytes[index..index + length], - ); - $dst_name.push(temp); - index += length; - remain_length -= length; + let mut device_scopes = Vec::new(); + while index != (header.length as usize) { + let val = DeviceScope::from_bytes_prefix(&bytes[index..]); + index += val.header.length as usize; + device_scopes.push(val); } - let header = *(bytes.as_ptr() as *const $header_struct); Self{ header, - $dst_name + device_scopes, } } })* @@ -211,33 +211,31 @@ macro_rules! impl_from_bytes { } impl_from_bytes!( - (Drhd, DrhdHeader, device_scopes), - (Rmrr, RmrrHeader, device_scopes), - (Atsr, AtsrHeader, device_scopes), - (Satc, SatcHeader, device_scopes), - (Sidp, SidpHeader, device_scopes) + (Drhd, DrhdHeader), + (Rmrr, RmrrHeader), + (Atsr, AtsrHeader), + (Satc, SatcHeader), + (Sidp, SidpHeader), ); impl DeviceScope { - /// Creates instance from bytes + /// Parses a [`DeviceScope`] from a prefix of the bytes. /// - /// # Safety + /// # Panics /// - /// User must ensure the bytes is valid. - /// - unsafe fn from_bytes(bytes: &[u8]) -> Self { - let length = bytes[1] as u32; - debug_assert_eq!(length, bytes.len() as u32); - let header = *(bytes.as_ptr() as *const DeviceScopeHeader); + /// This method may panic if the byte prefix does not represent a valid [`DeviceScope`]. + fn from_bytes_prefix(bytes: &[u8]) -> Self { + let header = DeviceScopeHeader::from_bytes(bytes); + debug_assert!((header.length as usize) <= bytes.len()); + + let mut index = core::mem::size_of::(); + debug_assert!((header.length as usize) >= index); - let mut index = size_of::(); - let mut remain_length = length - index as u32; let mut path = Vec::new(); - while remain_length > 0 { - let temp: (u8, u8) = *(bytes[index..index + 2].as_ptr() as *const (u8, u8)); - path.push(temp); + while index != (header.length as usize) { + let val = (bytes[index], bytes[index + 1]); + path.push(val); index += 2; - remain_length -= 2; } Self { header, path } @@ -245,42 +243,37 @@ impl DeviceScope { } impl Rhsa { - /// Creates instance from bytes + /// Parses an [`Rhsa`] from the bytes. /// - /// # Safety + /// # Panics /// - /// User must ensure the bytes is valid. - /// - pub unsafe fn from_bytes(bytes: &[u8]) -> Self { - let length = u16_from_slice(&bytes[2..4]) as u32; - debug_assert_eq!(length, bytes.len() as u32); - *(bytes.as_ptr() as *const Self) + /// This method may panic if the bytes do not represent a valid [`Rhsa`]. + pub fn from_bytes(bytes: &[u8]) -> Self { + let val = ::from_bytes(bytes); + debug_assert_eq!(val.length as usize, bytes.len()); + + val } } impl Andd { - /// Creates instance from bytes + /// Parses an [`Andd`] from the bytes. /// - /// # Safety + /// # Panics /// - /// User must ensure the bytes is valid. - /// - pub unsafe fn from_bytes(bytes: &[u8]) -> Self { - let length = u16_from_slice(&bytes[2..4]) as usize; - debug_assert_eq!(length, bytes.len()); + /// This method may panic if the bytes do not represent a valid [`Andd`]. + pub fn from_bytes(bytes: &[u8]) -> Self { + let header = AnddHeader::from_bytes(bytes); + debug_assert_eq!(header.length as usize, bytes.len()); - let index = core::mem::size_of::(); - let remain_length = length - core::mem::size_of::(); - let string = String::from_utf8(bytes[index..index + length].to_vec()).unwrap(); + let header_len = core::mem::size_of::(); + let acpi_object_name = core::str::from_utf8(&bytes[header_len..]) + .unwrap() + .to_owned(); - let header = *(bytes.as_ptr() as *const AnddHeader); Self { header, - acpi_object_name: string, + acpi_object_name, } } } - -fn u16_from_slice(input: &[u8]) -> u16 { - u16::from_ne_bytes(input[0..size_of::()].try_into().unwrap()) -}