Skip to content

Commit

Permalink
Merge pull request #460 from Freax13/zeroooooo-copyyyy
Browse files Browse the repository at this point in the history 
get rid of unnecessary uses of unsafe
  • Loading branch information
@joergroedel
joergroedel authored Oct 25, 2024
2 parents cc8b754 + 9ec1021 commit 787283d
Show file tree
Hide file tree
Showing 7 changed files with 84 additions and 171 deletions.
85 changes: 34 additions & 51 deletions kernel/src/acpi/tables.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@ use crate::fw_cfg::FwCfg;
use crate::string::FixedString;
use alloc::vec::Vec;
use core::mem;
use zerocopy::{FromBytes, FromZeros, Immutable, IntoBytes, KnownLayout};

/// ACPI Root System Description Pointer (RSDP)
/// used by ACPI programming interface
#[derive(Debug, Default)]
#[derive(Debug, Default, FromBytes, IntoBytes)]
#[repr(C, packed)]
struct RSDPDesc {
/// Signature must contain "RSD PTR"
Expand All @@ -40,7 +41,6 @@ impl RSDPDesc {
///
/// A [`Result`] containing the [`RSDPDesc`] if successful, or an [`SvsmError`] on failure.
fn from_fwcfg(fw_cfg: &FwCfg<'_>) -> Result<Self, SvsmError> {
let mut buf = mem::MaybeUninit::<Self>::uninit();
let path = option_env!("ACPI_RSDP_PATH").unwrap_or("etc/acpi/rsdp");
let file = fw_cfg.file_selector(path)?;

Expand All @@ -49,17 +49,13 @@ impl RSDPDesc {
}

fw_cfg.select(file.selector());
let ptr = buf.as_mut_ptr().cast::<u8>();
for i in 0..mem::size_of::<Self>() {
let byte: u8 = fw_cfg.read_le();
unsafe { ptr.add(i).write(byte) };
}

unsafe { Ok(buf.assume_init()) }
let mut this = Self::new_zeroed();
fw_cfg.read_bytes(this.as_mut_bytes());
Ok(this)
}
}

#[derive(Copy, Clone, Debug, Default)]
#[derive(Copy, Clone, Debug, Default, FromBytes, KnownLayout, Immutable)]
#[repr(C, packed)]
/// Raw header of an ACPI table. It corresponds to the beginning
/// portion of ACPI tables, before any specific table data
Expand Down Expand Up @@ -167,20 +163,17 @@ impl ACPITable {
///
/// A new [`ACPITable`] instance on success, or an [`SvsmError`] if parsing fails.
fn new(ptr: &[u8]) -> Result<Self, SvsmError> {
let raw_header = ptr
.get(..mem::size_of::<RawACPITableHeader>())
.ok_or(SvsmError::Acpi)?
.as_ptr()
.cast::<RawACPITableHeader>();
let size = unsafe { (*raw_header).len as usize };
let (raw_header, _) =
RawACPITableHeader::read_from_prefix(ptr).map_err(|_| SvsmError::Acpi)?;
let size = raw_header.len as usize;
let content = ptr.get(..size).ok_or(SvsmError::Acpi)?;

let mut buf = Vec::<u8>::new();
// Allow for a failable allocation before copying
buf.try_reserve(size).map_err(|_| SvsmError::Mem)?;
buf.extend_from_slice(content);

let header = unsafe { ACPITableHeader::new(*raw_header) };
let header = ACPITableHeader::new(raw_header);

Ok(Self { header, buf })
}
Expand Down Expand Up @@ -218,11 +211,14 @@ impl ACPITable {
///
/// # Returns
///
/// A pointer to the content of the ACPI table at specified offset as type `T`,
/// A reference to the content of the ACPI table at specified offset as type `T`,
/// or [`None`] if the offset is out of bounds.
fn content_ptr<T>(&self, offset: usize) -> Option<*const T> {
let end = offset.checked_add(mem::size_of::<T>())?;
Some(self.content()?.get(offset..end)?.as_ptr().cast::<T>())
fn content_ptr<T>(&self, offset: usize) -> Option<&T>
where
T: FromBytes + KnownLayout + Immutable,
{
let bytes = self.content()?.get(offset..)?;
T::ref_from_prefix(bytes).ok().map(|(value, _rest)| value)
}
}

Expand Down Expand Up @@ -289,14 +285,9 @@ impl ACPITableBuffer {
return Err(SvsmError::Mem);
}
buf.try_reserve(size).map_err(|_| SvsmError::Mem)?;
let ptr = buf.as_mut_ptr();

buf.resize(size, 0);
fw_cfg.select(file.selector());
for i in 0..size {
let byte: u8 = fw_cfg.read_le();
unsafe { ptr.add(i).write(byte) };
}
unsafe { buf.set_len(size) }
fw_cfg.read_bytes(&mut buf);

let mut acpibuf = Self {
buf,
Expand Down Expand Up @@ -328,15 +319,10 @@ impl ACPITableBuffer {
.map(|c| u32::from_le_bytes(c.try_into().unwrap()) as usize);

for offset in offsets {
let raw_header = offset
.checked_add(mem::size_of::<RawACPITableHeader>())
.and_then(|end| self.buf.get(offset..end))
.ok_or(SvsmError::Acpi)?
.as_ptr()
.cast::<RawACPITableHeader>();

let meta = unsafe { ACPITableMeta::new(&*raw_header, offset) };

let raw_header = self.buf.get(offset..).ok_or(SvsmError::Acpi)?;
let (raw_header, _) =
RawACPITableHeader::ref_from_prefix(raw_header).map_err(|_| SvsmError::Acpi)?;
let meta = ACPITableMeta::new(raw_header, offset);
self.tables.push(meta);
}

Expand Down Expand Up @@ -387,15 +373,15 @@ impl ACPITableBuffer {
const MADT_HEADER_SIZE: usize = 8;

/// Header of an entry within MADT
#[derive(Clone, Copy, Debug)]
#[derive(Clone, Copy, Debug, FromBytes, Immutable, KnownLayout)]
#[repr(C, packed)]
struct RawMADTEntryHeader {
entry_type: u8,
entry_len: u8,
}

/// Entry for a local APIC within MADT
#[derive(Clone, Copy, Debug)]
#[derive(Clone, Copy, Debug, FromBytes, Immutable, KnownLayout)]
#[repr(C, packed)]
struct RawMADTEntryLocalApic {
header: RawMADTEntryHeader,
Expand All @@ -405,7 +391,7 @@ struct RawMADTEntryLocalApic {
}

/// Entry for a local X2APIC within MADT
#[derive(Clone, Copy, Debug)]
#[derive(Clone, Copy, Debug, FromBytes, Immutable, KnownLayout)]
#[repr(C, packed)]
struct RawMADTEntryLocalX2Apic {
header: RawMADTEntryHeader,
Expand Down Expand Up @@ -488,38 +474,35 @@ pub fn load_acpi_cpu_info(fw_cfg: &FwCfg<'_>) -> Result<Vec<ACPICPUInfo>, SvsmEr
let entry_ptr = apic_table
.content_ptr::<RawMADTEntryHeader>(offset)
.ok_or(SvsmError::Acpi)?;
let (madt_type, entry_len) = unsafe { ((*entry_ptr).entry_type, (*entry_ptr).entry_len) };
let entry_len = usize::from(entry_len);
let entry_len = usize::from(entry_ptr.entry_len);

match madt_type {
match entry_ptr.entry_type {
0 if entry_len == mem::size_of::<RawMADTEntryLocalApic>() => {
let lapic_ptr = apic_table
.content_ptr::<RawMADTEntryLocalApic>(offset)
.ok_or(SvsmError::Acpi)?;
let (apic_id, flags) = unsafe { ((*lapic_ptr).apic_id as u32, (*lapic_ptr).flags) };
cpus.push(ACPICPUInfo {
apic_id,
enabled: (flags & 1) == 1,
apic_id: lapic_ptr.apic_id as u32,
enabled: (lapic_ptr.flags & 1) == 1,
});
}
9 if entry_len == mem::size_of::<RawMADTEntryLocalX2Apic>() => {
let x2apic_ptr = apic_table
.content_ptr::<RawMADTEntryLocalX2Apic>(offset)
.ok_or(SvsmError::Acpi)?;
let (apic_id, flags) = unsafe { ((*x2apic_ptr).apic_id, (*x2apic_ptr).flags) };
cpus.push(ACPICPUInfo {
apic_id,
enabled: (flags & 1) == 1,
apic_id: x2apic_ptr.apic_id,
enabled: (x2apic_ptr.flags & 1) == 1,
});
}
_ if entry_len == 0 => {
madt_type if entry_len == 0 => {
log::warn!(
"Found zero-length MADT entry with type {}, stopping",
madt_type
);
break;
}
_ => {
madt_type => {
log::info!("Ignoring MADT entry with type {}", madt_type);
}
}
Expand Down
6 changes: 6 additions & 0 deletions kernel/src/fw_cfg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ impl<'a> FwCfg<'a> {
self.driver.outw(FW_CFG_CTL, cfg);
}

pub fn read_bytes(&self, out: &mut [u8]) {
for byte in out.iter_mut() {
*byte = self.driver.inb(FW_CFG_DATA);
}
}

pub fn read_le<T>(&self) -> T
where
T: core::ops::Shl<usize, Output = T> + core::ops::BitOr<T, Output = T> + From<u8>,
Expand Down
75 changes: 15 additions & 60 deletions kernel/src/fw_meta.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ use crate::types::{PageSize, PAGE_SIZE};
use crate::utils::{zero_mem_region, MemoryRegion};
use alloc::vec::Vec;
use bootlib::kernel_launch::KernelLaunchInfo;
use zerocopy::{FromBytes, Immutable, KnownLayout};

use core::fmt;
use core::mem::{align_of, size_of, size_of_val};
use core::mem::{size_of, size_of_val};
use core::str::FromStr;

#[derive(Clone, Debug, Default)]
Expand Down Expand Up @@ -135,7 +136,7 @@ const OVMF_TABLE_FOOTER_GUID: &str = "96b582de-1fb2-45f7-baea-a366c55a082d";
const OVMF_SEV_META_DATA_GUID: &str = "dc886566-984a-4798-a75e-5585a7bf67cc";
const SVSM_INFO_GUID: &str = "a789a612-0597-4c4b-a49f-cbb1fe9d1ddd";

#[derive(Clone, Copy, Debug)]
#[derive(Clone, Copy, Debug, FromBytes, KnownLayout, Immutable)]
#[repr(C, packed)]
struct SevMetaDataHeader {
signature: [u8; 4],
Expand All @@ -144,7 +145,7 @@ struct SevMetaDataHeader {
num_desc: u32,
}

#[derive(Clone, Copy, Debug)]
#[derive(Clone, Copy, Debug, FromBytes, KnownLayout, Immutable)]
#[repr(C, packed)]
struct SevMetaDataDesc {
base: u32,
Expand All @@ -157,7 +158,7 @@ const SEV_META_DESC_TYPE_SECRETS: u32 = 2;
const SEV_META_DESC_TYPE_CPUID: u32 = 3;
const SEV_META_DESC_TYPE_CAA: u32 = 4;

#[derive(Debug, Clone, Copy)]
#[derive(Debug, Clone, Copy, FromBytes, KnownLayout, Immutable)]
#[repr(C, packed)]
struct RawMetaHeader {
len: u16,
Expand All @@ -171,7 +172,7 @@ impl RawMetaHeader {
}
}

#[derive(Debug, Clone, Copy)]
#[derive(Debug, Clone, Copy, FromBytes, KnownLayout, Immutable)]
#[repr(C, packed)]
struct RawMetaBuffer {
data: [u8; PAGE_SIZE - size_of::<RawMetaHeader>() - 32],
Expand All @@ -190,14 +191,7 @@ fn find_table<'a>(uuid: &Uuid, mem: &'a [u8]) -> Option<&'a [u8]> {

while idx != 0 {
let hdr_start = idx.checked_sub(size_of::<RawMetaHeader>())?;
let hdr_start_ptr = mem.get(hdr_start..idx)?.as_ptr().cast::<RawMetaHeader>();
if hdr_start_ptr.align_offset(align_of::<RawMetaHeader>()) != 0 {
log::error!("Misaligned firmware metadata table");
return None;
}

// Safety: we have checked the pointer is within bounds and aligned
let hdr = unsafe { hdr_start_ptr.read() };
let hdr = RawMetaHeader::ref_from_bytes(&mem[hdr_start..idx]).unwrap();

let data_len = hdr.data_len()?;
idx = hdr_start.checked_sub(data_len)?;
Expand All @@ -216,14 +210,7 @@ fn find_table<'a>(uuid: &Uuid, mem: &'a [u8]) -> Option<&'a [u8]> {
pub fn parse_fw_meta_data(mem: &[u8]) -> Result<SevFWMetaData, SvsmError> {
let mut meta_data = SevFWMetaData::new();

if mem.len() != size_of::<RawMetaBuffer>() {
return Err(SvsmError::Firmware);
}

// Safety: `RawMetaBuffer` has no invalid representations and is
// `repr(C, packed)`, which means there are no alignment requirements.
// We have also verified that the size of the slice matches.
let raw_meta = unsafe { &*mem.as_ptr().cast::<RawMetaBuffer>() };
let raw_meta = RawMetaBuffer::ref_from_bytes(mem).map_err(|_| SvsmError::Firmware)?;

// Check the UUID
let raw_uuid = raw_meta.header.uuid;
Expand Down Expand Up @@ -283,51 +270,19 @@ fn parse_sev_meta(
.ok_or(SvsmError::Firmware)?;
let sev_meta_end = sev_meta_start + size_of::<SevMetaDataHeader>();
// Bounds check the header and get a pointer to it
let sev_meta_hdr_ptr = raw_meta
let bytes = raw_meta
.data
.get(sev_meta_start..sev_meta_end)
.ok_or(SvsmError::Firmware)?
.as_ptr()
.cast::<SevMetaDataHeader>();

// Check that the header pointer is aligned. This also guarantees that
// descriptors down the line will be aligned. If the pointer was not
// aligned we would need to use ptr::read_unaligned(), so simply check
// beforehand and use ptr::read(), as there's no reason for the metadata
// to be misaligned.
if sev_meta_hdr_ptr.align_offset(align_of::<SevMetaDataHeader>()) != 0 {
log::error!("Misaligned SEV metadata header");
return Err(SvsmError::Firmware);
}
// Safety: we have checked the pointer is within bounds and aligned.
let sev_meta_hdr = unsafe { sev_meta_hdr_ptr.read() };
.ok_or(SvsmError::Firmware)?;
let sev_meta_hdr = SevMetaDataHeader::ref_from_bytes(bytes).map_err(|_| SvsmError::Firmware)?;

// Now find the descriptors
let bytes = &raw_meta.data[sev_meta_end..];
let num_desc = sev_meta_hdr.num_desc as usize;
let sev_descs_start = sev_meta_end;
let sev_descs_len = num_desc
.checked_mul(size_of::<SevMetaDataDesc>())
.ok_or(SvsmError::Firmware)?;
let sev_descs_end = sev_descs_start
.checked_add(sev_descs_len)
.ok_or(SvsmError::Firmware)?;
let (descs, _) = <[SevMetaDataDesc]>::ref_from_prefix_with_elems(bytes, num_desc)
.map_err(|_| SvsmError::Firmware)?;

// We have a variable number of descriptors following the header.
// Unfortunately flexible array members in Rust are not fully supported,
// so we cannot avoid using raw pointers.
let sev_descs_ptr = raw_meta
.data
.get(sev_descs_start..sev_descs_end)
.ok_or(SvsmError::Firmware)?
.as_ptr()
.cast::<SevMetaDataDesc>();

for i in 0..num_desc {
// Safety: We have checked that the descriptors are within bounds of
// the metadata memory. Since the descriptors follow the header, and
// the header is properly aligned, the descriptors must be so as
// well.
let desc = unsafe { sev_descs_ptr.add(i).read() };
for desc in descs {
let t = desc.t;
let base = PhysAddr::from(desc.base as usize);
let len = desc.len as usize;
Expand Down
Loading

0 comments on commit 787283d

Please sign in to comment.