From 2e7ffdc1b8bcf33799971198f5e31941f2ada721 Mon Sep 17 00:00:00 2001 From: wucke13 Date: Wed, 10 Jul 2024 12:06:24 +0200 Subject: [PATCH] chore: remove all unsafe 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 --- tool/microkit/Cargo.lock | 28 ++++++++++++++++++++++++++++ tool/microkit/Cargo.toml | 1 + tool/microkit/src/elf.rs | 24 +++++++++++++----------- tool/microkit/src/loader.rs | 11 ++++++++--- tool/microkit/src/main.rs | 28 +++++++++++++--------------- tool/microkit/src/util.rs | 17 ----------------- 6 files changed, 63 insertions(+), 46 deletions(-) diff --git a/tool/microkit/Cargo.lock b/tool/microkit/Cargo.lock index a3c57bbb..e3e960bb 100644 --- a/tool/microkit/Cargo.lock +++ b/tool/microkit/Cargo.lock @@ -2,6 +2,12 @@ # It is not intended for manual editing. version = 3 +[[package]] +name = "byteorder" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fd0f2584146f6f2ef48085050886acf353beff7305ebd1ae69500e27c67f64b" + [[package]] name = "itoa" version = "1.0.11" @@ -15,6 +21,7 @@ dependencies = [ "roxmltree", "serde", "serde_json", + "zerocopy", ] [[package]] @@ -94,3 +101,24 @@ name = "unicode-ident" version = "1.0.12" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b" + +[[package]] +name = "zerocopy" +version = "0.7.35" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1b9b4fd18abc82b8136838da5d50bae7bdea537c574d8dc1a34ed098d6c166f0" +dependencies = [ + "byteorder", + "zerocopy-derive", +] + +[[package]] +name = "zerocopy-derive" +version = "0.7.35" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fa4f8080344d4671fb4e831a13ad1e68092748387dfc4f55e356242fae12ce3e" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] diff --git a/tool/microkit/Cargo.toml b/tool/microkit/Cargo.toml index 590b840e..28fb5371 100644 --- a/tool/microkit/Cargo.toml +++ b/tool/microkit/Cargo.toml @@ -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"] } diff --git a/tool/microkit/src/elf.rs b/tool/microkit/src/elf.rs index fa66e705..7f273bb6 100644 --- a/tool/microkit/src/elf.rs +++ b/tool/microkit/src/elf.rs @@ -4,11 +4,12 @@ // SPDX-License-Identifier: BSD-2-Clause // -use crate::util::bytes_to_struct; use std::collections::HashMap; use std::fs; use std::path::Path; +use zerocopy::{FromBytes, FromZeroes}; + #[repr(C, packed)] struct ElfHeader32 { ident_magic: u32, @@ -34,7 +35,7 @@ struct ElfHeader32 { } #[repr(C, packed)] -#[derive(Copy, Clone)] +#[derive(Copy, Clone, FromBytes, FromZeroes)] struct ElfSymbol64 { name: u32, info: u8, @@ -45,6 +46,7 @@ struct ElfSymbol64 { } #[repr(C, packed)] +#[derive(FromBytes, FromZeroes)] struct ElfSectionHeader64 { name: u32, type_: u32, @@ -59,6 +61,7 @@ struct ElfSectionHeader64 { } #[repr(C, packed)] +#[derive(FromBytes, FromZeroes)] struct ElfProgramHeader64 { type_: u32, flags: u32, @@ -71,6 +74,7 @@ struct ElfProgramHeader64 { } #[repr(C, packed)] +#[derive(FromBytes, FromZeroes)] struct ElfHeader64 { ident_magic: u32, ident_class: u8, @@ -174,7 +178,7 @@ impl ElfFile { // Now need to read the header into a struct let hdr_bytes = &bytes[..hdr_size]; - let hdr = unsafe { bytes_to_struct::(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. @@ -197,7 +201,8 @@ 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::(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; @@ -225,7 +230,8 @@ 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::(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), @@ -265,12 +271,8 @@ impl ElfFile { let symbol_size = std::mem::size_of::(); while offset < symtab.len() { let sym_bytes = &symtab[offset..offset + symbol_size]; - let (sym_head, sym_body, sym_tail) = unsafe { sym_bytes.align_to::() }; - 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::(), 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. diff --git a/tool/microkit/src/loader.rs b/tool/microkit/src/loader.rs index 71e3a4b7..e769b531 100644 --- a/tool/microkit/src/loader.rs +++ b/tool/microkit/src/loader.rs @@ -6,12 +6,15 @@ use crate::elf::ElfFile; use crate::sel4::Config; -use crate::util::{kb, mask, mb, round_up, struct_to_bytes}; +use crate::util::{kb, mask, mb, round_up}; + use crate::MemoryRegion; use std::fs::File; use std::io::{BufWriter, Write}; use std::path::Path; +use zerocopy::AsBytes; + const PAGE_TABLE_SIZE: usize = 4096; const AARCH64_1GB_BLOCK_BITS: u64 = 30; @@ -61,6 +64,7 @@ fn check_non_overlapping(regions: &Vec<(u64, &[u8])>) { } #[repr(C)] +#[derive(AsBytes)] struct LoaderRegion64 { load_addr: u64, size: u64, @@ -69,6 +73,7 @@ struct LoaderRegion64 { } #[repr(C)] +#[derive(AsBytes)] struct LoaderHeader64 { magic: u64, flags: u64, @@ -273,13 +278,13 @@ impl<'a> Loader<'a> { .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"); diff --git a/tool/microkit/src/main.rs b/tool/microkit/src/main.rs index cfa793b7..8a98a7f8 100644 --- a/tool/microkit/src/main.rs +++ b/tool/microkit/src/main.rs @@ -28,10 +28,8 @@ use sysxml::{ parse, PlatformDescription, ProtectionDomain, SysMap, SysMapPerms, SysMemoryRegion, SystemDescription, VirtualMachine, }; -use util::{ - bytes_to_struct, comma_sep_u64, comma_sep_usize, json_str_as_bool, json_str_as_u64, - struct_to_bytes, -}; +use util::{comma_sep_u64, comma_sep_usize, json_str_as_bool, json_str_as_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"; @@ -84,6 +82,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, @@ -92,6 +91,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, @@ -488,6 +488,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, @@ -506,11 +507,9 @@ fn kernel_device_addrs(kernel_config: &Config, kernel_elf: &ElfFile) -> Vec let kernel_frame_size = size_of::(); let mut offset: usize = 0; while offset < size as usize { - let kernel_frame = unsafe { - bytes_to_struct::( - &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); } @@ -522,6 +521,7 @@ fn kernel_device_addrs(kernel_config: &Config, kernel_elf: &ElfFile) -> Vec // Corresponds to p_region_t in the kernel #[repr(C)] +#[derive(FromBytes, FromZeroes)] struct KernelRegion64 { start: u64, end: u64, @@ -537,9 +537,8 @@ fn kernel_phys_mem(kernel_config: &Config, kernel_elf: &ElfFile) -> Vec<(u64, u6 let p_region_size = size_of::(); let mut offset: usize = 0; while offset < size as usize { - let p_region = unsafe { - bytes_to_struct::(&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; } @@ -3032,10 +3031,9 @@ fn main() -> Result<(), String> { is_device: ut.is_device as u64, }) .collect(); - let mut untyped_info_data: Vec = - Vec::from(unsafe { struct_to_bytes(&untyped_info_header) }); + let mut untyped_info_data: Vec = 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)?; diff --git a/tool/microkit/src/util.rs b/tool/microkit/src/util.rs index 4258a5d1..9c3538c5 100644 --- a/tool/microkit/src/util.rs +++ b/tool/microkit/src/util.rs @@ -156,23 +156,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(p: &T) -> &[u8] { - ::core::slice::from_raw_parts((p as *const T) as *const u8, ::core::mem::size_of::()) -} - -#[allow(clippy::missing_safety_doc)] -pub unsafe fn bytes_to_struct(bytes: &[u8]) -> &T { - let (prefix, body, suffix) = unsafe { bytes.align_to::() }; - 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.