virtio: validate descriptor index from avail ring#2780
virtio: validate descriptor index from avail ring#2780benhillis wants to merge 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the virtio queue implementation against malicious or buggy guests by validating that descriptor indices read from the guest-provided avail ring are within the configured queue size, returning a specific error on violation.
Changes:
- Add a new
QueueError::InvalidDescriptorIndex(u16)error variant for invalid avail-ring descriptor indices. - Validate the descriptor index read from the avail ring against
queue_sizebefore using it.
benhillis
force-pushed
the
fix/virtio-descriptor-index-validation
branch
from
88b7723 to
4ca3965
Compare
The descriptor index read from the guest's available ring was used directly without checking it against queue_size. A malicious or buggy guest could supply an out-of-range index. Add an explicit bounds check with a clear error message.
A queue_size of 0 causes division-by-zero panics in get_available_descriptor_index and set_used_descriptor, which both compute modulo queue_size. Reject it early in QueueCore::new.
benhillis
force-pushed
the
fix/virtio-descriptor-index-validation
branch
from
4ca3965 to
0c6fd95
Compare
| #[error("descriptor index {0} is out of range")] | ||
| InvalidDescriptorIndex(u16), |
There was a problem hiding this comment.
The InvalidDescriptorIndex error message doesn’t include the queue size / valid range, which makes debugging guest issues harder. Consider including queue_size in the variant (e.g., store both the index and queue_size) and mention the valid range in the error text.
| mem: GuestMemory, | ||
| params: QueueParams, | ||
| ) -> Result<Self, QueueError> { | ||
| if params.size == 0 { |
There was a problem hiding this comment.
This new params.size == 0 check is redundant with QueueCoreGetWork::new’s is_power_of_two() validation (which already rejects 0), and it only checks for 0 (not other invalid sizes). To avoid inconsistent validation if SplitQueueGetWork::new is ever called directly, either remove this check (rely on the outer validation) or validate the full invariant here (non-zero power-of-two).
| if params.size == 0 { | |
| if !params.size.is_power_of_two() { |
1 participant
The descriptor index read from the guest's available ring was used directly without checking it against queue_size. A malicious or buggy guest could supply an out-of-range index. Add an explicit bounds check with a clear error message.