netvsp: fix untrusted guest input errors#3183
netvsp: fix untrusted guest input errors#3183erfrimod wants to merge 4 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the netvsp VMBus NIC implementation against malformed/untrusted guest inputs (including fuzz-discovered edge cases) by replacing panic paths with explicit validation and structured errors, plus adding regression tests.
Changes:
- Add defensive validation for RX buffer partitioning (
RxBufferRanges) to prevent divide-by-zero/underflow and propagate typed errors. - Replace
GuestBuffers::newpanic/unwrap paths withGuestBuffersErrorand plumb that error into the worker error surface. - Fix
max_subchannels()to correctly exclude the primary channel and add a test for rejecting RSS params withindirection_table_size == 0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| vm/devices/net/netvsp/src/test.rs | Adds regression tests covering RSS invalid input and RX buffer configuration validation. |
| vm/devices/net/netvsp/src/lib.rs | Adds RX buffer config validation + error wiring, fixes max_subchannels logic, and rejects RSS indirection table size of 0. |
| vm/devices/net/netvsp/src/buffers.rs | Introduces GuestBuffersError and converts prior assert/unwrap failure modes into recoverable errors with tests. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| mtu, | ||
| }); | ||
| } | ||
| if gpadl.first().is_none() { |
There was a problem hiding this comment.
should the gpadl empty check come before the other checks, since we have no need to do any math here? Which condition is more likely to hit, or there's not one?
| #[error("GPADL has no ranges")] | ||
| EmptyGpadl, | ||
| #[error("guest memory error")] | ||
| Memory(#[source] GuestMemoryError), |
There was a problem hiding this comment.
is this really a lock error, not generic memory?
| if !active_queues.is_empty() { | ||
| active_queues.len() as u16 | ||
| } else { | ||
| tracelimit::warn_ratelimited!("Invalid RSS indirection table"); |
There was a problem hiding this comment.
do we need any more information with this breadcrumb?
Fuzz testing found several places where netvsp and vmbus channel could panic on edge-case inputs: untrusted guest data, malformed save state, function calls with out-of-range channel indices. Adding defensive validation to turn those panics into Errors.