Repository: tock/tock (language: rust)

Issue:
Defensive programming in MPU driver API
Hello,
While looking at the cortex-M MPU drivers, I noticed some potential integer overflow errors that could cause panics and/or quietly break process memory isolation. I'm fairly confident these APIs aren't directly exposed to userspace processes, so its not an immediate issue, but considering that this is a security-critical API of a security-focused OS, it might be worthwhile being a bit paranoid.

An example: 
The MPU API is parameterized by `MIN_REGION_SIZE`:
https://github.com/tock/tock/blob/b73d01084367b23d2abed4159cd570192bf454fb/arch/cortex-m/src/mpu.rs#L373-L374
which is then used in the function `allocate_region`:
https://github.com/tock/tock/blob/b73d01084367b23d2abed4159cd570192bf454fb/arch/cortex-m/src/mpu.rs#L443
This code will crash the kernel if `MIN_REGION_SIZE` = 0.


A more interesting example:
The `CortexMRegion::new()` function computes the size of the region by finding the log base two from the provided `region_size` and subtracting 1: https://github.com/tock/tock/blob/b73d01084367b23d2abed4159cd570192bf454fb/arch/cortex-m/src/mpu.rs#L308
However, `log_base_two` is implemented such that `log_base_two(0)` = 0. This means that if you pass in a `region_size` of 0, there will be an integer underflow, resulting in a size of `u32::MAX`, which when written to the 5-bit size field on the MPU will be a size of `0b11111`, i.e., allowing access of the entire address space. 

It probably makes sense to add either some comments describing the safety conditions for these APIs, or some checks for well-formed inputs. If it makes sense to add either some comments or checks, I'd be happy to file a pr for it.


Discussion / hints:
Hi @enjhnsn2,

