Do not overflow when calculating region end in lookup

lib/propolis/src/util/aspace.rs

@@ -171,6 +171,8 @@ impl<T> ASpace<T> {
     }
 }
 
+// Compute the end of the inclusive range beginning at `start` and covering
+// `len` address space.
 fn safe_end(start: usize, len: usize) -> Option<usize> {
     if len == 0 {
         None
@@ -181,6 +183,25 @@ fn safe_end(start: usize, len: usize) -> Option<usize> {
     }
 }
 
+#[test]
+fn safe_end_bounds() {
+    // An inclusive region of size zero is nonsense.
+    assert_eq!(safe_end(0, 0), None);
+    assert_eq!(safe_end(1, 0), None);
+
+    // An inclusive region of size one can exist anywhere.
+    assert_eq!(safe_end(0, 1), Some(0));
+    assert_eq!(safe_end(16, 1), Some(16));
+    assert_eq!(safe_end(usize::MAX, 1), Some(usize::MAX));
+
+    // Given `[0, usize::MAX]` as possible addresses, the size of that set is
+    // actually `usize::MAX + 1`. This means:
+    // * there is no `len` that covers the entire possible span
+    // * start=0, len=usize::MAX is a span that ends at usize::MAX-1
+    assert_eq!(safe_end(0, usize::MAX), Some(usize::MAX - 1));
+    assert_eq!(safe_end(1, usize::MAX), Some(usize::MAX));
+}
+
 // Flatten the K/V nested tuple
 fn kv_flatten<'a, T>(i: (&'a usize, &'a (usize, T))) -> SpaceItem<'a, T> {
     let start = *i.0;

lib/propolis/src/vmm/mem.rs

@@ -1167,8 +1167,14 @@ impl MemCtx {
         len: usize,
     ) -> Option<(SubMapping<'_>, SubMapping<'_>)> {
         let start = addr.0 as usize;
-        let end = start + len;
+        let Some(end) = start.checked_add(len) else {
+            // The mappings in `self.map` do not wrap, so no mapping can match
+            // with a region wrapping at the end of the address space.
+            return None;
+        };
         if let Ok((addr, rlen, ent)) = self.map.region_at(start) {
+            // Unlike start+len before, we know `addr + rlen` cannot overflow:
+            // if it would, ASpace::register would have rejected this region.
             if addr + rlen < end {
                 return None;
             }
@@ -1437,4 +1443,36 @@ pub mod test {
         assert!(sub_write.write_bytes(&buf).is_ok());
         assert!(sub_write.read_bytes(&mut buf).is_err());
     }
+
+    // Tests above cover lookups inside one mapping, but Propolis uses memory
+    // through a `MemCtx` including an `ASpace<MapEnt>` covering all mappings
+    // for the VM.
+    #[test]
+    fn region_lookup() {
+        let hdl = VmmHdl::new_test(TEST_LEN)
+            .expect("create tempfile backed test hdl");
+        let hdl = Arc::new(hdl);
+
+        let mut phys = PhysMap::new(TEST_LEN, hdl);
+        phys.add_mem("test dram".to_string(), 0, PAGE_SIZE)
+            .expect("can add test DRAM");
+        let mem = phys.finalize();
+
+        let acc_mem = mem.access().expect("can access memory");
+
+        // We can get a readable region covering all added memory.
+        let region =
+            acc_mem.readable_region(&GuestRegion(GuestAddr(0), PAGE_SIZE));
+        assert!(region.is_some());
+
+        // But not a region extending past memory in the VM.
+        let region =
+            acc_mem.readable_region(&GuestRegion(GuestAddr(0), 2 * PAGE_SIZE));
+        assert!(region.is_none());
+
+        // And not a region that would wrap into VM memory.
+        let region = acc_mem
+            .readable_region(&GuestRegion(GuestAddr(u64::MAX), PAGE_SIZE));
+        assert!(region.is_none());
+    }
 }