vm_memory: Add a method to check if a guest address range is valid

The current crosvm implementation is using
`GuestMemory::checked_offset()` for the following two cases:
(1) To get a valid `GuestAddress` representing `addr + offset`.
(2) Guarantee that the memory range `[addr, addr+offset]` is valid.

However, using `checked_offset()` for the case (2) is wrong because the
method only checks if the end address `addr+offset` is valid and doesn't
check if an invalid region exists between `addr` and `addr+offset`.

So, this CL adds `GuestMemory::is_valid_range()` to replace the (2)
cases.

BUG=b:251753217
TEST=cargo test in vm_memory
TEST=start a vm with virtio devices

Change-Id: I21bce5d1c60acdff79284cdad963849a6e19e19c
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3945520
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: David Stevens <stevensd@chromium.org>
Reviewed-by: Noah Gold <nkgold@google.com>
Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>

devices/src/virtio/iommu/ipc_memory_mapper.rs

@@ -205,7 +205,7 @@ impl ExportedRegion {
             .export(iova, size)
             .context("failed to export")?;
         for r in &regions {
-            if !mem.address_in_range(r.gpa) || mem.checked_offset(r.gpa, r.len).is_none() {
+            if !mem.is_valid_range(r.gpa, r.len) {
                 bail!("region not in memory range");
             }
         }

devices/src/virtio/queue.rs

@@ -210,11 +210,12 @@ impl DescriptorChain {
 
     fn is_valid(&self) -> bool {
         if self.len > 0 {
-            if self.regions.iter().any(|r| {
-                self.mem
-                    .checked_offset(r.gpa, r.len as u64 - 1u64)
-                    .is_none()
-            }) {
+            // Each region in `self.regions` must be a contiguous range in `self.mem`.
+            if !self
+                .regions
+                .iter()
+                .all(|r| self.mem.is_valid_range(r.gpa, r.len as u64))
+            {
                 return false;
             }
         }

vm_memory/src/guest_memory.rs

@@ -322,7 +322,10 @@ impl GuestMemory {
             .any(|region| region.start() < end && start < region.end())
     }
 
-    /// Returns the address plus the offset if it is in range.
+    /// Returns an address `addr + offset` if it's in range.
+    ///
+    /// This function doesn't care whether a region `[addr, addr + offset)` is in range or not. To
+    /// guarantee it's a valid range, use `is_valid_range()` instead.
     pub fn checked_offset(&self, addr: GuestAddress, offset: u64) -> Option<GuestAddress> {
         addr.checked_add(offset).and_then(|a| {
             if self.address_in_range(a) {
@@ -333,6 +336,24 @@ impl GuestMemory {
         })
     }
 
+    /// Returns true if the given range `[start, start + length)` is a valid contiguous memory
+    /// range available to the guest and it's backed by a single underlying memory region.
+    pub fn is_valid_range(&self, start: GuestAddress, length: u64) -> bool {
+        if length == 0 {
+            return false;
+        }
+
+        let end = if let Some(end) = start.checked_add(length - 1) {
+            end
+        } else {
+            return false;
+        };
+
+        self.regions
+            .iter()
+            .any(|region| region.start() <= start && end < region.end())
+    }
+
     /// Returns the size of the memory region in bytes.
     pub fn num_regions(&self) -> u64 {
         self.regions.len() as u64
@@ -852,7 +873,14 @@ mod tests {
     fn two_regions() {
         let start_addr1 = GuestAddress(0x0);
         let start_addr2 = GuestAddress(0x10000);
-        assert!(GuestMemory::new(&[(start_addr1, 0x10000), (start_addr2, 0x10000)]).is_ok());
+        // The memory regions are `[0x0, 0x10000)`, `[0x10000, 0x20000)`.
+        let gm = GuestMemory::new(&[(start_addr1, 0x10000), (start_addr2, 0x10000)]).unwrap();
+
+        // Although each address in `[0x0, 0x20000)` is valid, `is_valid_range()` returns false for
+        // a range that is across multiple underlying regions.
+        assert!(gm.is_valid_range(GuestAddress(0x5000), 0x5000));
+        assert!(gm.is_valid_range(GuestAddress(0x10000), 0x5000));
+        assert!(!gm.is_valid_range(GuestAddress(0x5000), 0x10000));
     }
 
     #[test]
@@ -866,7 +894,9 @@ mod tests {
     fn region_hole() {
         let start_addr1 = GuestAddress(0x0);
         let start_addr2 = GuestAddress(0x40000);
+        // The memory regions are `[0x0, 0x20000)`, `[0x40000, 0x60000)`.
         let gm = GuestMemory::new(&[(start_addr1, 0x20000), (start_addr2, 0x20000)]).unwrap();
+
         assert!(gm.address_in_range(GuestAddress(0x10000)));
         assert!(!gm.address_in_range(GuestAddress(0x30000)));
         assert!(gm.address_in_range(GuestAddress(0x50000)));
@@ -875,9 +905,24 @@ mod tests {
         assert!(gm.range_overlap(GuestAddress(0x10000), GuestAddress(0x30000)),);
         assert!(!gm.range_overlap(GuestAddress(0x30000), GuestAddress(0x40000)),);
         assert!(gm.range_overlap(GuestAddress(0x30000), GuestAddress(0x70000)),);
-        assert!(gm.checked_offset(GuestAddress(0x10000), 0x10000).is_none());
-        assert!(gm.checked_offset(GuestAddress(0x50000), 0x8000).is_some());
-        assert!(gm.checked_offset(GuestAddress(0x50000), 0x10000).is_none());
+        assert_eq!(gm.checked_offset(GuestAddress(0x10000), 0x10000), None);
+        assert_eq!(
+            gm.checked_offset(GuestAddress(0x50000), 0x8000),
+            Some(GuestAddress(0x58000))
+        );
+        assert_eq!(gm.checked_offset(GuestAddress(0x50000), 0x10000), None);
+        assert!(gm.is_valid_range(GuestAddress(0x0), 0x10000));
+        assert!(gm.is_valid_range(GuestAddress(0x0), 0x20000));
+        assert!(!gm.is_valid_range(GuestAddress(0x0), 0x20000 + 1));
+
+        // While `checked_offset(GuestAddress(0x10000), 0x40000)` succeeds because 0x50000 is a
+        // valid address, `is_valid_range(GuestAddress(0x10000), 0x40000)` returns `false`
+        // because there is a hole inside of [0x10000, 0x50000).
+        assert_eq!(
+            gm.checked_offset(GuestAddress(0x10000), 0x40000),
+            Some(GuestAddress(0x50000))
+        );
+        assert!(!gm.is_valid_range(GuestAddress(0x10000), 0x40000));
     }
 
     #[test]

x86_64/src/fdt.rs

@@ -68,9 +68,10 @@ pub fn create_fdt(
     assert!(fdt_data_size as u64 <= X86_64_FDT_MAX_SIZE);
 
     let fdt_address = GuestAddress(fdt_load_offset);
-    guest_mem
-        .checked_offset(fdt_address, fdt_data_size as u64)
-        .ok_or(Error::FdtGuestMemoryWriteError)?;
+    if !guest_mem.is_valid_range(fdt_address, fdt_data_size as u64) {
+        return Err(Error::FdtGuestMemoryWriteError);
+    }
+
     guest_mem
         .write_obj_at_addr(hdr, fdt_address)
         .map_err(|_| Error::FdtGuestMemoryWriteError)?;

x86_64/src/lib.rs

@@ -466,9 +466,10 @@ fn configure_system(
     )?;
 
     let zero_page_addr = GuestAddress(ZERO_PAGE_OFFSET);
-    guest_mem
-        .checked_offset(zero_page_addr, mem::size_of::<boot_params>() as u64)
-        .ok_or(Error::ZeroPagePastRamEnd)?;
+    if !guest_mem.is_valid_range(zero_page_addr, mem::size_of::<boot_params>() as u64) {
+        return Err(Error::ZeroPagePastRamEnd);
+    }
+
     guest_mem
         .write_obj_at_addr(params, zero_page_addr)
         .map_err(|_| Error::ZeroPageSetup)?;

x86_64/src/regs.rs

@@ -235,9 +235,13 @@ const BOOT_GDT_MAX: usize = 4;
 fn write_gdt_table(table: &[u64], guest_mem: &GuestMemory) -> Result<()> {
     let boot_gdt_addr = GuestAddress(BOOT_GDT_OFFSET);
     for (index, entry) in table.iter().enumerate() {
-        let addr = guest_mem
-            .checked_offset(boot_gdt_addr, (index * mem::size_of::<u64>()) as u64)
+        let addr = boot_gdt_addr
+            .checked_add((index * mem::size_of::<u64>()) as u64)
             .ok_or(Error::WriteGDTFailure)?;
+        if !guest_mem.is_valid_range(addr, mem::size_of::<u64>() as u64) {
+            return Err(Error::WriteGDTFailure);
+        }
+
         guest_mem
             .write_obj_at_addr(*entry, addr)
             .map_err(|_| Error::WriteGDTFailure)?;