Skip to content

refine: Refine virtio-pci #96

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
junyu0312 merged 4 commits into from
Mar 6, 2026
Merged

refine: Refine virtio-pci #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bed6bd9
refine: Refine irq for virtio-mmio
junyu0312 Mar 5, 2026
2913b7a
refine: Reduce the visibility of virtio transport
junyu0312 Mar 5, 2026
9a1c5f4
refine: Refine virtio-pci
junyu0312 Mar 5, 2026
22f8f8a
refine: Refine error handling
junyu0312 Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/vm-device/src/device/virtio/virtio_balloon_traditional.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ where
C: MemoryContainer,
{
const NAME: &str = "virtio-balloon-tranditional";
const DEVICE_ID: u32 = DeviceId::Balloon as u32;
const DEVICE_ID: u16 = DeviceId::Balloon as u16;
const DEVICE_FEATURES: u64 = (1 << VIRTIO_F_VERSION_1);

fn virtqueues_size_max(&self) -> Vec<Option<u32>> {
Expand Down
2 changes: 1 addition & 1 deletion crates/vm-device/src/device/virtio/virtio_blk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ where
C: MemoryContainer,
{
const NAME: &str = "virtio-blk";
const DEVICE_ID: u32 = DeviceId::Blk as u32;
const DEVICE_ID: u16 = DeviceId::Blk as u16;
const DEVICE_FEATURES: u64 = (1 << VIRTIO_F_VERSION_1);

fn virtqueues_size_max(&self) -> Vec<Option<u32>> {
Expand Down
6 changes: 2 additions & 4 deletions crates/vm-virtio/src/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,19 +78,17 @@ where
}

if updated {
// update irq
let mut isr = dev.get_interrupt_status();
isr.insert(InterruptStatus::VIRTIO_MMIO_INT_VRING);
dev.set_interrupt_status(isr);
self.irq_chip.trigger_irq(self.irq_line, true);
dev.update_interrupt_status(isr);
}
}
}
}

pub trait VirtioDevice<C>: Sized + Send + Sync + 'static {
const NAME: &str;
const DEVICE_ID: u32;
const DEVICE_ID: u16;
const DEVICE_FEATURES: u64;

fn virtqueues_size_max(&self) -> Vec<Option<u32>>;
Expand Down
4 changes: 2 additions & 2 deletions crates/vm-virtio/src/device/pci.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use vm_pci::device::pci_device::PciDevice;

use crate::device::VirtioDevice;
use crate::transport::VirtioDev;
use crate::transport::pci::VirtioPciFunction;
use crate::transport::pci::VirtioPciTransport;

pub trait VirtioPciDevice<C>: VirtioDevice<C>
where
Expand All @@ -15,7 +15,7 @@ where
const IRQ_PIN: u8;

