Skip to content

Commit

Permalink
chore: remove all unsafe
Browse files Browse the repository at this point in the history
This commit removes all unsafe (entirely related to transmuting) from
the code. Instead, Google's zerocopy crate is used to do the
transmuting. Being widely used and thoroughly vetted, it is less likely
to cause hidden soundness issues.

Signed-off-by: wucke13 <[email protected]>
  • Loading branch information
wucke13 committed Jul 10, 2024
1 parent bd3151e commit 382c293
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 43 deletions.
28 changes: 28 additions & 0 deletions tool/microkit/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions tool/microkit/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ path = "src/main.rs"
roxmltree = "0.19.0"
serde = "1.0.203"
serde_json = "1.0.117"
zerocopy = { version = "0.7.35", features = ["derive"] }
22 changes: 11 additions & 11 deletions tool/microkit/src/elf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
use std::fs;
use std::path::Path;
use std::collections::HashMap;
use crate::util::bytes_to_struct;

use zerocopy::{FromBytes, FromZeroes};

#[repr(C, packed)]
struct ElfHeader32 {
Expand All @@ -34,7 +35,7 @@ struct ElfHeader32 {
}

#[repr(C, packed)]
#[derive(Copy, Clone)]
#[derive(Copy, Clone, FromBytes, FromZeroes)]
struct ElfSymbol64 {
name: u32,
info: u8,
Expand All @@ -45,6 +46,7 @@ struct ElfSymbol64 {
}

#[repr(C, packed)]
#[derive(FromBytes, FromZeroes)]
struct ElfSectionHeader64 {
name: u32,
type_: u32,
Expand All @@ -59,6 +61,7 @@ struct ElfSectionHeader64 {
}

#[repr(C, packed)]
#[derive(FromBytes, FromZeroes)]
struct ElfProgramHeader64 {
type_: u32,
flags: u32,
Expand All @@ -71,6 +74,7 @@ struct ElfProgramHeader64 {
}

#[repr(C, packed)]
#[derive(FromBytes, FromZeroes)]
struct ElfHeader64 {
ident_magic: u32,
ident_class: u8,
Expand Down Expand Up @@ -168,7 +172,7 @@ impl ElfFile {

// Now need to read the header into a struct
let hdr_bytes = &bytes[..hdr_size];
let hdr = unsafe { bytes_to_struct::<ElfHeader64>(hdr_bytes) };
let hdr = ElfHeader64::ref_from(hdr_bytes).expect("wrong size for ElfHeader64");

// We have checked this above but we should check again once we actually cast it to
// a struct.
Expand All @@ -188,7 +192,7 @@ impl ElfFile {
let phent_end = phent_start + (hdr.phentsize as u64);
let phent_bytes = &bytes[phent_start as usize..phent_end as usize];

let phent = unsafe { bytes_to_struct::<ElfProgramHeader64>(phent_bytes) };
let phent = ElfProgramHeader64::ref_from(phent_bytes).expect("wrong size for ElfProgramHeader64");

let segment_start = phent.offset as usize;
let segment_end = phent.offset as usize + phent.filesz as usize;
Expand Down Expand Up @@ -216,7 +220,7 @@ impl ElfFile {
let shent_end = shent_start + hdr.shentsize as u64;
let shent_bytes = &bytes[shent_start as usize..shent_end as usize];

let shent = unsafe { bytes_to_struct::<ElfSectionHeader64>(shent_bytes) };
let shent = ElfSectionHeader64::ref_from(shent_bytes).expect("wrong size for ElfSectionHeader64");
match shent.type_ {
2 => symtab_shent = Some(shent),
3 => shstrtab_shent = Some(shent),
Expand Down Expand Up @@ -250,12 +254,8 @@ impl ElfFile {
let symbol_size = std::mem::size_of::<ElfSymbol64>();
while offset < symtab.len() {
let sym_bytes = &symtab[offset..offset + symbol_size];
let (sym_head, sym_body, sym_tail) = unsafe { sym_bytes.align_to::<ElfSymbol64>() };
assert!(sym_head.is_empty());
assert!(sym_body.len() == 1);
assert!(sym_tail.is_empty());

let sym = sym_body[0];
assert_eq!(std::mem::align_of::<ElfSymbol64>(), 1);
let sym = ElfSymbol64::read_from(sym_bytes).expect("wrong size for ElfSymbol64");

let name = Self::get_string(symtab_str, sym.name as usize)?;
// It is possible for a valid ELF to contain multiple global symbols with the same name.
Expand Down
10 changes: 7 additions & 3 deletions tool/microkit/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
// SPDX-License-Identifier: BSD-2-Clause
//

use zerocopy::AsBytes;

use crate::{MemoryRegion};
use crate::util::{round_up, mb, kb, mask, struct_to_bytes};
use crate::util::{round_up, mb, kb, mask};
use crate::elf::{ElfFile};
use crate::sel4::{Config};
use std::path::Path;
Expand Down Expand Up @@ -57,6 +59,7 @@ fn check_non_overlapping(regions: &Vec<(u64, &[u8])>) {
}

#[repr(C)]
#[derive(AsBytes)]
struct LoaderRegion64 {
load_addr: u64,
size: u64,
Expand All @@ -65,6 +68,7 @@ struct LoaderRegion64 {
}

#[repr(C)]
#[derive(AsBytes)]
struct LoaderHeader64 {
magic: u64,
flags: u64,
Expand Down Expand Up @@ -250,11 +254,11 @@ impl<'a> Loader<'a> {
loader_buf.write_all(self.image.as_slice()).expect("Failed to write image data to loader");

// Then we write out the loader metadata (known as the 'header')
let header_bytes = unsafe { struct_to_bytes(&self.header) };
let header_bytes = self.header.as_bytes();
loader_buf.write_all(header_bytes).expect("Failed to write header data to loader");
// For each region, we need to write out the region metadata as well
for region in &self.region_metadata {
let region_metadata_bytes = unsafe { struct_to_bytes(region) };
let region_metadata_bytes = region.as_bytes();
loader_buf.write_all(region_metadata_bytes).expect("Failed to write region metadata to loader");
}

Expand Down
20 changes: 11 additions & 9 deletions tool/microkit/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ use elf::ElfFile;
use sel4::{ArmVmAttributes, BootInfo, Object, Invocation, InvocationArgs, ObjectType, Rights, PageSize, Aarch64Regs, Config, Arch};
use sysxml::{parse, PlatformDescription, SystemDescription, ProtectionDomain, VirtualMachine, SysMap, SysMapPerms, SysMemoryRegion};
use loader::Loader;
use util::{json_str_as_u64, json_str_as_bool, bytes_to_struct, struct_to_bytes, comma_sep_usize, comma_sep_u64};
use util::{json_str_as_u64, json_str_as_bool, comma_sep_usize, comma_sep_u64};
use zerocopy::{AsBytes, FromBytes, FromZeroes};

// Corresponds to the IPC buffer symbol in libmicrokit and the monitor
const SYMBOL_IPC_BUFFER: &str = "__sel4_ipc_buffer_obj";
Expand Down Expand Up @@ -71,6 +72,7 @@ const INIT_ASID_POOL_CAP_ADDRESS: u64 = 6;
/// it omits the 'regions' field.
/// This struct assumes a 64-bit target
#[repr(C)]
#[derive(AsBytes)]
struct MonitorUntypedInfoHeader64 {
cap_start: u64,
cap_end: u64,
Expand All @@ -79,6 +81,7 @@ struct MonitorUntypedInfoHeader64 {
/// Corresponds to 'struct region' in the monitor
/// This struct assumes a 64-bit target
#[repr(C)]
#[derive(AsBytes)]
struct MonitorRegion64 {
paddr: u64,
size_bits: u64,
Expand Down Expand Up @@ -430,6 +433,7 @@ struct KernelPartialBootInfo {

// Corresponds to kernel_frame_t in the kernel
#[repr(C)]
#[derive(AsBytes, FromBytes, FromZeroes)]
struct KernelFrame64 {
pub paddr: u64,
pub pptr: u64,
Expand All @@ -447,9 +451,7 @@ fn kernel_device_addrs(kernel_config: &Config, kernel_elf: &ElfFile) -> Vec<u64>
let kernel_frame_size = size_of::<KernelFrame64>();
let mut offset: usize = 0;
while offset < size as usize {
let kernel_frame = unsafe {
bytes_to_struct::<KernelFrame64>(&kernel_frame_bytes[offset..offset + kernel_frame_size])
};
let kernel_frame = KernelFrame64::ref_from(& kernel_frame_bytes[offset..offset + kernel_frame_size]).expect("wrong size for KernelFrame64");
if kernel_frame.user_accessible == 0 {
kernel_devices.push(kernel_frame.paddr);
}
Expand All @@ -461,6 +463,7 @@ fn kernel_device_addrs(kernel_config: &Config, kernel_elf: &ElfFile) -> Vec<u64>

// Corresponds to p_region_t in the kernel
#[repr(C)]
#[derive(FromBytes, FromZeroes)]
struct KernelRegion64 {
start: u64,
end: u64,
Expand All @@ -475,9 +478,8 @@ fn kernel_phys_mem(kernel_config: &Config, kernel_elf: &ElfFile) -> Vec<(u64, u6
let p_region_size = size_of::<KernelRegion64>();
let mut offset: usize = 0;
while offset < size as usize {
let p_region = unsafe {
bytes_to_struct::<KernelRegion64>(&p_region_bytes[offset..offset + p_region_size])
};
let p_region =
KernelRegion64::ref_from(&p_region_bytes[offset..offset + p_region_size]).expect("wrong size for KernelRegion64");
phys_mem.push((p_region.start, p_region.end));
offset += p_region_size;
}
Expand Down Expand Up @@ -2597,9 +2599,9 @@ fn main() -> Result<(), String> {
is_device: ut.is_device as u64,
})
.collect();
let mut untyped_info_data: Vec<u8> = Vec::from(unsafe { struct_to_bytes(&untyped_info_header) });
let mut untyped_info_data: Vec<u8> = Vec::from(untyped_info_header.as_bytes());
for o in &untyped_info_object_data {
untyped_info_data.extend(unsafe { struct_to_bytes(o) });
untyped_info_data.extend(o.as_bytes());
}
monitor_elf.write_symbol(monitor_config.untyped_info_symbol_name, &untyped_info_data)?;

Expand Down
20 changes: 0 additions & 20 deletions tool/microkit/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,26 +149,6 @@ pub fn json_str_as_bool(json: &serde_json::Value, field: &'static str) -> Result
}
}

/// Convert a struct into raw bytes in order to be written to
/// disk or some other format.
#[allow(clippy::missing_safety_doc)]
pub unsafe fn struct_to_bytes<T: Sized>(p: &T) -> &[u8] {
::core::slice::from_raw_parts(
(p as *const T) as *const u8,
::core::mem::size_of::<T>(),
)
}

#[allow(clippy::missing_safety_doc)]
pub unsafe fn bytes_to_struct<T>(bytes: &[u8]) -> &T {
let (prefix, body, suffix) = unsafe { bytes.align_to::<T>() };
assert!(prefix.is_empty());
assert!(body.len() == 1);
assert!(suffix.is_empty());

&body[0]
}

#[cfg(test)]
mod tests {
// Note this useful idiom: importing names from outer (for mod tests) scope.
Expand Down

0 comments on commit 382c293

Please sign in to comment.