Conversation
📝 WalkthroughWalkthroughAdds virtio-gpu protocol definitions, guest-memory and descriptor helpers, control/cursor queue handlers, GPU resource and scanout state, and MMIO/PCI device registration. ChangesVirtio GPU support
Sequence Diagram(s)sequenceDiagram
participant VM as DeviceManagerBuilder
participant GPU as VirtioGpu
participant Queue as ControlqHandler
participant Memory as MemoryAddressSpace
VM->>GPU: Create with shared memory
VM->>GPU: Attach via MMIO or register via PCI
GPU->>Queue: Handle control queue descriptor
Queue->>Memory: Read guest request and backing data
Queue-->>GPU: Return response length
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
crates/vm-device/src/device/virtio/virtio_gpu/controlq_handler.rs (1)
295-297: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnimplemented commands panic via
todo!().
handle_get_edidand manyhandle_commandbranches (GET_CAPSET*,CTX_*,*_3D,*_BLOB) calltodo!(). Most currently correspond to feature bits (VIRTIO_GPU_F_VIRGL,VIRTIO_GPU_F_EDID, blob features) not advertised inVirtioGpu::DEVICE_FEATURES, so a spec-compliant driver shouldn't reach them today — but as a defense-in-depth measure and to avoid surprises as features are added later, consider returningVIRTIO_GPU_RESP_ERR_UNSPECinstead of panicking.Also applies to: 409-437
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm-device/src/device/virtio/virtio_gpu/controlq_handler.rs` around lines 295 - 297, Replace the todo!() calls in handle_get_edid and the unsupported handle_command branches covering GET_CAPSET*, CTX_*, *_3D, and *_BLOB with graceful unsupported-command responses using VIRTIO_GPU_RESP_ERR_UNSPEC. Preserve normal handling for supported commands and ensure none of these currently unadvertised or future feature paths can panic.crates/vm-virtio/src/types/device/gpu.rs (1)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
#[repr(u32)]toVirtioGpuConfigOffsetfor consistency and correctFromReprsignature.
VirtioGpuCtrlTypeinrequest.rsuses#[repr(u32)], butVirtioGpuConfigOffsetomits it. Without#[repr(...)], strum'sFromReprgeneratesfrom_repr(usize)rather thanfrom_repr(u32), forcing callers to cast fromu32MMIO offsets unnecessarily.♻️ Proposed fix
#[derive(FromRepr)] +#[repr(u32)] pub enum VirtioGpuConfigOffset { EventsRead = 0x00,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm-virtio/src/types/device/gpu.rs` around lines 47 - 54, Add #[repr(u32)] to the VirtioGpuConfigOffset enum alongside its existing FromRepr derive, ensuring FromRepr exposes a u32-based from_repr signature consistent with VirtioGpuCtrlType and allowing direct use of u32 MMIO offsets.crates/vm-mm/src/manager.rs (1)
113-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the GPA range validation loop to reduce duplication.
The validation loop (lines 117–126) is identical to the one in
copy_from_slice(lines 74–86) andmemset(lines 39–51). A shared helper would eliminate three copies of the same logic and prevent them from diverging.♻️ Proposed helper extraction
+ fn validate_gpa_range(&self, gpa: u64, len: usize) -> Result<(), Error> { + let mut remaining = len; + let mut check_gpa = gpa; + while remaining > 0 { + let region = self.try_get_region_by_gpa(check_gpa)?; + let offset = check_gpa - region.gpa; + let avail = region.len() - offset as usize; + let step = remaining.min(avail); + remaining -= step; + check_gpa += step as u64; + } + Ok(()) + } pub fn copy_to_slice(&self, mut gpa: u64, buf: &mut [u8]) -> Result<(), Error> { - let mut remaining = buf.len(); - let mut check_gpa = gpa; - - while remaining > 0 { - let region = self.try_get_region_by_gpa(check_gpa)?; - let offset = check_gpa - region.gpa; - let avail = region.len() - offset as usize; - let step = remaining.min(avail); - remaining -= step; - check_gpa += step as u64; - } + self.validate_gpa_range(gpa, buf.len())?; remaining = buf.len();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm-mm/src/manager.rs` around lines 113 - 151, Extract the duplicated GPA range validation loop from copy_to_slice into a shared helper on the manager, then replace the corresponding validation loops in copy_to_slice, copy_from_slice, and memset with calls to that helper. Preserve the existing region lookup and error behavior while keeping each method’s actual memory operation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm-device/src/device/virtio/virtio_gpu/controlq_handler.rs`:
- Around line 32-76: Replace the panic paths in copy_from_iov and
copy_from_framebuffer_to_resource with propagated VirtioError/VirtioGpuError
results: convert memory.copy_to_slice failures and incomplete backing copies
into errors instead of unwrap/assert. Update handle_transfer_to_host_2d and its
callers to propagate the failure and return
VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER for invalid guest addresses or offsets.
- Around line 214-224: Update the transfer bounds validation in the handler
containing the shown VirtioGpuCtrlType response to avoid unchecked u32 additions
for x + width and y + height. Use checked arithmetic so overflow is treated as
an invalid parameter, matching the existing handle_resource_create_2d approach,
while preserving the current rejection of out-of-bounds rectangles.
- Around line 299-442: Replace every descriptor-chain assert in handle_command
with non-panicking validation that returns the appropriate VirtioError when
chain.len() differs from the command’s required descriptor count. Apply this
consistently across all handled command branches, preserving each branch’s
existing processing for valid chains and response behavior.
- Around line 101-142: Update handle_resource_create_2d to reject an
already-registered cmd.resource_id before constructing or inserting the new
VirtioGpuResource. Set resp.r#type to the appropriate invalid-parameter response
and return, preserving the existing resource and scanout binding; only insert
the resource when the id is unused.
In `@crates/vm-virtio/src/types/device/gpu.rs`:
- Line 43: Update the doc comment for blob_alignment to state the representable
u32 maximum, 4294967295, instead of 4294967296. Preserve the existing
power-of-two and minimum-value documentation.
In `@crates/vm-virtio/src/virtqueue/virtq_desc_table.rs`:
- Around line 42-52: Both VirtqDescTable::as_ref and VirtqDescTable::as_mut must
validate the descriptor length before constructing typed slices. In
crates/vm-virtio/src/virtqueue/virtq_desc_table.rs lines 42-52, add the self.len
versus size_of::<T>() check before the immutable slice creation; apply the
identical check in lines 56-66 before mutable slice creation, returning
VirtioError::TransmuteDesc when the descriptor is too short.
- Around line 138-149: Update get_chain_mut to prevent infinite traversal by
tracking visited descriptor indices and terminating or rejecting the chain when
an index repeats; apply the same cycle protection to get_chain so both traversal
methods are safe. Resolve the misleading mut API in get_chain_mut by either
removing it and reusing get_chain, or changing it to take &mut self, call
get_mut, and return mutable descriptor references if callers require mutation.
---
Nitpick comments:
In `@crates/vm-device/src/device/virtio/virtio_gpu/controlq_handler.rs`:
- Around line 295-297: Replace the todo!() calls in handle_get_edid and the
unsupported handle_command branches covering GET_CAPSET*, CTX_*, *_3D, and
*_BLOB with graceful unsupported-command responses using
VIRTIO_GPU_RESP_ERR_UNSPEC. Preserve normal handling for supported commands and
ensure none of these currently unadvertised or future feature paths can panic.
In `@crates/vm-mm/src/manager.rs`:
- Around line 113-151: Extract the duplicated GPA range validation loop from
copy_to_slice into a shared helper on the manager, then replace the
corresponding validation loops in copy_to_slice, copy_from_slice, and memset
with calls to that helper. Preserve the existing region lookup and error
behavior while keeping each method’s actual memory operation unchanged.
In `@crates/vm-virtio/src/types/device/gpu.rs`:
- Around line 47-54: Add #[repr(u32)] to the VirtioGpuConfigOffset enum
alongside its existing FromRepr derive, ensuring FromRepr exposes a u32-based
from_repr signature consistent with VirtioGpuCtrlType and allowing direct use of
u32 MMIO offsets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfc1dfd0-c4de-40e3-aa7f-596930ec359a
📒 Files selected for processing (27)
crates/vm-cli/src/cmd/device.rscrates/vm-device/src/device/mod.rscrates/vm-device/src/device/virtio.rscrates/vm-device/src/device/virtio/virtio_gpu.rscrates/vm-device/src/device/virtio/virtio_gpu/controlq_handler.rscrates/vm-device/src/device/virtio/virtio_gpu/cursorq_handler.rscrates/vm-device/src/device/virtio/virtio_gpu/resource.rscrates/vm-device/src/device/virtio/virtio_gpu/scanout.rscrates/vm-mm/src/manager.rscrates/vm-virtio/src/result.rscrates/vm-virtio/src/types/device.rscrates/vm-virtio/src/types/device/gpu.rscrates/vm-virtio/src/types/device/gpu/error.rscrates/vm-virtio/src/types/device/gpu/request.rscrates/vm-virtio/src/types/device/gpu/request/cmd_get_display_info.rscrates/vm-virtio/src/types/device/gpu/request/cmd_get_edid.rscrates/vm-virtio/src/types/device/gpu/request/cmd_resource_attach_backing.rscrates/vm-virtio/src/types/device/gpu/request/cmd_resource_create_2d.rscrates/vm-virtio/src/types/device/gpu/request/cmd_resource_detach_baking.rscrates/vm-virtio/src/types/device/gpu/request/cmd_resource_flush.rscrates/vm-virtio/src/types/device/gpu/request/cmd_resource_unref.rscrates/vm-virtio/src/types/device/gpu/request/cmd_set_scanout.rscrates/vm-virtio/src/types/device/gpu/request/cmd_transfer_to_host_2d.rscrates/vm-virtio/src/types/device/gpu/request/virtio_gpu_scanout.rscrates/vm-virtio/src/types/device_id.rscrates/vm-virtio/src/virtqueue/virtq_desc_table.rscrates/vm-vmm/src/vm/device_builder.rs
Comment on lines
+32
to
+76
| fn copy_from_iov( | ||
| memory: &MemoryAddressSpace, | ||
| iovs: &[VirtioGpuMemBacking], | ||
| src_offset: u64, | ||
| mut dst: &mut [u8], | ||
| ) -> bool { | ||
| let mut offset = src_offset; | ||
|
|
||
| for iov in iovs { | ||
| if offset >= iov.length as u64 { | ||
| offset -= iov.length as u64; | ||
| continue; | ||
| } | ||
|
|
||
| let len = (iov.length as usize - offset as usize).min(dst.len()); | ||
|
|
||
| memory | ||
| .copy_to_slice(iov.addr + offset, &mut dst[..len]) | ||
| .unwrap(); | ||
|
|
||
| dst = &mut dst[len..]; | ||
|
|
||
| if dst.is_empty() { | ||
| return true; | ||
| } | ||
|
|
||
| offset = 0; | ||
| } | ||
|
|
||
| false | ||
| } | ||
|
|
||
| fn copy_from_framebuffer_to_resource( | ||
| memory: &MemoryAddressSpace, | ||
| resource: &mut VirtioGpuResource, | ||
| src_offset: u64, | ||
| dst_offset: u32, | ||
| len: usize, | ||
| ) { | ||
| let dst = &mut resource.buf[dst_offset as usize..dst_offset as usize + len]; | ||
|
|
||
| let is_empty = copy_from_iov(memory, &resource.backing, src_offset, dst); | ||
|
|
||
| assert!(is_empty); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guest-controlled memory addresses/offsets can panic the process.
copy_from_iov calls .unwrap() on memory.copy_to_slice(iov.addr + offset, ...), where iov.addr comes straight from the guest via RESOURCE_ATTACH_BACKING (Line 269-273). An invalid/unmapped address supplied by the guest will panic here instead of returning an error. Separately, copy_from_framebuffer_to_resource's assert!(is_empty) (Line 75) fires whenever the guest-controlled cmd.offset in TRANSFER_TO_HOST_2D runs past the total attached backing length — also guest-triggerable.
Propagate a VirtioError/VirtioGpuError instead of panicking so a malicious or buggy driver can only get an error response, not crash the VMM.
Sketch
-fn copy_from_iov(
+fn copy_from_iov(
memory: &MemoryAddressSpace,
iovs: &[VirtioGpuMemBacking],
src_offset: u64,
mut dst: &mut [u8],
-) -> bool {
+) -> Result<bool, VirtioError> {
...
- memory
- .copy_to_slice(iov.addr + offset, &mut dst[..len])
- .unwrap();
+ memory.copy_to_slice(iov.addr + offset, &mut dst[..len])?;
...
- false
+ Ok(false)
}copy_from_framebuffer_to_resource and handle_transfer_to_host_2d would then propagate the Result and set VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER on failure instead of asserting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm-device/src/device/virtio/virtio_gpu/controlq_handler.rs` around
lines 32 - 76, Replace the panic paths in copy_from_iov and
copy_from_framebuffer_to_resource with propagated VirtioError/VirtioGpuError
results: convert memory.copy_to_slice failures and incomplete backing copies
into errors instead of unwrap/assert. Update handle_transfer_to_host_2d and its
callers to propagate the failure and return
VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER for invalid guest addresses or offsets.
Comment on lines
+101
to
+142
| async fn handle_resource_create_2d( | ||
| &self, | ||
| cmd: &VirtioGpuResourceCreate2D, | ||
| resp: &mut VirtioGpuCtrlHdr, | ||
| ) { | ||
| let mut resources = self.resource.lock().await; | ||
|
|
||
| let Some(format) = VirtioGpuFormats::from_repr(cmd.format) else { | ||
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | ||
|
|
||
| return; | ||
| }; | ||
|
|
||
| let Some(stride) = (cmd.width as usize).checked_mul(format.bpp()) else { | ||
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | ||
|
|
||
| return; | ||
| }; | ||
|
|
||
| let Some(len) = (cmd.height as usize).checked_mul(stride) else { | ||
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | ||
|
|
||
| return; | ||
| }; | ||
|
|
||
| let buf = vec![0; len]; | ||
|
|
||
| resources.insert( | ||
| cmd.resource_id, | ||
| VirtioGpuResource { | ||
| id: cmd.resource_id, | ||
| format, | ||
| width: cmd.width, | ||
| height: cmd.height, | ||
| stride, | ||
| buf, | ||
| backing: Default::default(), | ||
| }, | ||
| ); | ||
|
|
||
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_OK_NODATA as u32; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Duplicate resource_id silently overwrites the existing resource.
resources.insert(cmd.resource_id, ...) overwrites any resource already registered under that id without checking. A guest re-creating an id currently bound to a scanout (via SET_SCANOUT) could swap the scanout's backing content without an explicit SET_SCANOUT, which deviates from expected resource-id semantics.
Suggested fix
+ if resources.contains_key(&cmd.resource_id) {
+ resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID as u32;
+ return;
+ }
+
let Some(format) = VirtioGpuFormats::from_repr(cmd.format) else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async fn handle_resource_create_2d( | |
| &self, | |
| cmd: &VirtioGpuResourceCreate2D, | |
| resp: &mut VirtioGpuCtrlHdr, | |
| ) { | |
| let mut resources = self.resource.lock().await; | |
| let Some(format) = VirtioGpuFormats::from_repr(cmd.format) else { | |
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | |
| return; | |
| }; | |
| let Some(stride) = (cmd.width as usize).checked_mul(format.bpp()) else { | |
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | |
| return; | |
| }; | |
| let Some(len) = (cmd.height as usize).checked_mul(stride) else { | |
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | |
| return; | |
| }; | |
| let buf = vec![0; len]; | |
| resources.insert( | |
| cmd.resource_id, | |
| VirtioGpuResource { | |
| id: cmd.resource_id, | |
| format, | |
| width: cmd.width, | |
| height: cmd.height, | |
| stride, | |
| buf, | |
| backing: Default::default(), | |
| }, | |
| ); | |
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_OK_NODATA as u32; | |
| } | |
| async fn handle_resource_create_2d( | |
| &self, | |
| cmd: &VirtioGpuResourceCreate2D, | |
| resp: &mut VirtioGpuCtrlHdr, | |
| ) { | |
| let mut resources = self.resource.lock().await; | |
| if resources.contains_key(&cmd.resource_id) { | |
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID as u32; | |
| return; | |
| } | |
| let Some(format) = VirtioGpuFormats::from_repr(cmd.format) else { | |
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | |
| return; | |
| }; | |
| let Some(stride) = (cmd.width as usize).checked_mul(format.bpp()) else { | |
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | |
| return; | |
| }; | |
| let Some(len) = (cmd.height as usize).checked_mul(stride) else { | |
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | |
| return; | |
| }; | |
| let buf = vec![0; len]; | |
| resources.insert( | |
| cmd.resource_id, | |
| VirtioGpuResource { | |
| id: cmd.resource_id, | |
| format, | |
| width: cmd.width, | |
| height: cmd.height, | |
| stride, | |
| buf, | |
| backing: Default::default(), | |
| }, | |
| ); | |
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_OK_NODATA as u32; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm-device/src/device/virtio/virtio_gpu/controlq_handler.rs` around
lines 101 - 142, Update handle_resource_create_2d to reject an
already-registered cmd.resource_id before constructing or inserting the new
VirtioGpuResource. Set resp.r#type to the appropriate invalid-parameter response
and return, preserving the existing resource and scanout binding; only insert
the resource when the id is unused.
Comment on lines
+214
to
+224
| if cmd.r.x > resource.width | ||
| || cmd.r.width > resource.width | ||
| || cmd.r.x + cmd.r.width > resource.width | ||
| || cmd.r.y > resource.height | ||
| || cmd.r.height > resource.height | ||
| || cmd.r.y + cmd.r.height > resource.height | ||
| { | ||
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unchecked u32 addition in the transfer bounds check can overflow.
cmd.r.x + cmd.r.width > resource.width and cmd.r.y + cmd.r.height > resource.height can overflow u32 and wrap, bypassing the intended bound. handle_resource_create_2d already uses checked_mul for similar arithmetic (Lines 114-124); do the same here.
Suggested fix
- if cmd.r.x > resource.width
- || cmd.r.width > resource.width
- || cmd.r.x + cmd.r.width > resource.width
- || cmd.r.y > resource.height
- || cmd.r.height > resource.height
- || cmd.r.y + cmd.r.height > resource.height
- {
+ let x_end = cmd.r.x.checked_add(cmd.r.width);
+ let y_end = cmd.r.y.checked_add(cmd.r.height);
+ if x_end.is_none_or(|v| v > resource.width) || y_end.is_none_or(|v| v > resource.height) {
resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32;
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if cmd.r.x > resource.width | |
| || cmd.r.width > resource.width | |
| || cmd.r.x + cmd.r.width > resource.width | |
| || cmd.r.y > resource.height | |
| || cmd.r.height > resource.height | |
| || cmd.r.y + cmd.r.height > resource.height | |
| { | |
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | |
| return; | |
| } | |
| let x_end = cmd.r.x.checked_add(cmd.r.width); | |
| let y_end = cmd.r.y.checked_add(cmd.r.height); | |
| if x_end.is_none_or(|v| v > resource.width) || y_end.is_none_or(|v| v > resource.height) { | |
| resp.r#type = VirtioGpuCtrlType::VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER as u32; | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm-device/src/device/virtio/virtio_gpu/controlq_handler.rs` around
lines 214 - 224, Update the transfer bounds validation in the handler containing
the shown VirtioGpuCtrlType response to avoid unchecked u32 additions for x +
width and y + height. Use checked arithmetic so overflow is treated as an
invalid parameter, matching the existing handle_resource_create_2d approach,
while preserving the current rejection of out-of-bounds rectangles.
Comment on lines
+299
to
+442
| async fn handle_command( | ||
| &self, | ||
| desc_ring: &VirtqDescTableRef, | ||
| desc_id: u16, | ||
| ) -> Result<u32, VirtioError> { | ||
| let desc_entry = desc_ring.get(desc_id); | ||
| let command = desc_entry.as_ref::<VirtioGpuCtrlHdr>(&self.memory)?; | ||
|
|
||
| let ctrl_type = VirtioGpuCtrlType::from_repr(command.r#type) | ||
| .ok_or(VirtioGpuError::UnknownCtrlType(command.r#type))?; | ||
|
|
||
| match ctrl_type { | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_GET_DISPLAY_INFO => { | ||
| let chain = desc_ring.get_chain(desc_id); | ||
| assert_eq!(chain.len(), 2); | ||
|
|
||
| let response = chain[1].as_mut::<VirtioGpuRespDisplayInfo>(&self.memory)?; | ||
| response.as_mut_bytes().fill(0); | ||
|
|
||
| self.handle_get_display_info(response).await; | ||
|
|
||
| Ok(size_of::<VirtioGpuRespDisplayInfo>().try_into().unwrap()) | ||
| } | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_RESOURCE_CREATE_2D => { | ||
| let chain = desc_ring.get_chain(desc_id); | ||
| assert_eq!(chain.len(), 2); | ||
|
|
||
| let cmd = chain[0].as_ref::<VirtioGpuResourceCreate2D>(&self.memory)?; | ||
| let response = chain[1].as_mut::<VirtioGpuCtrlHdr>(&self.memory)?; | ||
| response.as_mut_bytes().fill(0); | ||
|
|
||
| self.handle_resource_create_2d(cmd, response).await; | ||
|
|
||
| Ok(size_of::<VirtioGpuCtrlHdr>().try_into().unwrap()) | ||
| } | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_RESOURCE_UNREF => { | ||
| let chain = desc_ring.get_chain(desc_id); | ||
| assert_eq!(chain.len(), 2); | ||
|
|
||
| let cmd = chain[0].as_ref::<VirtioGpuResourceUnref>(&self.memory)?; | ||
| let response = chain[1].as_mut::<VirtioGpuCtrlHdr>(&self.memory)?; | ||
| response.as_mut_bytes().fill(0); | ||
|
|
||
| self.handle_resource_unref(cmd, response).await; | ||
|
|
||
| Ok(size_of::<VirtioGpuCtrlHdr>().try_into().unwrap()) | ||
| } | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_SET_SCANOUT => { | ||
| let chain = desc_ring.get_chain(desc_id); | ||
| assert_eq!(chain.len(), 2); | ||
|
|
||
| let cmd = chain[0].as_ref::<VirtioGpuSetScanout>(&self.memory)?; | ||
| let response = chain[1].as_mut::<VirtioGpuCtrlHdr>(&self.memory)?; | ||
| response.as_mut_bytes().fill(0); | ||
|
|
||
| self.handle_set_scanout(cmd, response).await; | ||
|
|
||
| Ok(size_of::<VirtioGpuCtrlHdr>().try_into().unwrap()) | ||
| } | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_RESOURCE_FLUSH => { | ||
| let chain = desc_ring.get_chain(desc_id); | ||
| assert_eq!(chain.len(), 2); | ||
|
|
||
| let cmd = chain[0].as_ref::<VirtioGpuCmdResourceFlush>(&self.memory)?; | ||
| let response = chain[1].as_mut::<VirtioGpuCtrlHdr>(&self.memory)?; | ||
| response.as_mut_bytes().fill(0); | ||
|
|
||
| self.handle_resource_flush(cmd, response).await; | ||
|
|
||
| Ok(size_of::<VirtioGpuCtrlHdr>().try_into().unwrap()) | ||
| } | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D => { | ||
| let chain = desc_ring.get_chain(desc_id); | ||
| assert_eq!(chain.len(), 2); | ||
|
|
||
| let cmd = chain[0].as_ref::<VirtioGpuTransferToHost2D>(&self.memory)?; | ||
| let response = chain[1].as_mut::<VirtioGpuCtrlHdr>(&self.memory)?; | ||
| response.as_mut_bytes().fill(0); | ||
|
|
||
| self.handle_transfer_to_host_2d(cmd, response).await; | ||
|
|
||
| Ok(size_of::<VirtioGpuCtrlHdr>().try_into().unwrap()) | ||
| } | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING => { | ||
| let chain = desc_ring.get_chain(desc_id); | ||
| assert_eq!(chain.len(), 3); | ||
|
|
||
| let cmd = chain[0].as_ref::<VirtioGpuResourceAttachBacking>(&self.memory)?; | ||
| let entries = chain[1] | ||
| .as_slice::<VirtioGpuMemEntry>(&self.memory, cmd.nr_entries as usize)?; | ||
| let response = chain[2].as_mut::<VirtioGpuCtrlHdr>(&self.memory)?; | ||
| response.as_mut_bytes().fill(0); | ||
|
|
||
| self.handle_resource_attach_backing(cmd, entries, response) | ||
| .await; | ||
|
|
||
| Ok(size_of::<VirtioGpuCtrlHdr>().try_into().unwrap()) | ||
| } | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING => { | ||
| let chain = desc_ring.get_chain(desc_id); | ||
| assert_eq!(chain.len(), 2); | ||
|
|
||
| let cmd = chain[0].as_ref::<VirtioGpuResourceDetachBacking>(&self.memory)?; | ||
| let response = chain[1].as_mut::<VirtioGpuCtrlHdr>(&self.memory)?; | ||
| response.as_mut_bytes().fill(0); | ||
|
|
||
| self.handle_resource_detach_backing(cmd, response).await; | ||
|
|
||
| Ok(size_of::<VirtioGpuCtrlHdr>().try_into().unwrap()) | ||
| } | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_GET_CAPSET_INFO => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_GET_CAPSET => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_GET_EDID => { | ||
| let chain = desc_ring.get_chain(desc_id); | ||
| assert_eq!(chain.len(), 2); | ||
|
|
||
| let cmd = chain[0].as_ref::<VirtioGpuGetEdid>(&self.memory)?; | ||
| let response = chain[1].as_mut::<VirtioGpuRespEdid>(&self.memory)?; | ||
| response.as_mut_bytes().fill(0); | ||
|
|
||
| self.handle_get_edid(cmd, response); | ||
|
|
||
| Ok(size_of::<VirtioGpuRespEdid>().try_into().unwrap()) | ||
| } | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_SET_SCANOUT_BLOB => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_CTX_CREATE => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_CTX_DESTROY => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_CTX_ATTACH_RESOURCE => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_CTX_DETACH_RESOURCE => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_RESOURCE_CREATE_3D => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_TRANSFER_TO_HOST_3D => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_TRANSFER_FROM_HOST_3D => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_SUBMIT_3D => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_UPDATE_CURSOR => todo!(), | ||
| VirtioGpuCtrlType::VIRTIO_GPU_CMD_MOVE_CURSOR => todo!(), | ||
| _ => Err(VirtioError::VirtioGpu(VirtioGpuError::InvalidCommand( | ||
| ctrl_type, | ||
| ))), | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Malformed descriptor chains panic instead of returning an error.
Every branch in handle_command (e.g. Lines 313, 324, 336, 348, 360, 372, 384, 399, 413) asserts the exact descriptor-chain length with assert_eq!. A guest that sends a control command with the wrong number of chained buffers crashes the whole process instead of getting an error response.
Sketch
- let chain = desc_ring.get_chain(desc_id);
- assert_eq!(chain.len(), 2);
+ let chain = desc_ring.get_chain(desc_id);
+ if chain.len() != 2 {
+ return Err(VirtioError::VirtioGpu(VirtioGpuError::InvalidCommand(ctrl_type)));
+ }Apply the same pattern to every assert_eq!(chain.len(), N) site in this function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm-device/src/device/virtio/virtio_gpu/controlq_handler.rs` around
lines 299 - 442, Replace every descriptor-chain assert in handle_command with
non-panicking validation that returns the appropriate VirtioError when
chain.len() differs from the command’s required descriptor count. Apply this
consistently across all handled command branches, preserving each branch’s
existing processing for valid chains and response behavior.
| /// value is zero. | ||
| pub num_capsets: u32, | ||
| /// specifies the minimal alignment, in bytes, required by the device for resource blobs. The | ||
| /// value is a power of two. Minimum value is 1, maximum value is 4294967296. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Doc comment says "maximum value is 4294967296" but that overflows u32.
blob_alignment is u32 whose max is 4294967295. The value 4294967296 (2³²) cannot be represented. Clarify the comment to avoid confusion.
📝 Proposed fix
- /// specifies the minimal alignment, in bytes, required by the device for resource blobs. The
- /// value is a power of two. Minimum value is 1, maximum value is 4294967296.
+ /// specifies the minimal alignment, in bytes, required by the device for resource blobs. The
+ /// value is a power of two. Minimum value is 1, maximum value is 4294967295.
pub blob_alignment: u32,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// value is a power of two. Minimum value is 1, maximum value is 4294967296. | |
| /// specifies the minimal alignment, in bytes, required by the device for resource blobs. The | |
| /// value is a power of two. Minimum value is 1, maximum value is 4294967295. | |
| pub blob_alignment: u32, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm-virtio/src/types/device/gpu.rs` at line 43, Update the doc comment
for blob_alignment to state the representable u32 maximum, 4294967295, instead
of 4294967296. Preserve the existing power-of-two and minimum-value
documentation.
Comment on lines
+42
to
+52
| pub fn as_ref<T>(&self, memory: &MemoryAddressSpace) -> Result<&T> | ||
| where | ||
| T: FromBytes + KnownLayout + Immutable, | ||
| { | ||
| let req: NonNull<u8> = self.addr(memory)?; | ||
|
|
||
| let bytes = unsafe { slice::from_raw_parts(req.as_ptr(), size_of::<T>()) }; | ||
| let t = T::ref_from_bytes(bytes).map_err(|_| VirtioError::TransmuteDesc)?; | ||
|
|
||
| Ok(t) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Missing self.len bounds check in as_ref and as_mut. Both methods create typed slices from guest memory using size_of::<T>() without validating that the descriptor's len is large enough, unlike as_slice which performs this check at Line 79. A descriptor with a smaller len causes out-of-bounds reads (in as_ref) or writes (in as_mut), leading to incorrect data processing or guest memory corruption.
crates/vm-virtio/src/virtqueue/virtq_desc_table.rs#L42-L52: Addif size_of::<T>() > self.len as usize { return Err(VirtioError::TransmuteDesc); }before creating the slice inas_ref.crates/vm-virtio/src/virtqueue/virtq_desc_table.rs#L56-L66: Add the same bounds check inas_mutbefore creating the mutable slice.
🛡️ Proposed fix for both methods
pub fn as_ref<T>(&self, memory: &MemoryAddressSpace) -> Result<&T>
where
T: FromBytes + KnownLayout + Immutable,
{
+ if size_of::<T>() > self.len as usize {
+ return Err(VirtioError::TransmuteDesc);
+ }
let req: NonNull<u8> = self.addr(memory)?;
let bytes = unsafe { slice::from_raw_parts(req.as_ptr(), size_of::<T>()) };
let t = T::ref_from_bytes(bytes).map_err(|_| VirtioError::TransmuteDesc)?;
Ok(t)
} #[allow(clippy::mut_from_ref)]
pub fn as_mut<T>(&self, memory: &MemoryAddressSpace) -> Result<&mut T>
where
T: FromBytes + IntoBytes + KnownLayout,
{
+ if size_of::<T>() > self.len as usize {
+ return Err(VirtioError::TransmuteDesc);
+ }
let req: NonNull<u8> = self.addr(memory)?;
let bytes = unsafe { slice::from_raw_parts_mut(req.as_ptr(), size_of::<T>()) };
let t = T::mut_from_bytes(bytes).map_err(|_| VirtioError::TransmuteDesc)?;
Ok(t)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn as_ref<T>(&self, memory: &MemoryAddressSpace) -> Result<&T> | |
| where | |
| T: FromBytes + KnownLayout + Immutable, | |
| { | |
| let req: NonNull<u8> = self.addr(memory)?; | |
| let bytes = unsafe { slice::from_raw_parts(req.as_ptr(), size_of::<T>()) }; | |
| let t = T::ref_from_bytes(bytes).map_err(|_| VirtioError::TransmuteDesc)?; | |
| Ok(t) | |
| } | |
| pub fn as_ref<T>(&self, memory: &MemoryAddressSpace) -> Result<&T> | |
| where | |
| T: FromBytes + KnownLayout + Immutable, | |
| { | |
| if size_of::<T>() > self.len as usize { | |
| return Err(VirtioError::TransmuteDesc); | |
| } | |
| let req: NonNull<u8> = self.addr(memory)?; | |
| let bytes = unsafe { slice::from_raw_parts(req.as_ptr(), size_of::<T>()) }; | |
| let t = T::ref_from_bytes(bytes).map_err(|_| VirtioError::TransmuteDesc)?; | |
| Ok(t) | |
| } |
📍 Affects 1 file
crates/vm-virtio/src/virtqueue/virtq_desc_table.rs#L42-L52(this comment)crates/vm-virtio/src/virtqueue/virtq_desc_table.rs#L56-L66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm-virtio/src/virtqueue/virtq_desc_table.rs` around lines 42 - 52,
Both VirtqDescTable::as_ref and VirtqDescTable::as_mut must validate the
descriptor length before constructing typed slices. In
crates/vm-virtio/src/virtqueue/virtq_desc_table.rs lines 42-52, add the self.len
versus size_of::<T>() check before the immutable slice creation; apply the
identical check in lines 56-66 before mutable slice creation, returning
VirtioError::TransmuteDesc when the descriptor is too short.
Comment on lines
+138
to
+149
| pub fn get_chain_mut(&self, first_idx: u16) -> Vec<&VirtqDesc> { | ||
| let mut descs = vec![]; | ||
|
|
||
| let mut curr = self.get(first_idx); | ||
| descs.push(curr); | ||
| while curr.flags & VIRTQ_DESC_F_NEXT != 0 { | ||
| curr = self.get(curr.next); | ||
| descs.push(curr); | ||
| } | ||
|
|
||
| descs | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
get_chain_mut lacks cycle detection and duplicates get_chain.
The method follows NEXT-linked descriptors without cycle detection — a malicious guest could create a circular chain (e.g., A→B→A) to hang the VMM indefinitely. The pre-existing get_chain (Lines 125-136) has the same vulnerability, but this is new code that perpetuates it.
Additionally, get_chain_mut is identical to get_chain despite the "mut" name: it takes &self and returns Vec<&VirtqDesc> (shared references). If mutable access is intended, it should take &mut self, use get_mut, and return Vec<&mut VirtqDesc>. If not, remove it and use get_chain directly.
🔒 Proposed fix: add cycle detection
pub fn get_chain_mut(&self, first_idx: u16) -> Vec<&VirtqDesc> {
let mut descs = vec![];
let mut curr = self.get(first_idx);
descs.push(curr);
while curr.flags & VIRTQ_DESC_F_NEXT != 0 {
curr = self.get(curr.next);
descs.push(curr);
+ if descs.len() >= self.queue_size as usize {
+ break;
+ }
}
descs
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn get_chain_mut(&self, first_idx: u16) -> Vec<&VirtqDesc> { | |
| let mut descs = vec![]; | |
| let mut curr = self.get(first_idx); | |
| descs.push(curr); | |
| while curr.flags & VIRTQ_DESC_F_NEXT != 0 { | |
| curr = self.get(curr.next); | |
| descs.push(curr); | |
| } | |
| descs | |
| } | |
| pub fn get_chain_mut(&self, first_idx: u16) -> Vec<&VirtqDesc> { | |
| let mut descs = vec![]; | |
| let mut curr = self.get(first_idx); | |
| descs.push(curr); | |
| while curr.flags & VIRTQ_DESC_F_NEXT != 0 { | |
| curr = self.get(curr.next); | |
| descs.push(curr); | |
| if descs.len() >= self.queue_size as usize { | |
| break; | |
| } | |
| } | |
| descs | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm-virtio/src/virtqueue/virtq_desc_table.rs` around lines 138 - 149,
Update get_chain_mut to prevent infinite traversal by tracking visited
descriptor indices and terminating or rejecting the chain when an index repeats;
apply the same cycle protection to get_chain so both traversal methods are safe.
Resolve the misleading mut API in get_chain_mut by either removing it and
reusing get_chain, or changing it to take &mut self, call get_mut, and return
mutable descriptor references if callers require mutation.
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
1 participant
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.
Summary by CodeRabbit