fn into_pci_device(self) -> PciDevice {
let virtio_function = VirtioPciFunction::<C, _> {
let virtio_function = VirtioPciTransport::<C, _> {
dev: VirtioDev::new(self),
};
let function = Type0Function::new(virtio_function).unwrap();
Expand Down
7 changes: 5 additions & 2 deletions crates/vm-virtio/src/transport.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@ use crate::types::interrupt_status::InterruptStatus;
use crate::types::status::Status;
use crate::virtqueue::Virtqueue;

pub mod control_register;
pub mod mmio;
pub mod pci;

mod control_register;

pub struct VirtioDev<C, D> {
device: D,

Expand Down Expand Up @@ -316,7 +317,9 @@ where
self.interrupt_status
}

pub fn set_interrupt_status(&mut self, is: InterruptStatus) {
pub fn update_interrupt_status(&mut self, is: InterruptStatus) {
self.interrupt_status = is;

self.device.trigger_irq(!self.interrupt_status.is_empty());
}
}
13 changes: 4 additions & 9 deletions crates/vm-virtio/src/transport/mmio/mmio_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ where
match reg {
MmioControlRegister::MagicValue => VIRTIO_MMIO_MAGIC_VALUE,
MmioControlRegister::Version => VIRTIO_MMIO_VERSION,
MmioControlRegister::DeviceId => D::DEVICE_ID,
MmioControlRegister::DeviceId => D::DEVICE_ID as u32,
MmioControlRegister::VendorId => VIRTIO_MMIO_VENDOR_ID,
MmioControlRegister::DeviceFeatures => dev.read_reg(ControlRegister::DeviceFeatures),
MmioControlRegister::QueueSizeMax => dev.read_reg(ControlRegister::QueueSizeMax),
Expand Down Expand Up @@ -66,14 +66,9 @@ where
MmioControlRegister::QueueReady => dev.write_reg(ControlRegister::QueueReady, val),
MmioControlRegister::QueueNotify => dev.write_reg(ControlRegister::QueueNotify, val),
MmioControlRegister::InterruptAck => {
// TODO

dev.interrupt_status
.remove(InterruptStatus::from_bits_truncate(val));

if dev.interrupt_status.is_empty() {
dev.device.trigger_irq(false);
}
let mut is = dev.get_interrupt_status();
is.remove(InterruptStatus::from_bits_truncate(val));
dev.update_interrupt_status(is);

Ok(())
}
Expand Down
117 changes: 21 additions & 96 deletions crates/vm-virtio/src/transport/pci.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,118 +11,36 @@ use vm_pci::types::configuration_space::ConfigurationSpace;

use crate::device::pci::VirtioPciDevice;
use crate::transport::VirtioDev;
use crate::transport::control_register::ControlRegister;
use crate::transport::pci::common_config_handler::CommonConfigHandler;
use crate::transport::pci::pci_header::VENDOR_ID;
use crate::transport::pci::device_handler::DeviceHandler;
use crate::transport::pci::isr_handler::IsrHandler;
use crate::transport::pci::notify_handler::NotifyHandler;
use crate::types::pci::VirtioPciCap;
use crate::types::pci::VirtioPciCapCfgType;
use crate::types::pci::VirtioPciCommonCfg;
use crate::types::pci::VirtioPciNotifyCap;

pub mod pci_header;

mod common_config_handler;
mod device_handler;
mod isr_handler;
mod notify_handler;

struct NotifyHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
dev: Arc<Mutex<VirtioDev<C, D>>>,
}
const VIRTIO_PCI_VENDOR_ID: u16 = 0x1AF4;

impl<C, D> BarHandler for NotifyHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
fn read(&self, _offset: u64, _data: &mut [u8]) {
unreachable!()
}

fn write(&self, _offset: u64, data: &[u8]) {
assert_eq!(data.len(), 2);
let queue_index = u16::from_le_bytes(data.try_into().unwrap());
let mut dev = self.dev.lock().unwrap();
dev.write_reg(ControlRegister::QueueNotify, queue_index.into())
.unwrap();
}
}

struct IsrHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
dev: Arc<Mutex<VirtioDev<C, D>>>,
}

impl<C, D> BarHandler for IsrHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
fn read(&self, _offset: u64, data: &mut [u8]) {
let mut dev = self.dev.lock().unwrap();

let isr = dev.read_reg(ControlRegister::InterruptStatus);
data[0] = isr as u8;

/*
* From `4.1.4.5.1 Device Requirements: ISR status capability`
* - The device MUST reset ISR status to 0 on driver read.
*/
dev.write_reg(ControlRegister::InterruptStatus, 0).unwrap();
dev.device.trigger_irq(false);
}

fn write(&self, _offset: u64, _data: &[u8]) {
unreachable!()
}
}

struct DeviceHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
dev: Arc<Mutex<VirtioDev<C, D>>>,
}

impl<C, D> BarHandler for DeviceHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
fn read(&self, offset: u64, data: &mut [u8]) {
let dev = self.dev.lock().unwrap();

dev.read_config(offset.try_into().unwrap(), data.len(), data)
.unwrap();
}

fn write(&self, offset: u64, data: &[u8]) {
let mut dev = self.dev.lock().unwrap();

dev.write_config(offset.try_into().unwrap(), data.len(), data)
.unwrap();
}
}

