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 <[email protected]>
seL4 · Aug 19, 2024 · 5ccd008 · 5ccd008
1 parent 306109b
commit 5ccd008
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 52 deletions.
diff --git 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
@@ -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<SysMap, String> {
         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<ProtectionDomain, String> {
         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<VirtualMachine, String> {
@@ -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<SysMemoryRegion, String> {
         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<SystemDescription, String> {
+pub fn parse(filename: &str, xml: &str, config: &Config) -> Result<SystemDescription, String> {
     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
@@ -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
@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Copyright 2021, Breakaway Consulting Pty. Ltd.
+
+ SPDX-License-Identifier: BSD-2-Clause
+-->
+<system>
+    <memory_region name="foo" size="0x1_000" />
+    <protection_domain name="test1">
+        <program_image path="test" />
+        <!-- This number is 2^60. -->
+        <map mr="foo" vaddr="0x1000000000000000" />
+    </protection_domain>
+</system>
diff --git 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(