From 9944b029bf898308c4dc41116fdc571816cbb0e5 Mon Sep 17 00:00:00 2001 From: iximeow Date: Thu, 9 Apr 2026 19:39:25 +0000 Subject: [PATCH 1/2] Do not overflow when calculating region end in lookup This fixes a relatively boring bug I noticed in following what a particularly evil NVMe PRP could cause us to do: if a guest offers a PRP referencing the last page (i.e. `u64::MAX & !0xfff`) and we try to get a region covering that page, the addition in `region_mappings` could overflow. Yet another win for panic-on-overflow, I wouldn't have noticed this in a release build. I say this is "boring" because there won't be memory mapped up at the end of u64 space, so to be "exciting" there would have to be a very large region being looked up. Something like a length of `u64::MAX - 0x1000` where the overflow lands in the same region and we pass `addr + rlen < end`. Anything else and we'll fail the check and return `None`, just later than we really should have. --- lib/propolis/src/util/aspace.rs | 21 ++++++++++++++++++ lib/propolis/src/vmm/mem.rs | 38 ++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 1 deletion(-) 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..56fad6ff1 100644 --- a/lib/propolis/src/vmm/mem.rs +++ b/lib/propolis/src/vmm/mem.rs @@ -1167,7 +1167,11 @@ 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) { if addr + rlen < end { return None; @@ -1437,4 +1441,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()); + } } From ecbe6d91c2b867eefd98b104fd3be50cece06973 Mon Sep 17 00:00:00 2001 From: iximeow Date: Thu, 9 Apr 2026 21:13:10 +0000 Subject: [PATCH 2/2] better words --- lib/propolis/src/vmm/mem.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/propolis/src/vmm/mem.rs b/lib/propolis/src/vmm/mem.rs index 56fad6ff1..fbce4b3da 100644 --- a/lib/propolis/src/vmm/mem.rs +++ b/lib/propolis/src/vmm/mem.rs @@ -1173,6 +1173,8 @@ impl MemCtx { 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; }