From 5ccd00807fe81c4a4dcafaa198d2dc3df75ec3bc Mon Sep 17 00:00:00 2001 From: Ivan Velickovic Date: Sun, 18 Aug 2024 19:05:39 +1000 Subject: [PATCH] Add more error checking for memory mappings This patch checks that a memory mapping is not at a virtual address that is greater than the maximum virtual address. Signed-off-by: Ivan Velickovic --- tool/microkit/src/main.rs | 15 ++--- tool/microkit/src/sdf.rs | 69 +++++++++----------- tool/microkit/src/sel4.rs | 21 ++++++ tool/microkit/tests/sdf/sys_map_too_high.xml | 14 ++++ tool/microkit/tests/test.rs | 13 ++-- 5 files changed, 80 insertions(+), 52 deletions(-) create mode 100644 tool/microkit/tests/sdf/sys_map_too_high.xml diff --git a/tool/microkit/src/main.rs b/tool/microkit/src/main.rs index 3424ca3ed..f1e8a7d61 100644 --- a/tool/microkit/src/main.rs +++ b/tool/microkit/src/main.rs @@ -14,8 +14,8 @@ use microkit_tool::{ UntypedObject, MAX_PDS, PD_MAX_NAME_LENGTH, }; use sdf::{ - parse, PlatformDescription, ProtectionDomain, SysMap, SysMapPerms, SysMemoryRegion, - SystemDescription, VirtualMachine, + parse, ProtectionDomain, SysMap, SysMapPerms, SysMemoryRegion, SystemDescription, + VirtualMachine, }; use sel4::{ default_vm_attr, Aarch64Regs, Arch, ArmVmAttributes, BootInfo, Config, Invocation, @@ -1362,11 +1362,9 @@ fn build_system( text_pos: None, }; - let stack_vaddr = config.user_top(); - let stack_map = SysMap { mr: stack_mr.name.clone(), - vaddr: stack_vaddr - pd.stack_size, + vaddr: config.pd_stack_top() - pd.stack_size, perms: SysMapPerms::Read as u8 | SysMapPerms::Write as u8, cached: true, text_pos: None, @@ -2742,13 +2740,13 @@ fn build_system( let regs = match config.arch { Arch::Aarch64 => Aarch64Regs { pc: pd_elf_files[pd_idx].entry, - sp: config.user_top(), + sp: config.pd_stack_top(), ..Default::default() } .field_names(), Arch::Riscv64 => Riscv64Regs { pc: pd_elf_files[pd_idx].entry, - sp: config.user_top(), + sp: config.pd_stack_top(), ..Default::default() } .field_names(), @@ -3328,8 +3326,7 @@ fn main() -> Result<(), String> { "Microkit tool has various assumptions about the word size being 64-bits." ); - let plat_desc = PlatformDescription::new(&kernel_config); - let system = match parse(args.system, &xml, &plat_desc) { + let system = match parse(args.system, &xml, &kernel_config) { Ok(system) => system, Err(err) => { eprintln!("{err}"); diff --git a/tool/microkit/src/sdf.rs b/tool/microkit/src/sdf.rs index 6b5337d51..4df969851 100644 --- a/tool/microkit/src/sdf.rs +++ b/tool/microkit/src/sdf.rs @@ -4,7 +4,7 @@ // SPDX-License-Identifier: BSD-2-Clause // -use crate::sel4::{Arch, Config, IrqTrigger, PageSize}; +use crate::sel4::{Config, IrqTrigger, PageSize}; use crate::util::str_to_bool; use crate::MAX_PDS; use std::path::{Path, PathBuf}; @@ -74,23 +74,6 @@ fn loc_string(xml_sdf: &XmlSystemDescription, pos: roxmltree::TextPos) -> String format!("{}:{}:{}", xml_sdf.filename, pos.row, pos.col) } -/// There are some platform-specific properties that must be known when parsing the -/// SDF for error-checking and validation, these go in this struct. -pub struct PlatformDescription { - /// Note that we have the invariant that page sizes are be ordered by size - page_sizes: [u64; 2], -} - -impl PlatformDescription { - pub const fn new(kernel_config: &Config) -> PlatformDescription { - let page_sizes = match kernel_config.arch { - Arch::Aarch64 | Arch::Riscv64 => [0x1000, 0x200_000], - }; - - PlatformDescription { page_sizes } - } -} - #[repr(u8)] pub enum SysMapPerms { Read = 1, @@ -213,6 +196,7 @@ impl SysMap { xml_sdf: &XmlSystemDescription, node: &roxmltree::Node, allow_setvar: bool, + max_vaddr: u64, ) -> Result { let mut attrs = vec!["mr", "vaddr", "perms", "cached"]; if allow_setvar { @@ -222,6 +206,15 @@ impl SysMap { let mr = checked_lookup(xml_sdf, node, "mr")?.to_string(); let vaddr = sdf_parse_number(checked_lookup(xml_sdf, node, "vaddr")?, node)?; + + if vaddr >= max_vaddr { + return Err(value_error( + xml_sdf, + node, + format!("vaddr (0x{:x}) must be less than 0x{:x}", vaddr, max_vaddr), + )); + } + let perms = if let Some(xml_perms) = node.attribute("perms") { match SysMapPerms::from_str(xml_perms) { Ok(parsed_perms) => parsed_perms, @@ -279,9 +272,9 @@ impl ProtectionDomain { } fn from_xml( + config: &Config, xml_sdf: &XmlSystemDescription, node: &roxmltree::Node, - plat_desc: &PlatformDescription, is_child: bool, ) -> Result { let mut attrs = vec![ @@ -379,13 +372,13 @@ impl ProtectionDomain { )); } - if stack_size % plat_desc.page_sizes[0] != 0 { + if stack_size % config.page_sizes()[0] != 0 { return Err(value_error( xml_sdf, node, format!( "stack size must be aligned to the smallest page size, {} bytes", - plat_desc.page_sizes[0] + config.page_sizes()[0] ), )); } @@ -433,7 +426,8 @@ impl ProtectionDomain { program_image = Some(Path::new(program_image_path).to_path_buf()); } "map" => { - let map = SysMap::from_xml(xml_sdf, &child, true)?; + let map_max_vaddr = config.pd_map_max_vaddr(stack_size); + let map = SysMap::from_xml(xml_sdf, &child, true, map_max_vaddr)?; if let Some(setvar_vaddr) = child.attribute("setvar_vaddr") { // Check that the symbol does not already exist @@ -520,9 +514,9 @@ impl ProtectionDomain { vaddr: None, }) } - "protection_domain" => child_pds.push(ProtectionDomain::from_xml( - xml_sdf, &child, plat_desc, true, - )?), + "protection_domain" => { + child_pds.push(ProtectionDomain::from_xml(config, xml_sdf, &child, true)?) + } "virtual_machine" => { if virtual_machine.is_some() { return Err(value_error( @@ -532,7 +526,7 @@ impl ProtectionDomain { )); } - virtual_machine = Some(VirtualMachine::from_xml(xml_sdf, &child)?); + virtual_machine = Some(VirtualMachine::from_xml(config, xml_sdf, &child)?); } _ => { let pos = xml_sdf.doc.text_pos_at(child.range().start); @@ -580,6 +574,7 @@ impl ProtectionDomain { impl VirtualMachine { fn from_xml( + config: &Config, xml_sdf: &XmlSystemDescription, node: &roxmltree::Node, ) -> Result { @@ -654,7 +649,7 @@ impl VirtualMachine { "map" => { // Virtual machines do not have program images and so we do not allow // setvar_vaddr on SysMap - let map = SysMap::from_xml(xml_sdf, &child, false)?; + let map = SysMap::from_xml(xml_sdf, &child, false, config.vm_map_max_vaddr())?; maps.push(map); } _ => { @@ -690,9 +685,9 @@ impl VirtualMachine { impl SysMemoryRegion { fn from_xml( + config: &Config, xml_sdf: &XmlSystemDescription, node: &roxmltree::Node, - plat_desc: &PlatformDescription, ) -> Result { check_attributes(xml_sdf, node, &["name", "size", "page_size", "phys_addr"])?; @@ -703,10 +698,10 @@ impl SysMemoryRegion { sdf_parse_number(xml_page_size, node)? } else { // Default to the minimum page size - plat_desc.page_sizes[0] + config.page_sizes()[0] }; - let page_size_valid = plat_desc.page_sizes.contains(&page_size); + let page_size_valid = config.page_sizes().contains(&page_size); if !page_size_valid { return Err(value_error( xml_sdf, @@ -999,11 +994,7 @@ fn pd_flatten( Ok(all_pds) } -pub fn parse( - filename: &str, - xml: &str, - plat_desc: &PlatformDescription, -) -> Result { +pub fn parse(filename: &str, xml: &str, config: &Config) -> Result { let doc = match roxmltree::Document::parse(xml) { Ok(doc) => doc, Err(err) => return Err(format!("Could not parse '{}': {}", filename, err)), @@ -1039,11 +1030,11 @@ pub fn parse( let child_name = child.tag_name().name(); match child_name { - "protection_domain" => root_pds.push(ProtectionDomain::from_xml( - &xml_sdf, &child, plat_desc, false, - )?), + "protection_domain" => { + root_pds.push(ProtectionDomain::from_xml(config, &xml_sdf, &child, false)?) + } "channel" => channel_nodes.push(child), - "memory_region" => mrs.push(SysMemoryRegion::from_xml(&xml_sdf, &child, plat_desc)?), + "memory_region" => mrs.push(SysMemoryRegion::from_xml(config, &xml_sdf, &child)?), _ => { let pos = xml_sdf.doc.text_pos_at(child.range().start); return Err(format!( diff --git a/tool/microkit/src/sel4.rs b/tool/microkit/src/sel4.rs index 1c36a3a4f..228674bdd 100644 --- a/tool/microkit/src/sel4.rs +++ b/tool/microkit/src/sel4.rs @@ -68,6 +68,27 @@ impl Config { Arch::Riscv64 => 0x0000003ffffff000, } } + + pub fn page_sizes(&self) -> [u64; 2] { + match self.arch { + Arch::Aarch64 | Arch::Riscv64 => [0x1000, 0x200_000], + } + } + + pub fn pd_stack_top(&self) -> u64 { + self.user_top() + } + + pub fn pd_map_max_vaddr(&self, stack_size: u64) -> u64 { + // This function depends on this invariant + assert!(self.pd_stack_top() == self.user_top()); + + self.user_top() - stack_size + } + + pub fn vm_map_max_vaddr(&self) -> u64 { + self.user_top() + } } pub enum Arch { diff --git a/tool/microkit/tests/sdf/sys_map_too_high.xml b/tool/microkit/tests/sdf/sys_map_too_high.xml new file mode 100644 index 000000000..36bae79b3 --- /dev/null +++ b/tool/microkit/tests/sdf/sys_map_too_high.xml @@ -0,0 +1,14 @@ + + + + + + + + + + diff --git a/tool/microkit/tests/test.rs b/tool/microkit/tests/test.rs index 2205b0dbd..29fed79f5 100644 --- a/tool/microkit/tests/test.rs +++ b/tool/microkit/tests/test.rs @@ -22,15 +22,12 @@ const DEFAULT_KERNEL_CONFIG: sel4::Config = sel4::Config { riscv_pt_levels: None, }; -const DEFAULT_PLAT_DESC: sdf::PlatformDescription = - sdf::PlatformDescription::new(&DEFAULT_KERNEL_CONFIG); - fn check_error(test_name: &str, expected_err: &str) { let mut path = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")); path.push("tests/sdf/"); path.push(test_name); let xml = std::fs::read_to_string(path).unwrap(); - let parse_err = sdf::parse(test_name, &xml, &DEFAULT_PLAT_DESC).unwrap_err(); + let parse_err = sdf::parse(test_name, &xml, &DEFAULT_KERNEL_CONFIG).unwrap_err(); if !parse_err.starts_with(expected_err) { eprintln!( @@ -408,6 +405,14 @@ mod system { ) } + #[test] + fn test_map_too_high() { + check_error( + "sys_map_too_high.xml", + "Error: vaddr (0x1000000000000000) must be less than 0xfffffff000 on element 'map'", + ) + } + #[test] fn test_too_many_pds() { check_error(