block: fix off-by-one error in addr validation

The block device validated addresses from virtio descriptors before
trying to execute requests. In this validation, the `checked_offset`
function of guest memory was used to determine if the slice defined
by the sum of the address and length of the virtio descriptor was
within the guest memory bounds. However, this sum is greater than
the last valid offset, `addr + len - 1`, by 1. This made the block
device mark descriptors with slices at the very end of a region as
invalid, making the last byte of a memory region unusable by the
block device.

Changed the request parsing method to subtract one from the sum.

Also adde dregression tests for this case.

Signed-off-by: George Pisaltu <[email protected]>
Signed-off-by: alindima <[email protected]>

Author: georgepisaltu

src/devices/src/virtio/block/device.rs

@@ -230,9 +230,10 @@ impl Block {
                             e.status()
                         }
                     };
-                    // We use unwrap because the request parsing process already checked that the
-                    // status_addr was valid.
-                    mem.write_obj(status, request.status_addr).unwrap();
+
+                    if let Err(e) = mem.write_obj(status, request.status_addr) {
+                        error!("Failed to write virtio block status: {:?}", e)
+                    }
                 }
                 Err(e) => {
                     error!("Failed to parse available descriptor chain: {:?}", e);
@@ -584,6 +585,39 @@ pub(crate) mod tests {
         assert_eq!(vq.used.ring[0].get().len, 0);
     }
 
+    #[test]
+    fn test_addr_out_of_bounds() {
+        let mut block = default_block();
+        // Default mem size is 0x10000
+        let mem = default_mem();
+        let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
+        block.set_queue(0, vq.create_queue());
+        block.activate(mem.clone()).unwrap();
+        initialize_virtqueue(&vq);
+        vq.dtable[1].set(0xff00, 0x1000, VIRTQ_DESC_F_NEXT | VIRTQ_DESC_F_WRITE, 2);
+
+        let request_type_addr = GuestAddress(vq.dtable[0].addr.get());
+
+        // Mark the next available descriptor.
+        vq.avail.idx.set(1);
+        // Read.
+        {
+            vq.used.idx.set(0);
+
+            mem.write_obj::<u32>(VIRTIO_BLK_T_IN, request_type_addr)
+                .unwrap();
+            vq.dtable[1]
+                .flags
+                .set(VIRTQ_DESC_F_NEXT | VIRTQ_DESC_F_WRITE);
+
+            check_metric_after_block!(
+                &METRICS.block.execute_fails,
+                1,
+                invoke_handler_for_queue_event(&mut block)
+            );
+        }
+    }
+
     #[test]
     fn test_request_execute_failures() {
         let mut block = default_block();
@@ -671,6 +705,38 @@ pub(crate) mod tests {
             VIRTIO_BLK_S_UNSUPP
         );
     }
+    #[test]
+    fn test_end_of_region() {
+        let mut block = default_block();
+        let mem = default_mem();
+        let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
+        block.set_queue(0, vq.create_queue());
+        block.activate(mem.clone()).unwrap();
+        initialize_virtqueue(&vq);
+        vq.dtable[1].set(0xf000, 0x1000, VIRTQ_DESC_F_NEXT | VIRTQ_DESC_F_WRITE, 2);
+
+        let request_type_addr = GuestAddress(vq.dtable[0].addr.get());
+        let status_addr = GuestAddress(vq.dtable[2].addr.get());
+
+        vq.used.idx.set(0);
+
+        mem.write_obj::<u32>(VIRTIO_BLK_T_IN, request_type_addr)
+            .unwrap();
+        vq.dtable[1]
+            .flags
+            .set(VIRTQ_DESC_F_NEXT | VIRTQ_DESC_F_WRITE);
+
+        check_metric_after_block!(
+            &METRICS.block.read_count,
+            1,
+            invoke_handler_for_queue_event(&mut block)
+        );
+
+        assert_eq!(vq.used.idx.get(), 1);
+        assert_eq!(vq.used.ring[0].get().id, 0);
+        assert_eq!(vq.used.ring[0].get().len, vq.dtable[1].len.get());
+        assert_eq!(mem.read_obj::<u32>(status_addr).unwrap(), VIRTIO_BLK_S_OK);
+    }
 
     #[test]
     fn test_read_write() {

src/devices/src/virtio/block/request.rs

@@ -160,7 +160,7 @@ impl Request {
 
             // Check that the address of the data descriptor is valid in guest memory.
             let _ = mem
-                .checked_offset(data_desc.addr, data_desc.len as usize)
+                .checked_offset(data_desc.addr, (data_desc.len - 1) as usize)
                 .ok_or(Error::GuestMemory(GuestMemoryError::InvalidGuestAddress(
                     data_desc.addr,
                 )))?;
@@ -181,7 +181,7 @@ impl Request {
         // Check that the address of the status descriptor is valid in guest memory.
         // We will write an u32 status here after executing the request.
         let _ = mem
-            .checked_offset(status_desc.addr, mem::size_of::<u32>())
+            .checked_offset(status_desc.addr, (mem::size_of::<u32>() - 1) as usize)
             .ok_or(Error::GuestMemory(GuestMemoryError::InvalidGuestAddress(
                 status_desc.addr,
             )))?;