Skip to content

Commit

Permalink
fix: Ensure the entire queue is contained in memory
Browse files Browse the repository at this point in the history
The previous is_valid check only checked whether the last byte of
avail_ring, descriptor_table or used_ring are valid guest physical
addresses. However, since guest memory is not neccessarily contiguous
(for example, the MMIO gap leaves a hole), this allowed a guest to set
up a queue structure that "starts" inside the MMIO gap and ends outside
of it. This would pass validations, and then later potentially crash
firecracker when a device tried to consume descriptors from it. This
happens because while all actual memory reads and writes perform
additional bounds checks, some code in queue.rs used .unwrap() under the
assumption that all such bounds check would always pass (which they
would, if is_valid() were implemented correctly). This would then result
in a panic!().

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
  • Loading branch information
roypat committed Aug 22, 2023
1 parent 4c6dc0f commit 7909c5e
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 12 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Expand Up @@ -33,6 +33,9 @@
and the FXSR bit (CPUID.80000001h:EDX[24]).
- Fixed the T2A CPU template to set the RstrFpErrPtrs bit
(CPUID.80000008h:EBX[2]).
- Fixed a bug where Firecracker would crash during boot if a guest set up a virtio
queue that partially overlapped with the MMIO gap. Now Firecracker instead
correctly refuses to activate the corresponding virtio device.

## [1.4.0]

Expand Down
16 changes: 4 additions & 12 deletions src/vmm/src/devices/virtio/queue.rs
Expand Up @@ -263,30 +263,22 @@ impl Queue {
} else if used_ring.raw_value() & 0x3 != 0 {
error!("virtio queue used ring breaks alignment constraints");
false
} else if desc_table
.checked_add(desc_table_size)
.map_or(true, |v| !mem.address_in_range(v))
{
// range check entire descriptor table to be assigned valid guest physical addresses
} else if mem.get_slice(desc_table, desc_table_size as usize).is_err() {
error!(
"virtio queue descriptor table goes out of bounds: start:0x{:08x} size:0x{:08x}",
desc_table.raw_value(),
desc_table_size
);
false
} else if avail_ring
.checked_add(avail_ring_size)
.map_or(true, |v| !mem.address_in_range(v))
{
} else if mem.get_slice(avail_ring, avail_ring_size as usize).is_err() {
error!(
"virtio queue available ring goes out of bounds: start:0x{:08x} size:0x{:08x}",
avail_ring.raw_value(),
avail_ring_size
);
false
} else if used_ring
.checked_add(used_ring_size)
.map_or(true, |v| !mem.address_in_range(v))
{
} else if mem.get_slice(used_ring, used_ring_size as usize).is_err() {
error!(
"virtio queue used ring goes out of bounds: start:0x{:08x} size:0x{:08x}",
used_ring.raw_value(),
Expand Down

0 comments on commit 7909c5e

Please sign in to comment.