pub struct VirtioPciFunction<C, D>
pub struct VirtioPciTransport<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
pub dev: Arc<Mutex<VirtioDev<C, D>>>,
}

impl<C, D> PciTypeFunctionCommon for VirtioPciFunction<C, D>
impl<C, D> PciTypeFunctionCommon for VirtioPciTransport<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
const VENDOR_ID: u16 = VENDOR_ID;
const VENDOR_ID: u16 = VIRTIO_PCI_VENDOR_ID;
const DEVICE_ID: u16 = 0x1040 + D::DEVICE_ID as u16;
const CLASS_CODE: u32 = D::CLASS_CODE;

Expand All @@ -144,7 +62,9 @@ where
bar: 0,
id: 0,
offset: 0,
length: size_of::<VirtioPciCommonCfg>().try_into().unwrap(),
length: size_of::<VirtioPciCommonCfg>()
.try_into()
.map_err(|_| Error::CapTooLarge)?,
Comment on lines +65 to +67
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's check the full imports section and verify size_of usage
head -100 crates/vm-virtio/src/transport/pci.rs | cat -n

Repository: junyu0312/rust-vm

Length of output: 3961

🏁 Script executed:

# Search for size_of usage and mem imports specifically
rg -n 'size_of|use.*mem' crates/vm-virtio/src/transport/pci.rs | head -20

Repository: junyu0312/rust-vm

Length of output: 183

🏁 Script executed:

# Get the full context around lines 65 and 77
sed -n '60,85p' crates/vm-virtio/src/transport/pci.rs | cat -n

Repository: junyu0312/rust-vm

Length of output: 1225

🏁 Script executed:

# Check if there are any other size_of usages beyond lines 65 and 77
rg -n 'size_of::<' crates/vm-virtio/src/transport/pci.rs

Repository: junyu0312/rust-vm

Length of output: 183

🏁 Script executed:

# Verify the full file to confirm no size_of is imported anywhere
wc -l crates/vm-virtio/src/transport/pci.rs
cat crates/vm-virtio/src/transport/pci.rs | cat -n | grep -E '^\s+[0-9]+\s+(use|std|core|mem)'

Repository: junyu0312/rust-vm

Length of output: 1074

size_of is not imported and will fail compilation.

Lines 65 and 77 use size_of::<...>() without importing it. Add the required import statement.

Proposed fix 
 use std::sync::Arc;