All of the above are good points. In general, the MPU (and analog mechanisms across other architectures) is integral to upholding [Tock's threat model](https://book.tockos.org/doc/threat_model/threat_model), and as such a _correct_ implementation of its drivers are crucial. In theory, this should mean that the MPU code would be particularly clear, robust, well-tested, or even formally modeled and verified---in practice, it's robustness, tests and clarity around safety invariants may be lacking. This is highlighted by issues such as #2076, #3544, #3541, #2947, #2173, #2435, ...

I think it's important to realize that these subsystems have grown organically, and it's difficult to keep track of the various (evolving) requirements and changing kernel infrastructure while not compromising clarity, or in some cases, correctness. In particular, the Cortex-M MPU implementation has fared fairly well in this regard, with very few actual bugs manifesting over the past few years. On the RISC-V front we had a more significant amount of bugs and implementation quirks; as a direct result of that, I re-designed that entire subsystem from scratch in #3597, taking into account lessons learned from the previous implementation.

I don't know whether the ARM MPU implementation is in a state that would necessitate similarly drastic action. However, we would certainly appreciate someone thinking through the various edge cases in question, writing test cases, and/or developing a formal model of it! Here's my 2ct on that issue:

- Adding unit tests would be great. Maybe these could also be useful to inform future formal proof goals. You may want to take inspiration from some of the efforts I started on the RISC-V front for this:
  https://github.com/tock/tock/blob/b73d01084367b23d2abed4159cd570192bf454fb/arch/rv32i/src/pmp.rs#L876-L1121 

  I do believe that this should not require quite as much mocking of types on ARM as it did on RISC-V: because the ARM MPU is configured through a set of MMIO registers, you can allocate a region of memory with identical size and alignment to this type over initialized memory, you can "invent" (cast) one to this type and use it in a mocked `StaticRef<` as part of the larger `MPU` struct to mock interactions with hardware. For tests that touch only on the `MpuRegion` associated type, you should not be needing to mock hardware at all.

  Maybe there is even a way to write shared test cases that work on the abstract `mpu::MPU` trait, and can then be called from both the RISC-V and ARM tests to ensure that neither implementation admits illegal, and rejects legal cases?

- Carefully and formally stating invariants throughout the code can also ease reasoning through the code. I tried to establish a pattern where all invariants are checked at the top of a method, and no real changes to hardware or stored state are done until all are verified in the RISC-V PMP implementation---it could be much more formal, but maybe that could be a source of inspiration?
  https://github.com/tock/tock/blob/b73d01084367b23d2abed4159cd570192bf454fb/arch/rv32i/src/pmp.rs#L738-L832

- Cross-checking the ARM MPU and RISC-V PMP implementations may reveal bugs, or at the very least a divergence in how the `mpu::MPU` trait constraints are understood. For example, here's an interesting test case where it's not immediately clear whether the `mpu::MPU` spec admits or forbids it:
  https://github.com/tock/tock/blob/b73d01084367b23d2abed4159cd570192bf454fb/arch/rv32i/src/pmp.rs#L1081-L1084

This is a super interesting and valuable effort, and we'd be more than happy to help you in improving this subsystem!
> However, `log_base_two` is implemented such that `log_base_two(0)` = 0. This means that if you pass in a `region_size` of 0, there will be an integer underflow, resulting in a size of `u32::MAX`, which when written to the 5-bit size field on the MPU will be a size of `0b11111`, i.e., allowing access of the entire address space.

Apart from my high-level comments, this is definitely a bug and we should return immediately with an error for that parameter -- there's not a reasonable way we could support it.
> I don't know whether the ARM MPU implementation is in a state that would necessitate similarly drastic action. However, we would certainly appreciate someone thinking through the various edge cases in question, writing test cases, and/or developing a formal model of it! 

From what I've seen of the implementation, something like a rewrite would not be a good use of time. I think formally modeling the Cortex-M MPU and doing some testing will be enough to provide strong correctness guarantees for the MPU code.  

> Cross-checking the ARM MPU and RISC-V PMP implementations may reveal bugs, or at the very least a divergence in how the mpu::MPU trait constraints are understood

Ya, I agree that differentially fuzzing the two architecture's MPU drivers could be quite valuable. Depending on time constraints, me or @Samir-Rashid (who is looking at the RiscV MPU implementation) may have the cycles to work on this at some point.

> Apart from my high-level comments, this is definitely a bug and we should return immediately with an error for that parameter -- there's not a reasonable way we could support it.

Sounds good. I can make that change, but the `CortexMRegion::new(...) -> CortexMRegion` is currently infallible, so this will require changing the type signature to return an `Option<CortexMRegion>`. 



> However, `log_base_two` is implemented such that `log_base_two(0)` = 0. This means that if you pass in a `region_size` of 0, there will be an integer underflow, resulting in a size of `u32::MAX`, which when written to the 5-bit size field on the MPU will be a size of `0b11111`, i.e., allowing access of the entire address space.

I assume this would panic first?


In any case, I would be very interested in seeing what this code looks like in panic-less rust.
> I assume this would panic first?

I made sure to test this, and under Tock's current debug/release profiles, integer underflows panic in debug mode, but not in release mode (the default behavior in Rust).


Fix patch:
diff --git a/arch/cortex-m/src/mpu.rs b/arch/cortex-m/src/mpu.rs
index e1cfaede1a..2722fbb5b6 100644
--- a/arch/cortex-m/src/mpu.rs
+++ b/arch/cortex-m/src/mpu.rs
@@ -17,6 +17,10 @@ use kernel::utilities::registers::interfaces::{Readable, Writeable};
 use kernel::utilities::registers::{register_bitfields, FieldValue, ReadOnly, ReadWrite};
 use kernel::utilities::StaticRef;
 
+/// Smallest allowable MPU region across all CortexM cores
+/// Individual cores may have bigger min sizes, but never lower than 32
+const CORTEXM_MIN_REGION_SIZE: usize = 32;
+
 /// MPU Registers for the Cortex-M3, Cortex-M4 and Cortex-M7 families
 /// Described in section 4.5 of
 /// <http://infocenter.arm.com/help/topic/com.arm.doc.dui0553a/DUI0553A_cortex_m4_dgug.pdf>
@@ -275,7 +279,13 @@ impl CortexMRegion {
         region_num: usize,
         subregions: Option<(usize, usize)>,
         permissions: mpu::Permissions,
-    ) -> CortexMRegion {
+    ) -> Option<CortexMRegion> {
+        // Logical size must be above minimum size for cortexM MPU regions and
+        // and less than the size of the underlying physical region
+        if logical_size < CORTEXM_MIN_REGION_SIZE || region_size < logical_size {
+            return None;
+        }
+
         // Determine access and execute permissions
         let (access, execute) = match permissions {
             mpu::Permissions::ReadWriteExecute => (
@@ -325,11 +335,11 @@ impl CortexMRegion {
             attributes += RegionAttributes::SRD.val(mask as u32);
         }
 
-        CortexMRegion {
+        Some(CortexMRegion {
             location: Some((logical_start, logical_size)),
             base_address,
             attributes,
-        }
+        })
     }
 
     fn empty(region_num: usize) -> CortexMRegion {
@@ -537,7 +547,7 @@ impl<const NUM_REGIONS: usize, const MIN_REGION_SIZE: usize> mpu::MPU
             region_num,
             subregions,
             permissions,
-        );
+        )?;
 
         config.regions[region_num] = region;
         config.is_dirty.set(true);
@@ -676,7 +686,7 @@ impl<const NUM_REGIONS: usize, const MIN_REGION_SIZE: usize> mpu::MPU
             0,
             Some((0, num_enabled_subregions0 - 1)),
             permissions,
-        );
+        )?;
 
         // We cannot have a completely unused MPU region
         let region1 = if num_enabled_subregions1 == 0 {
@@ -690,7 +700,7 @@ impl<const NUM_REGIONS: usize, const MIN_REGION_SIZE: usize> mpu::MPU
                 1,
                 Some((0, num_enabled_subregions1 - 1)),
                 permissions,
-            )
+            )?
         };
 
         config.regions[0] = region0;
@@ -750,7 +760,8 @@ impl<const NUM_REGIONS: usize, const MIN_REGION_SIZE: usize> mpu::MPU
             0,
             Some((0, num_enabled_subregions0 - 1)),
             permissions,
-        );
+        )
+        .ok_or(())?;
 
         let region1 = if num_enabled_subregions1 == 0 {
             CortexMRegion::empty(1)
@@ -764,6 +775,7 @@ impl<const NUM_REGIONS: usize, const MIN_REGION_SIZE: usize> mpu::MPU
                 Some((0, num_enabled_subregions1 - 1)),
                 permissions,
             )
+            .ok_or(())?
         };
 
         config.regions[0] = region0;


Test patch: