virtio: validate indirect descriptor table size#3137
virtio: validate indirect descriptor table size#3137benhillis wants to merge 1 commit intomicrosoft:mainfrom
Conversation
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.
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.
| let indirect_byte_len = descriptor.length; | ||
| if indirect_byte_len == 0 | ||
| || !(indirect_byte_len as usize).is_multiple_of(size_of::<SplitDescriptor>()) | ||
| { | ||
| return Err(QueueError::InvalidIndirectSize(indirect_byte_len)); | ||
| } |
There was a problem hiding this comment.
This new validation ensures the indirect table byte length is non-zero and aligned, but the computed entry count is still later derived via division and cast down to u16 without a bounds check. For large aligned descriptor.length values, that cast can truncate and make an oversized indirect table appear small (notably for packed indirect traversal which uses active_indirect_len). Add an explicit check that (descriptor.length / 16) fits in u16 (and reject otherwise) before proceeding.
| let indirect_byte_len = descriptor.length; | ||
| if indirect_byte_len == 0 | ||
| || !(indirect_byte_len as usize).is_multiple_of(size_of::<SplitDescriptor>()) | ||
| { | ||
| return Err(QueueError::InvalidIndirectSize(indirect_byte_len)); | ||
| } |
There was a problem hiding this comment.
The new InvalidIndirectSize path should be covered by a unit/integration test (e.g., an INDIRECT descriptor with length == 0 and another with a non-multiple-of-16 length should cause queue.try_next() to return an error). There are already indirect-descriptor tests in vm/devices/virtio/virtio/src/tests.rs, but none appear to exercise this new validation.
| TooLong, | ||
| #[error("Invalid queue size {0}. Must be a power of 2.")] | ||
| InvalidQueueSize(u16), | ||
| #[error("indirect descriptor table has invalid byte length {0}")] |
There was a problem hiding this comment.
The InvalidIndirectSize error message doesn’t state the actual constraint (must be a non-zero multiple of the descriptor size, 16 bytes). Consider updating the message to include that requirement so failures are actionable when diagnosing guest issues.
| #[error("indirect descriptor table has invalid byte length {0}")] | |
| #[error( | |
| "indirect descriptor table has invalid byte length {0}; must be a non-zero multiple of the descriptor size (16 bytes)" | |
| )] |
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.