+use std::mem::size_of;
 use std::sync::Mutex;
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm-virtio/src/transport/pci.rs` around lines 65 - 67, The code uses
size_of::<VirtioPciCommonCfg>() (and another size_of call) but size_of is not
imported; add the missing import by bringing core::mem::size_of (or
std::mem::size_of depending on crate usage) into scope so the calls in the pci
transport (references: size_of, VirtioPciCommonCfg) compile successfully; ensure
the import is placed with the other use statements at the top of the module.

..Default::default()
};

Expand All @@ -154,7 +74,9 @@ where
{
let virtio_pci_notify_cap = VirtioPciNotifyCap {
cap: VirtioPciCap {
cap_len: size_of::<VirtioPciNotifyCap>().try_into().unwrap(),
cap_len: size_of::<VirtioPciNotifyCap>()
.try_into()
.map_err(|_| Error::CapTooLarge)?,
cfg_type: VirtioPciCapCfgType::VirtioPciCapNotifyCfg as u8,
bar: 1,
id: 0,
Expand Down Expand Up @@ -190,7 +112,10 @@ where
length: 0x1000,
..Default::default()
};
assert!(D::DEVICE_SPECIFICATION_CONFIGURATION_LEN <= 0x1000);

if D::DEVICE_SPECIFICATION_CONFIGURATION_LEN > 0x1000 {
return Err(Error::CapTooLarge);
}

cfg.alloc_capability(virtio_pci_device_cfg_cap.into())?;
}
Expand All @@ -199,7 +124,7 @@ where
}
}

impl<C, D> PciType0Function for VirtioPciFunction<C, D>
impl<C, D> PciType0Function for VirtioPciTransport<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
Expand Down
36 changes: 36 additions & 0 deletions crates/vm-virtio/src/transport/pci/device_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
use std::sync::Arc;
use std::sync::Mutex;

use vm_mm::allocator::MemoryContainer;
use vm_pci::device::function::BarHandler;

use crate::device::pci::VirtioPciDevice;
use crate::transport::VirtioDev;

pub struct DeviceHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
pub dev: Arc<Mutex<VirtioDev<C, D>>>,
}

impl<C, D> BarHandler for DeviceHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
fn read(&self, offset: u64, data: &mut [u8]) {
let dev = self.dev.lock().unwrap();

dev.read_config(offset.try_into().unwrap(), data.len(), data)
.unwrap();
Comment on lines +23 to +27
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n crates/vm-virtio/src/transport/pci/device_handler.rs | head -50

Repository: junyu0312/rust-vm

Length of output: 1147

🏁 Script executed:

rg -n 'fn read_config|fn write_config' crates/vm-virtio/src/ -A 3

Repository: junyu0312/rust-vm

Length of output: 973

🏁 Script executed:

rg -n 'pub fn read_config|pub fn write_config' crates/vm-virtio/src/ -A 3

Repository: junyu0312/rust-vm

Length of output: 658

🏁 Script executed:

rg -n 'BarHandler' crates/vm-virtio/src/ -B 2 -A 2

Repository: junyu0312/rust-vm

Length of output: 3925

🏁 Script executed:

rg -n 'impl.*BarHandler' crates/vm-virtio/src/ -B 2 -A 10

Repository: junyu0312/rust-vm

Length of output: 4224

🏁 Script executed:

rg -n 'Result<' crates/vm-virtio/src/device.rs -B 1 -A 1 | head -40

Repository: junyu0312/rust-vm

Length of output: 253

🏁 Script executed:

# Check if there are other BarHandler implementations with different error handling
fd -e rs -x rg -l 'impl.*BarHandler' {} \;

Repository: junyu0312/rust-vm

Length of output: 284

🏁 Script executed:

cat -n crates/vm-virtio/src/transport/pci/common_config_handler.rs | head -80

Repository: junyu0312/rust-vm

Length of output: 2945

🏁 Script executed:

rg -n 'fn read|fn write' crates/vm-virtio/src/transport/pci/common_config_handler.rs -A 15

Repository: junyu0312/rust-vm

Length of output: 1405

🏁 Script executed:

rg -n 'lock().unwrap()' crates/vm-virtio/src/transport/pci/ -B 2 -A 2

Repository: junyu0312/rust-vm

Length of output: 43

🏁 Script executed:

# Check if there's any documented policy on error handling for guest-triggered operations
rg -n 'guest|BAR|handler' crates/vm-virtio/src/ -i --max-count=5 | head -20

Repository: junyu0312/rust-vm

Length of output: 1961

Handle invalid PCI config offsets and operation failures gracefully instead of panicking.

Lines 26-27 and 33-34 unwrap on guest-supplied offset conversion and fallible config operations. These unwraps can be triggered by malicious or misconfigured guests and will crash the service. The codebase already establishes the correct pattern in CommonConfigHandler, which validates offsets and logs errors instead of panicking.

Errors from read_config() and write_config() should be logged and handled gracefully (typically by filling buffers with zeros for reads or ignoring invalid writes). The offset conversion to usize should also be validated before use.

🛠️ Defensive handling proposal 
+use tracing::warn;
+
 impl<C, D> BarHandler for DeviceHandler<C, D>
 where
     C: MemoryContainer,
     D: VirtioPciDevice<C>,
 {
     fn read(&self, offset: u64, data: &mut [u8]) {
+        let Ok(offset) = usize::try_from(offset) else {
+            warn!(offset, "PCI config read offset too large");
+            data.fill(0);
+            return;
+        };
         let dev = self.dev.lock().unwrap();
-
-        dev.read_config(offset.try_into().unwrap(), data.len(), data)
-            .unwrap();
+        if let Err(err) = dev.read_config(offset, data.len(), data) {
+            warn!(?err, offset, len = data.len(), "PCI config read failed");
+            data.fill(0);
+        }
     }

     fn write(&self, offset: u64, data: &[u8]) {
+        let Ok(offset) = usize::try_from(offset) else {
+            warn!(offset, "PCI config write offset too large");
+            return;
+        };
         let mut dev = self.dev.lock().unwrap();
-
-        dev.write_config(offset.try_into().unwrap(), data.len(), data)
-            .unwrap();
+        if let Err(err) = dev.write_config(offset, data.len(), data) {
+            warn!(?err, offset, len = data.len(), "PCI config write failed");
+        }
     }
 }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm-virtio/src/transport/pci/device_handler.rs` around lines 23 - 27,
