diff --git a/lib/propolis/src/util/aspace.rs b/lib/propolis/src/util/aspace.rs index 6f7b32970..9f63d3ed7 100644 --- a/lib/propolis/src/util/aspace.rs +++ b/lib/propolis/src/util/aspace.rs @@ -171,6 +171,8 @@ impl ASpace { } } +// Compute the end of the inclusive range beginning at `start` and covering +// `len` address space. fn safe_end(start: usize, len: usize) -> Option { if len == 0 { None @@ -181,6 +183,25 @@ fn safe_end(start: usize, len: usize) -> Option { } } +#[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; diff --git a/lib/propolis/src/vmm/mem.rs b/lib/propolis/src/vmm/mem.rs index 37d671f59..fbce4b3da 100644 --- a/lib/propolis/src/vmm/mem.rs +++ b/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` 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()); + } }