virtio: validate indirect descriptor table size#3137
virtio: validate indirect descriptor table size#3137benhillis wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the virtio queue descriptor-chain parsing by validating the guest-provided indirect descriptor table byte length, preventing zero-length indirect tables and misaligned lengths that previously resulted in silent truncation.
Changes:
- Add a new
QueueError::InvalidIndirectSize(u32)error variant. - Validate indirect descriptor table byte length is non-zero and a multiple of the descriptor size before mapping/parsing it.
The indirect descriptor table length from the guest was not validated to be a non-zero multiple of the descriptor size (16 bytes). A zero length would produce an empty indirect table, and a misaligned length would cause the last partial descriptor to be silently truncated via integer division. Add an explicit check that rejects zero or misaligned indirect table byte lengths with a new InvalidIndirectSize error variant.
benhillis
force-pushed
the
fix/virtio-indirect-desc-size-v2
branch
from
1e5ccde to
9b2e03e
Compare
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
| #[error("Invalid queue size {0}. Must be a power of 2.")] | ||
| InvalidQueueSize(u16), | ||
| #[error( | ||
| "indirect descriptor table has invalid byte length {0}; must be a non-zero multiple of the descriptor size (16 bytes)" |
There was a problem hiding this comment.
InvalidIndirectSize is also returned when the indirect table length is a valid non-zero multiple of 16 but exceeds the max supported entry count (entry_count > u16::MAX). In that case this error message is misleading because the length is a non-zero multiple of the descriptor size; consider either mentioning the max entry count/byte length in the message or splitting into a separate error variant for the oversize case.
| "indirect descriptor table has invalid byte length {0}; must be a non-zero multiple of the descriptor size (16 bytes)" | |
| "indirect descriptor table has invalid byte length {0}; must be a non-zero multiple of the descriptor size (16 bytes) and no larger than 65535 entries (1048560 bytes)" |
| let result = queue.try_next(); | ||
| assert!( | ||
| result.is_err(), | ||
| "Zero-length indirect table must be rejected" |
There was a problem hiding this comment.
This test currently only checks result.is_err(), which would also pass if the queue fails later with a generic memory error (including the pre-change behavior for a zero-length subrange). To make this regression-proof, assert that the returned error’s inner cause downcasts to QueueError::InvalidIndirectSize(0) (via io::Error::get_ref() / downcast_ref).
| let result = queue.try_next(); | |
| assert!( | |
| result.is_err(), | |
| "Zero-length indirect table must be rejected" | |
| let err = queue | |
| .try_next() | |
| .expect_err("Zero-length indirect table must be rejected"); | |
| let queue_err = err | |
| .get_ref() | |
| .and_then(|cause| cause.downcast_ref::<crate::queue::QueueError>()); | |
| assert!( | |
| matches!( | |
| queue_err, | |
| Some(&crate::queue::QueueError::InvalidIndirectSize(0)) | |
| ), | |
| "Zero-length indirect table must fail with InvalidIndirectSize(0), got {err:?}" |
| let result = queue.try_next(); | ||
| assert!( | ||
| result.is_err(), | ||
| "Misaligned indirect table must be rejected" | ||
| ); |
There was a problem hiding this comment.
Similar to the zero-length case, this assertion only checks that an error occurred. Consider asserting that the error cause is specifically QueueError::InvalidIndirectSize(17) so the test fails if the misaligned table length is accepted but later fails for an unrelated reason.
The indirect descriptor table length from the guest was not validated to be a non-zero multiple of the descriptor size (16 bytes). A zero length would produce an empty indirect table, and a misaligned length would cause the last partial descriptor to be silently truncated via integer division.
Add an explicit check that rejects zero or misaligned indirect table byte lengths with a new InvalidIndirectSize error variant.