The read() (and corresponding write()) methods in device_handler.rs currently
unwrap the guest-supplied offset conversion and the fallible
dev.read_config()/dev.write_config() calls; replace those unwraps with defensive
validation and error handling: attempt offset.try_into() and if it fails log an
error (using the same logger pattern as CommonConfigHandler) and for reads fill
the provided data buffer with zeros and return, for writes ignore the operation;
for dev.read_config()/dev.write_config() match on Result and on Err log the
error and either zero-fill the read buffer or skip the write instead of
panicking. Ensure you reference the same mutex-guarded device (dev =
self.dev.lock().unwrap()) and reuse the logging/error patterns from
CommonConfigHandler so guest-supplied bad offsets or I/O errors are handled
gracefully.

}

fn write(&self, offset: u64, data: &[u8]) {
let mut dev = self.dev.lock().unwrap();

dev.write_config(offset.try_into().unwrap(), data.len(), data)
.unwrap();
}
}
41 changes: 41 additions & 0 deletions crates/vm-virtio/src/transport/pci/isr_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
use std::sync::Arc;
use std::sync::Mutex;

use vm_mm::allocator::MemoryContainer;
use vm_pci::device::function::BarHandler;

use crate::device::pci::VirtioPciDevice;
use crate::transport::VirtioDev;
use crate::transport::control_register::ControlRegister;

pub struct IsrHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
pub dev: Arc<Mutex<VirtioDev<C, D>>>,
}

impl<C, D> BarHandler for IsrHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
fn read(&self, _offset: u64, data: &mut [u8]) {
let mut dev = self.dev.lock().unwrap();

let isr = dev.read_reg(ControlRegister::InterruptStatus);
data[0] = isr as u8;

/*
* From `4.1.4.5.1 Device Requirements: ISR status capability`
* - The device MUST reset ISR status to 0 on driver read.
*/
dev.write_reg(ControlRegister::InterruptStatus, 0).unwrap();
dev.device.trigger_irq(false);
}
Comment on lines +24 to +36
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's see the actual file content and surrounding code
cat -n crates/vm-virtio/src/transport/pci/isr_handler.rs | head -50

Repository: junyu0312/rust-vm

Length of output: 1392

🏁 Script executed:

# Let's also find the BarHandler trait definition
rg -A 10 "trait BarHandler" --type rust

Repository: junyu0312/rust-vm

Length of output: 724

🏁 Script executed:

# Check for any documentation or spec references in the codebase
rg "InterruptStatus" -B 3 -A 3 --type rust | head -40

Repository: junyu0312/rust-vm

Length of output: 3338

🏁 Script executed:

# Let's check if there's spec documentation or comments about the ISR capability
rg "4.1.4.5.1" -B 5 -A 5 --type rust

Repository: junyu0312/rust-vm

Length of output: 982

🏁 Script executed:

# Check how BarHandler is used when calling these functions
rg "\.read\(" crates/vm-pci -B 3 -A 3 --type rust | head -60

Repository: junyu0312/rust-vm

Length of output: 1649

🏁 Script executed:

# Check the VirtioDev definition to understand the register size
rg "struct VirtioDev" -A 20 --type rust | head -40

