From f4ead6fe8b2e30624418aa9f91a609b36f00d6b1 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 27 Nov 2024 14:20:33 +0000 Subject: [PATCH] Use config_generation for safe multi-part config reads. --- src/device/blk.rs | 12 ++++++++---- src/device/console.rs | 10 ++++++---- src/device/net/dev_raw.rs | 3 ++- src/device/socket/vsock.rs | 17 ++++++++++------- src/transport/fake.rs | 5 +++++ src/transport/mmio.rs | 5 +++++ src/transport/mod.rs | 16 ++++++++++++++++ src/transport/pci.rs | 5 +++++ src/transport/some.rs | 7 +++++++ 9 files changed, 64 insertions(+), 16 deletions(-) diff --git a/src/device/blk.rs b/src/device/blk.rs index 1ac4803b..6fc9d252 100644 --- a/src/device/blk.rs +++ b/src/device/blk.rs @@ -56,10 +56,14 @@ impl VirtIOBlk { let negotiated_features = transport.begin_init(SUPPORTED_FEATURES); // Read configuration space. - let capacity = transport.read_config_space::(offset_of!(BlkConfig, capacity_low))? - as u64 - | (transport.read_config_space::(offset_of!(BlkConfig, capacity_high))? as u64) - << 32; + let capacity = transport.read_consistent(|| { + Ok( + transport.read_config_space::(offset_of!(BlkConfig, capacity_low))? as u64 + | (transport.read_config_space::(offset_of!(BlkConfig, capacity_high))? + as u64) + << 32, + ) + })?; info!("found a block device of size {}KB", capacity / 2); let queue = VirtQueue::new( diff --git a/src/device/console.rs b/src/device/console.rs index 37585517..a8d6095a 100644 --- a/src/device/console.rs +++ b/src/device/console.rs @@ -127,10 +127,12 @@ impl VirtIOConsole { /// Returns the size of the console, if the device supports reporting this. pub fn size(&self) -> Result> { if self.negotiated_features.contains(Features::SIZE) { - Ok(Some(Size { - columns: self.transport.read_config_space(offset_of!(Config, cols))?, - rows: self.transport.read_config_space(offset_of!(Config, rows))?, - })) + self.transport.read_consistent(|| { + Ok(Some(Size { + columns: self.transport.read_config_space(offset_of!(Config, cols))?, + rows: self.transport.read_config_space(offset_of!(Config, rows))?, + })) + }) } else { Ok(None) } diff --git a/src/device/net/dev_raw.rs b/src/device/net/dev_raw.rs index 8f478673..58d30d8d 100644 --- a/src/device/net/dev_raw.rs +++ b/src/device/net/dev_raw.rs @@ -31,7 +31,8 @@ impl VirtIONetRaw(offset_of!(Config, status))?; debug!("Got MAC={:02x?}, status={:?}", mac, status); diff --git a/src/device/socket/vsock.rs b/src/device/socket/vsock.rs index 0566b2e7..5270e82e 100644 --- a/src/device/socket/vsock.rs +++ b/src/device/socket/vsock.rs @@ -247,13 +247,16 @@ impl VirtIOSocket(offset_of!(VirtioVsockConfig, guest_cid_low))? - as u64 - | (transport.read_config_space::(offset_of!(VirtioVsockConfig, guest_cid_high))? - as u64) - << 32; + let guest_cid = transport.read_consistent(|| { + Ok( + transport.read_config_space::(offset_of!(VirtioVsockConfig, guest_cid_low))? + as u64 + | (transport + .read_config_space::(offset_of!(VirtioVsockConfig, guest_cid_high))? + as u64) + << 32, + ) + })?; debug!("guest cid: {guest_cid:?}"); let rx = VirtQueue::new( diff --git a/src/transport/fake.rs b/src/transport/fake.rs index c0aa39b6..0fc00dc7 100644 --- a/src/transport/fake.rs +++ b/src/transport/fake.rs @@ -96,6 +96,10 @@ impl Transport for FakeTransport { pending } + fn read_config_generation(&self) -> u32 { + self.state.lock().unwrap().config_generation + } + fn read_config_space(&self, offset: usize) -> Result { assert!(align_of::() <= 4, "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.", @@ -133,6 +137,7 @@ pub struct State { pub guest_page_size: u32, pub interrupt_pending: bool, pub queues: Vec, + pub config_generation: u32, } impl State { diff --git a/src/transport/mmio.rs b/src/transport/mmio.rs index f70a404f..50d77c6c 100644 --- a/src/transport/mmio.rs +++ b/src/transport/mmio.rs @@ -497,6 +497,11 @@ impl Transport for MmioTransport { } } + fn read_config_generation(&self) -> u32 { + // SAFETY: self.header points to a valid VirtIO MMIO region. + unsafe { volread!(self.header, config_generation) } + } + fn read_config_space(&self, offset: usize) -> Result { assert!(align_of::() <= 4, "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.", diff --git a/src/transport/mod.rs b/src/transport/mod.rs index 3e8a0990..b9fb7dfa 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -101,6 +101,9 @@ pub trait Transport { ); } + /// Reads the configuration space generation. + fn read_config_generation(&self) -> u32; + /// Reads a value from the device config space. fn read_config_space(&self, offset: usize) -> Result; @@ -110,6 +113,19 @@ pub trait Transport { offset: usize, value: T, ) -> Result<()>; + + /// Safely reads multiple fields from config space by ensuring that the config generation is the + /// same before and after all reads, and retrying if not. + fn read_consistent(&self, f: impl Fn() -> Result) -> Result { + loop { + let before = self.read_config_generation(); + let result = f(); + let after = self.read_config_generation(); + if before == after { + break result; + } + } + } } bitflags! { diff --git a/src/transport/pci.rs b/src/transport/pci.rs index 59bd78d1..044ac9e0 100644 --- a/src/transport/pci.rs +++ b/src/transport/pci.rs @@ -326,6 +326,11 @@ impl Transport for PciTransport { isr_status & 0x3 != 0 } + fn read_config_generation(&self) -> u32 { + // SAFETY: self.header points to a valid VirtIO MMIO region. + unsafe { volread!(self.common_cfg, config_generation) }.into() + } + fn read_config_space(&self, offset: usize) -> Result { assert!(align_of::() <= 4, "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.", diff --git a/src/transport/some.rs b/src/transport/some.rs index 7da3f9f3..1d824b13 100644 --- a/src/transport/some.rs +++ b/src/transport/some.rs @@ -123,6 +123,13 @@ impl Transport for SomeTransport { } } + fn read_config_generation(&self) -> u32 { + match self { + Self::Mmio(mmio) => mmio.read_config_generation(), + Self::Pci(pci) => pci.read_config_generation(), + } + } + fn read_config_space(&self, offset: usize) -> Result { match self { Self::Mmio(mmio) => mmio.read_config_space(offset),