Repository navigation
Limit virtqueue descriptor chains to the queue size - #536
Merged
Merged
Conversation
An indirect descriptor can only be the last descriptor of a chain in the descriptor table: it cannot have VIRTQ_DESC_F_NEXT set or be in an indirect table (VirtIO spec 1.4, Sec 2.7.5.3.1). SplitQueue::get_avail() asserted the latter, so a guest could panic the device worker with an indirect descriptor in an indirect table. Return an error in both cases instead. Fixes: bd56123 ("feat(virtio): implement split queue operations") Signed-off-by: Changyuan Lyu <changyuanl@google.com>
A driver cannot create a descriptor chain longer than the queue size, counting the descriptors in an indirect table (VirtIO spec 1.4, Sec 2.7.5.3.1). However, SplitQueue::get_avail() follows `next` without a limit, so with a loop in the descriptor table or in an indirect table, a guest could keep the device worker spinning and growing the buffer lists until the host runs out of memory. Fail a chain once it has more descriptors than the queue size. Also take only the descriptors within the length of an indirect table. Fixes: bd56123 ("feat(virtio): implement split queue operations") Signed-off-by: Changyuan Lyu <changyuanl@google.com>
PackedQueue::get_avail() takes every descriptor in an indirect table, whose length comes from the guest, and accepts an indirect descriptor at each descriptor of a list. With a list of indirect descriptors referring to a large table, a guest could make the device worker spend CPU time and host memory far beyond the size of the guest memory. An indirect descriptor cannot be in a list (VirtIO spec 1.4, Sec 2.8.19), so reject it there. The same section lets the device decide the maximum length of a chain; like a split queue, limit an indirect table to the queue size. Fixes: f97eb72 ("feat(virtio): support packed queue") Signed-off-by: Changyuan Lyu <changyuanl@google.com>
Coverage Report for CI Build 42721820371Coverage increased (+0.2%) to 39.717%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SplitQueue::get_avail()walks a descriptor chain without a limit and assertsthat an indirect table has no indirect descriptors. A guest can panic the device
worker with a nested indirect descriptor, or link a chain back to itself to keep
the worker spinning and growing the buffer lists until the host runs out of
memory.
PackedQueue::get_avail()takes every descriptor of an indirect table,whose length comes from the guest, and accepts an indirect descriptor at each
descriptor of a list.
VIRTQ_DESC_F_NEXTset in split queues, instead of panicking.descriptors in an indirect table (VirtIO spec 1.4, Sec 2.7.5.3.1), and read
only the descriptors within an indirect table.
Sec 2.8.19) and limit an indirect table to the queue size.
alioth does not offer
VIRTIO_F_INDIRECT_DESC, and a conforming driver nevercreates a chain longer than the queue size, so these checks do not affect
conforming guests.
Every commit passes
cargo +nightly fmt --check,cargo clippy --all-targets -D warningson x86_64-unknown-linux-gnu/musl, aarch64-unknown-linux-gnu andaarch64-apple-darwin, and
cargo test --workspacelocally.