Repository: junyu0312/rust-vm

Length of output: 1239

🏁 Script executed:

# Look for any test cases or usage of ISR reads
rg "InterruptStatus" crates/vm-virtio -B 2 -A 2 --type rust | grep -A 2 -B 2 "read\|offset"

Repository: junyu0312/rust-vm

Length of output: 1548

🏁 Script executed:

# Check how the MMIO handler processes ISR reads to compare behavior
rg -A 10 "InterruptAck|InterruptStatus" crates/vm-virtio/src/transport/mmio/mmio_handler.rs | head -60

Repository: junyu0312/rust-vm

Length of output: 1555

🏁 Script executed:

# Check if there are actual test cases or callers of the ISR handler
rg "IsrHandler" -B 2 -A 5 --type rust

Repository: junyu0312/rust-vm

Length of output: 2499

🏁 Script executed:

# Look at how BAR reads are invoked at the PCI level
rg "bar_handler.read" -B 5 -A 5 --type rust

Repository: junyu0312/rust-vm

Length of output: 43

🏁 Script executed:

# Check if there's any validation of buffer size in other BarHandler implementations
fd "handler\.rs|bar.*\.rs" --type f | xargs grep -l "BarHandler" | head -5

Repository: junyu0312/rust-vm

Length of output: 276

🏁 Script executed:

# Check other BarHandler implementations for how they handle buffers
cat -n crates/vm-virtio/src/transport/pci/common_config_handler.rs | head -100

Repository: junyu0312/rust-vm

Length of output: 4009

🏁 Script executed:

# Check the device_handler implementation
cat -n crates/vm-virtio/src/transport/pci/device_handler.rs | head -100

Repository: junyu0312/rust-vm

Length of output: 1147

🏁 Script executed:

# Check notify_handler implementation
cat -n crates/vm-virtio/src/transport/pci/notify_handler.rs | head -100

Repository: junyu0312/rust-vm

Length of output: 1192

🏁 Script executed:

# Look at the PCI BAR routing to understand if empty buffers are possible
rg "bar_handler.read\|bar_handler.write" -B 10 -A 3 --type rust

Repository: junyu0312/rust-vm

Length of output: 43

🏁 Script executed:

# Check the VirtIO PCI spec reference - look for ISR capability definition
rg "4.1.4" -B 2 -A 10 --type rust

Repository: junyu0312/rust-vm

Length of output: 1068

🏁 Script executed:

# Check if there are any tests for the ISR handler
find . -name "*.rs" -type f | xargs grep -l "IsrHandler" | xargs grep -l "test\|#\[cfg(test)]"

Repository: junyu0312/rust-vm

Length of output: 45

🏁 Script executed:

# Look for how BAR reads are actually called - check the PCI root complex
cat -n crates/vm-pci/src/root_complex/mmio/bar_handler.rs

Repository: junyu0312/rust-vm

Length of output: 2158

🏁 Script executed:

# Verify the ISR register size from VirtIO spec - check device implementation
rg "read_reg.*InterruptStatus\|InterruptStatus.*u8\|InterruptStatus.*u32" -B 2 -A 2 --type rust | head -40

Repository: junyu0312/rust-vm

Length of output: 43

🏁 Script executed:

# Check if there's a specific BAR size for ISR capability
rg "Bar.*Bar2\|IsrHandler" -B 5 -A 5 --type rust | grep -A 5 -B 5 "Bar2"

Repository: junyu0312/rust-vm

Length of output: 43

Guard ISR read size and offset before acknowledging interrupts.

Line 28 accesses data[0] without validating the buffer is non-empty, which will panic on empty reads. Additionally, line 34 unconditionally clears the ISR status regardless of the requested offset, causing spurious interrupt acknowledgment for reads outside offset 0. Per the VirtIO spec, ISR status should only be cleared on valid reads.

Validate that _offset == 0 and data.len() >= 1 before proceeding. If either condition fails, return early without clearing the ISR status.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm-virtio/src/transport/pci/isr_handler.rs` around lines 24 - 36, In
the read method of the ISR handler (fn read in isr_handler.rs) guard the access
and acknowledgement by first checking that _offset == 0 and data.len() >= 1; if
either check fails return early without touching the device. Only when those
conditions are met, lock self.dev, read ControlRegister::InterruptStatus into
data[0], then clear it via dev.write_reg(ControlRegister::InterruptStatus, 0)
and call dev.device.trigger_irq(false); do not clear or acknowledge interrupts
for out-of-range offsets or empty buffers.


fn write(&self, _offset: u64, _data: &[u8]) {
unreachable!()
}
Comment on lines +38 to +40
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n crates/vm-virtio/src/transport/pci/isr_handler.rs

Repository: junyu0312/rust-vm

Length of output: 1392

🏁 Script executed:

rg -n "BarHandler" crates/vm-virtio/src/transport/pci/ -A 10 -B 2

Repository: junyu0312/rust-vm

Length of output: 8426

🏁 Script executed:

sed -n '85,150p' crates/vm-virtio/src/transport/pci/common_config_handler.rs

Repository: junyu0312/rust-vm

Length of output: 2935

🏁 Script executed:

cat -n crates/vm-virtio/src/transport/pci/notify_handler.rs

Repository: junyu0312/rust-vm

Length of output: 1192

🏁 Script executed:

rg -n "BarHandler" --type rust -B 3 -A 3 | head -60

Repository: junyu0312/rust-vm

Length of output: 3671

🏁 Script executed:

cat -n crates/vm-pci/src/root_complex/mmio_router.rs | head -100

Repository: junyu0312/rust-vm

Length of output: 2240

🏁 Script executed:

rg -n "\.read\(|\.write\(" crates/vm-pci/src/root_complex/ -B 5 -A 5

Repository: junyu0312/rust-vm

Length of output: 2998

Replace unreachable!() with a graceful ignore for guest ISR writes.

The ISR status capability is read-only per Virtio specification, but a guest can still attempt writes to the ISR BAR offset through a MMIO write. The current unreachable!() will panic and crash the hypervisor on such writes. Guest behavior should be handled gracefully by silently ignoring the write rather than panicking.

Suggested fix 
 fn write(&self, _offset: u64, _data: &[u8]) {
-    unreachable!()
+    // ISR status is read-only; guest writes are ignored.
 }
📝 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
fn write(&self, _offset: u64, _data: &[u8]) {
unreachable!()
}
fn write(&self, _offset: u64, _data: &[u8]) {
// ISR status is read-only; guest writes are ignored.
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm-virtio/src/transport/pci/isr_handler.rs` around lines 38 - 40, The
write method in isr_handler.rs currently calls unreachable!(), which panics on
guest MMIO writes; replace it with a graceful no-op that silently ignores writes
to the ISR status capability (i.e., implement fn write(&self, _offset: u64,
_data: &[u8]) { /* ignore guest writes */ } or optionally a debug/trace log) so
guest writes do not crash the hypervisor; update the implementation of write in
the ISR handler accordingly.

}
35 changes: 35 additions & 0 deletions crates/vm-virtio/src/transport/pci/notify_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
use std::sync::Arc;
use std::sync::Mutex;

use vm_mm::allocator::MemoryContainer;
use vm_pci::device::function::BarHandler;

use crate::device::pci::VirtioPciDevice;
use crate::transport::VirtioDev;
use crate::transport::control_register::ControlRegister;

pub struct NotifyHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
pub dev: Arc<Mutex<VirtioDev<C, D>>>,
}

impl<C, D> BarHandler for NotifyHandler<C, D>
where
C: MemoryContainer,
D: VirtioPciDevice<C>,
{
fn read(&self, _offset: u64, _data: &mut [u8]) {
unreachable!()
}

fn write(&self, _offset: u64, data: &[u8]) {
assert_eq!(data.len(), 2);
let queue_index = u16::from_le_bytes(data.try_into().unwrap());
let mut dev = self.dev.lock().unwrap();
dev.write_reg(ControlRegister::QueueNotify, queue_index.into())
.unwrap();
Comment on lines +24 to +33
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n crates/vm-virtio/src/transport/pci/notify_handler.rs

Repository: junyu0312/rust-vm

Length of output: 1192

🏁 Script executed:

# Find BarHandler trait definition
rg -n 'trait BarHandler' --type rust

Repository: junyu0312/rust-vm

Length of output: 135

🏁 Script executed:

# Find where NotifyHandler is used as BarHandler
rg -n 'NotifyHandler.*BarHandler\|impl.*BarHandler.*NotifyHandler' crates/vm-virtio/src --type rust -A5

Repository: junyu0312/rust-vm

Length of output: 43

🏁 Script executed:

# Check how BAR operations are dispatched to handlers
rg -n '\.read\(.*offset\|\.write\(.*offset' crates/vm-virtio/src/transport/pci --type rust -B2 -A2

Repository: junyu0312/rust-vm

Length of output: 43

🏁 Script executed:

# Understand the call chain from guest BAR access
rg -n 'BarHandler\|NotifyHandler' crates/vm-virtio/src/transport/pci --type rust | head -20

Repository: junyu0312/rust-vm

Length of output: 43

🏁 Script executed:

cat -n crates/vm-pci/src/device/function.rs | head -50

Repository: junyu0312/rust-vm

Length of output: 735

🏁 Script executed:

# Search for how BAR reads/writes are dispatched
rg -n 'BarHandler' crates/vm-pci/src --type rust -B3 -A3

Repository: junyu0312/rust-vm

Length of output: 5116

🏁 Script executed:

# Find where handlers are called
rg -n 'handler\.read\|handler\.write' crates --type rust -B2 -A2

Repository: junyu0312/rust-vm

Length of output: 43

🏁 Script executed:

# Look for NotifyHandler instantiation
rg -n 'NotifyHandler' crates --type rust

Repository: junyu0312/rust-vm

Length of output: 445

🏁 Script executed:

# Find where handler.read and handler.write are called from MMIO access
rg -n 'handler\.read|handler\.write' crates/vm-pci --type rust -B5 -A3

Repository: junyu0312/rust-vm

Length of output: 1735

🏁 Script executed:

# Look for MMIO access dispatch in the router
cat -n crates/vm-pci/src/root_complex/mmio_router.rs

Repository: junyu0312/rust-vm

Length of output: 2240

🏁 Script executed:

# Verify the dispatcher is called from guest MMIO access path
rg -n 'pub fn read|pub fn write' crates/vm-pci/src/root_complex/mmio/bar_handler.rs -B2 -A10

Repository: junyu0312/rust-vm

Length of output: 43

🏁 Script executed:

# Check what triggers the MMIO dispatcher
rg -n 'BarHandler|bar_handler' crates/vm-core/src --type rust 2>/dev/null | head -20

Repository: junyu0312/rust-vm

Length of output: 43

Avoid guest-triggerable panics in BAR notify handler.

Guest MMIO operations on BAR1 can trigger three distinct panics:

  • Line 25: unreachable!() on any read operation
  • Line 29: assert_eq!(data.len(), 2) for writes with incorrect size
  • Lines 30 & 33: .unwrap() calls on type conversions and register writes

This allows guests to DoS the VMM process.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm-virtio/src/transport/pci/notify_handler.rs` around lines 24 - 33,
The BAR notify handler currently panics on guest-triggered conditions; update
the read and write implementations to never panic: in read(&self, ...) replace
unreachable!() with a benign no-op (e.g., return early or return a Result/Ok) so
guest reads do not abort, and in write(&self, ...) validate data.len() and
gracefully ignore or return an error for unexpected sizes instead of assert_eq!.
Replace direct unwraps on data.try_into() and self.dev.lock().unwrap() with
proper error handling (match or if-let) and propagate or log errors; similarly
handle errors returned by dev.write_reg(ControlRegister::QueueNotify, ...)
instead of unwrapping so guest-crafted input cannot crash the VMM.

}
}
Loading
Loading