From 13f42ab376758d42d64c4baa64101d197abcc378 Mon Sep 17 00:00:00 2001 From: Dylan Steele Date: Tue, 4 Aug 2026 22:34:43 -0400 Subject: [PATCH 1/2] virtio/console: reject unknown control port IDs Guest control messages currently index ports and derived queues without validating the supplied port ID. Reject unknown PORT_READY and PORT_OPEN IDs before any port or queue access so malformed guest input cannot panic the VMM or change console state. Complete driver-to-device control chains with a used length of zero because the device does not write into them. Assisted-by: Codex:gpt-5.6-sol Signed-off-by: Dylan Steele --- src/devices/src/virtio/console/device.rs | 108 +++++++++++++++++++++-- 1 file changed, 100 insertions(+), 8 deletions(-) diff --git a/src/devices/src/virtio/console/device.rs b/src/devices/src/virtio/console/device.rs index cd55ee6fb8..35c58bfc13 100644 --- a/src/devices/src/virtio/console/device.rs +++ b/src/devices/src/virtio/console/device.rs @@ -1,7 +1,7 @@ use std::cmp; use std::io::Write; use std::iter::zip; -use std::mem::{size_of, size_of_val}; +use std::mem::size_of; #[cfg(unix)] use std::os::unix::io::{AsRawFd, RawFd}; use std::sync::Arc; @@ -188,10 +188,7 @@ impl Console { continue; } }; - if let Err(e) = control_tx - .queue - .add_used(mem, head.index, size_of_val(&cmd) as u32) - { + if let Err(e) = control_tx.queue.add_used(mem, head.index, 0) { error!("failed to add used elements to the queue: {e:?}"); } @@ -207,12 +204,17 @@ impl Console { } } control_event::VIRTIO_CONSOLE_PORT_READY => { + let Some(port) = self.ports.get(cmd.id as usize) else { + log::warn!("Guest reported unknown console port {} ready", cmd.id); + continue; + }; + if cmd.value != 1 { log::error!("Port initialization failed: {cmd:?}"); continue; } - if let Some(term) = self.ports[cmd.id as usize].terminal() { + if let Some(term) = port.terminal() { self.control.mark_console_port(mem, cmd.id); self.control.port_open(cmd.id, true); let (cols, rows) = term.get_win_size(); @@ -224,13 +226,20 @@ impl Console { self.control.port_open(cmd.id, true) } - let name = self.ports[cmd.id as usize].name(); + let name = port.name(); log::trace!("Port ready {id}: {name}", id = cmd.id); if !name.is_empty() { self.control.port_name(cmd.id, name) } } control_event::VIRTIO_CONSOLE_PORT_OPEN => { + let Some(port_id) = + ((cmd.id as usize) < self.ports.len()).then_some(cmd.id as usize) + else { + log::warn!("Guest reported unknown console port {} open", cmd.id); + continue; + }; + let opened = match cmd.value { 0 => false, 1 => true, @@ -249,7 +258,7 @@ impl Console { continue; } - ports_to_start.push(cmd.id as usize); + ports_to_start.push(port_id); } _ => log::warn!("Unknown console control event {:x}", cmd.event), } @@ -370,3 +379,86 @@ impl VmmExitObserver for Console { log::trace!("Console on_vmm_exit finished"); } } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use utils::eventfd::EventFd; + use vm_memory::{Bytes, GuestAddress, GuestMemoryMmap}; + + use super::*; + use crate::legacy::DummyIrqChip; + use crate::virtio::queue::tests::VirtQueue; + + const QUEUE_SIZE: u16 = 8; + const CONTROL_ADDR: u64 = 0x4000; + + fn process_control_command(command: VirtioConsoleControl) -> (Console, u16, u32, u32) { + let mem = GuestMemoryMmap::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap(); + let queues = [0, 0x400, 0x800, 0xc00] + .map(|start| VirtQueue::new(GuestAddress(start), &mem, QUEUE_SIZE)); + + mem.write_obj(command, GuestAddress(CONTROL_ADDR)).unwrap(); + queues[CONTROL_TXQ_INDEX].dtable[0].set( + CONTROL_ADDR, + size_of::() as u32, + 0, + 0, + ); + queues[CONTROL_TXQ_INDEX].avail.ring[0].set(0); + queues[CONTROL_TXQ_INDEX].avail.idx.set(1); + + let device_queues = queues + .iter() + .map(|queue| { + DeviceQueue::new( + queue.create_queue(), + Arc::new(EventFd::new(0).expect("create queue event")), + ) + }) + .collect(); + let interrupt = InterruptTransport::new(DummyIrqChip::new().into(), "test".into()) + .expect("create interrupt transport"); + let mut console = Console::new(vec![PortDescription { + name: "test".into(), + input: None, + output: None, + terminal: None, + }]) + .unwrap(); + console + .activate(mem.clone(), interrupt, device_queues) + .unwrap(); + + assert!(console.process_control_tx()); + assert!(!console.process_control_tx()); + + let used_index = queues[CONTROL_TXQ_INDEX].used.idx.get(); + let used = queues[CONTROL_TXQ_INDEX].used.ring[0].get(); + (console, used_index, used.id, used.len) + } + + #[test] + fn unknown_port_ids_are_completed_without_side_effects() { + for event in [ + control_event::VIRTIO_CONSOLE_PORT_READY, + control_event::VIRTIO_CONSOLE_PORT_OPEN, + ] { + for id in [1, u32::MAX] { + let command = VirtioConsoleControl { + id, + event, + value: 1, + }; + let (console, used_index, used_id, used_len) = process_control_command(command); + + assert_eq!(used_index, 1); + assert_eq!(used_id, 0); + assert_eq!(used_len, 0); + assert!(console.queues.iter().all(Option::is_some)); + assert!(console.control.queue_pop().is_none()); + } + } + } +} From 740236a62d2c7c9cefe773dd4e951ea38950f6e1 Mon Sep 17 00:00:00 2001 From: Dylan Steele Date: Tue, 4 Aug 2026 22:39:22 -0400 Subject: [PATCH 2/2] virtio/console: validate control transmit chains The control transmit path currently reads an object from the head address without validating descriptor direction, declared length, chained layout, or guest memory ranges. That can read outside the driver-declared buffer and leave unreadable elements stranded. Validate the complete readable chain, accept scatter/gather layouts totaling exactly one control message, reject writable or malformed chains, and parse through the established descriptor Reader. Return every popped chain once with no device-written bytes. Assisted-by: Codex:gpt-5.6-sol Signed-off-by: Dylan Steele --- src/devices/src/virtio/console/device.rs | 255 ++++++++++++++++++++--- 1 file changed, 224 insertions(+), 31 deletions(-) diff --git a/src/devices/src/virtio/console/device.rs b/src/devices/src/virtio/console/device.rs index 35c58bfc13..636ea76552 100644 --- a/src/devices/src/virtio/console/device.rs +++ b/src/devices/src/virtio/console/device.rs @@ -23,11 +23,78 @@ use crate::virtio::console::port::Port; use crate::virtio::console::port_queue_mapping::{ QueueDirection, num_queues, port_id_to_queue_idx, }; -use crate::virtio::{InterruptTransport, PortDescription, VmmExitObserver}; +use crate::virtio::descriptor_utils::Reader; +use crate::virtio::queue::{VIRTQ_DESC_F_NEXT, VIRTQ_DESC_F_WRITE}; +use crate::virtio::{DescriptorChain, InterruptTransport, PortDescription, VmmExitObserver}; pub(crate) const CONTROL_RXQ_INDEX: usize = 2; pub(crate) const CONTROL_TXQ_INDEX: usize = 3; +fn read_control_command( + mem: &GuestMemoryMmap, + head: DescriptorChain<'_>, +) -> Option { + let mut descriptor = Some(head.clone()); + let mut descriptor_indices = Vec::new(); + let mut readable_len = 0usize; + + while let Some(current) = descriptor { + if descriptor_indices.contains(¤t.index) { + log::error!("Console control descriptor chain contains a loop"); + return None; + } + descriptor_indices.push(current.index); + + if current.flags & !(VIRTQ_DESC_F_NEXT | VIRTQ_DESC_F_WRITE) != 0 { + log::error!("Console control transmit chain uses unsupported descriptor flags"); + return None; + } + + if current.is_write_only() { + log::error!("Console control transmit chain contains a writable descriptor"); + return None; + } + + readable_len = match readable_len.checked_add(current.len as usize) { + Some(len) => len, + None => { + log::error!("Console control descriptor length overflow"); + return None; + } + }; + + let has_next = current.flags & VIRTQ_DESC_F_NEXT != 0; + descriptor = current.next_descriptor(); + if has_next && descriptor.is_none() { + log::error!("Malformed console control descriptor chain"); + return None; + } + } + + if readable_len != size_of::() { + log::error!( + "Invalid console control payload length: expected {}, got {readable_len}", + size_of::() + ); + return None; + } + + let mut reader = match Reader::new(mem, head) { + Ok(reader) => reader, + Err(e) => { + log::error!("Invalid console control descriptor memory: {e}"); + return None; + } + }; + match reader.read_obj() { + Ok(command) => Some(command), + Err(e) => { + log::error!("Failed to read console control payload: {e}"); + None + } + } +} + pub(crate) const AVAIL_FEATURES: u64 = (1 << uapi::VIRTIO_CONSOLE_F_SIZE as u64) | (1 << uapi::VIRTIO_CONSOLE_F_MULTIPORT as u64) | (1 << uapi::VIRTIO_F_VERSION_1 as u64); @@ -177,20 +244,14 @@ impl Console { while let Some(head) = control_tx.queue.pop(mem) { raise_irq = true; - let cmd: VirtioConsoleControl = match mem.read_obj(head.addr) { - Ok(cmd) => cmd, - Err(e) => { - log::error!( - "Failed to read VirtioConsoleControl struct: {e:?}, struct len = {len}, head.len = {head_len}", - len = size_of::(), - head_len = head.len, - ); - continue; - } - }; - if let Err(e) = control_tx.queue.add_used(mem, head.index, 0) { + let head_index = head.index; + let cmd = read_control_command(mem, head); + if let Err(e) = control_tx.queue.add_used(mem, head_index, 0) { error!("failed to add used elements to the queue: {e:?}"); } + let Some(cmd) = cmd else { + continue; + }; log::trace!("VirtioConsoleControl cmd: {cmd:?}"); match cmd.event { @@ -389,23 +450,46 @@ mod tests { use super::*; use crate::legacy::DummyIrqChip; + use crate::virtio::console::console_control::Payload; use crate::virtio::queue::tests::VirtQueue; + use crate::virtio::queue::{VIRTQ_DESC_F_NEXT, VIRTQ_DESC_F_WRITE}; const QUEUE_SIZE: u16 = 8; const CONTROL_ADDR: u64 = 0x4000; - fn process_control_command(command: VirtioConsoleControl) -> (Console, u16, u32, u32) { + struct ProcessResult { + console: Console, + raised_irq: bool, + used_index: u16, + used_id: u32, + used_len: u32, + } + + fn process_control_chain( + command: VirtioConsoleControl, + descriptors: &[(u64, u32, u16, u16)], + ) -> ProcessResult { let mem = GuestMemoryMmap::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap(); let queues = [0, 0x400, 0x800, 0xc00] .map(|start| VirtQueue::new(GuestAddress(start), &mem, QUEUE_SIZE)); - mem.write_obj(command, GuestAddress(CONTROL_ADDR)).unwrap(); - queues[CONTROL_TXQ_INDEX].dtable[0].set( - CONTROL_ADDR, - size_of::() as u32, - 0, - 0, - ); + let command_bytes = command.as_slice(); + let mut command_offset = 0; + for (index, &(addr, len, flags, next)) in descriptors.iter().enumerate() { + queues[CONTROL_TXQ_INDEX].dtable[index].set(addr, len, flags, next); + if flags & VIRTQ_DESC_F_WRITE == 0 && command_offset < command_bytes.len() { + let write_len = usize::min(len as usize, command_bytes.len() - command_offset); + if mem + .write( + &command_bytes[command_offset..command_offset + write_len], + GuestAddress(addr), + ) + .is_ok() + { + command_offset += write_len; + } + } + } queues[CONTROL_TXQ_INDEX].avail.ring[0].set(0); queues[CONTROL_TXQ_INDEX].avail.idx.set(1); @@ -431,12 +515,19 @@ mod tests { .activate(mem.clone(), interrupt, device_queues) .unwrap(); - assert!(console.process_control_tx()); + let raised_irq = console.process_control_tx(); + let used_index = queues[CONTROL_TXQ_INDEX].used.idx.get(); assert!(!console.process_control_tx()); + assert_eq!(queues[CONTROL_TXQ_INDEX].used.idx.get(), used_index); - let used_index = queues[CONTROL_TXQ_INDEX].used.idx.get(); let used = queues[CONTROL_TXQ_INDEX].used.ring[0].get(); - (console, used_index, used.id, used.len) + ProcessResult { + console, + raised_irq, + used_index, + used_id: used.id, + used_len: used.len, + } } #[test] @@ -451,14 +542,116 @@ mod tests { event, value: 1, }; - let (console, used_index, used_id, used_len) = process_control_command(command); - - assert_eq!(used_index, 1); - assert_eq!(used_id, 0); - assert_eq!(used_len, 0); - assert!(console.queues.iter().all(Option::is_some)); - assert!(console.control.queue_pop().is_none()); + let result = process_control_chain( + command, + &[(CONTROL_ADDR, size_of::() as u32, 0, 0)], + ); + + assert!(result.raised_irq); + assert_eq!(result.used_index, 1); + assert_eq!(result.used_id, 0); + assert_eq!(result.used_len, 0); + assert!(result.console.queues.iter().all(Option::is_some)); + assert!(result.console.control.queue_pop().is_none()); } } } + + #[test] + fn valid_control_chains_are_processed() { + let command = VirtioConsoleControl { + id: 0, + event: control_event::VIRTIO_CONSOLE_DEVICE_READY, + value: 1, + }; + let chains = [ + ("contiguous", vec![(CONTROL_ADDR, 8, 0, 0)]), + ( + "split", + vec![ + (CONTROL_ADDR, 3, VIRTQ_DESC_F_NEXT, 1), + (CONTROL_ADDR + 0x100, 5, 0, 0), + ], + ), + ]; + + for (name, chain) in chains { + let result = process_control_chain(command, &chain); + + assert!(result.raised_irq, "{name}"); + assert_eq!(result.used_index, 1, "{name}"); + assert_eq!(result.used_id, 0, "{name}"); + assert_eq!(result.used_len, 0, "{name}"); + let Some(Payload::ConsoleControl(response)) = result.console.control.queue_pop() else { + panic!("expected port-add response"); + }; + let response_id = response.id; + let response_event = response.event; + assert_eq!(response_id, 0); + assert_eq!(response_event, control_event::VIRTIO_CONSOLE_PORT_ADD); + assert!(result.console.control.queue_pop().is_none()); + } + } + + #[test] + fn invalid_control_chains_are_completed_without_side_effects() { + let command = VirtioConsoleControl { + id: 0, + event: control_event::VIRTIO_CONSOLE_PORT_READY, + value: 1, + }; + let chains = [ + ("short", vec![(CONTROL_ADDR, 7, 0, 0)]), + ("trailing payload", vec![(CONTROL_ADDR, 9, 0, 0)]), + ("write only", vec![(CONTROL_ADDR, 8, VIRTQ_DESC_F_WRITE, 0)]), + ( + "mixed direction", + vec![ + (CONTROL_ADDR, 4, VIRTQ_DESC_F_NEXT, 1), + (CONTROL_ADDR + 0x100, 4, VIRTQ_DESC_F_WRITE, 0), + ], + ), + ("invalid first range", vec![(0x10000, 8, 0, 0)]), + ( + "invalid second range", + vec![(CONTROL_ADDR, 4, VIRTQ_DESC_F_NEXT, 1), (0x10000, 4, 0, 0)], + ), + ( + "loop", + vec![ + (CONTROL_ADDR, 4, VIRTQ_DESC_F_NEXT, 1), + (CONTROL_ADDR + 0x100, 4, VIRTQ_DESC_F_NEXT, 0), + ], + ), + ("oversized length", vec![(CONTROL_ADDR, u32::MAX, 0, 0)]), + ("unsupported flags", vec![(CONTROL_ADDR, 8, 4, 0)]), + ]; + + for (name, chain) in chains { + let result = process_control_chain(command, &chain); + + assert!(result.raised_irq, "{name}"); + assert_eq!(result.used_index, 1, "{name}"); + assert_eq!(result.used_id, 0, "{name}"); + assert_eq!(result.used_len, 0, "{name}"); + assert!(result.console.queues.iter().all(Option::is_some), "{name}"); + assert!(result.console.control.queue_pop().is_none(), "{name}"); + } + } + + #[test] + fn malformed_control_head_has_no_side_effects() { + let command = VirtioConsoleControl { + id: 0, + event: control_event::VIRTIO_CONSOLE_PORT_READY, + value: 1, + }; + let result = + process_control_chain(command, &[(CONTROL_ADDR, 8, VIRTQ_DESC_F_NEXT, QUEUE_SIZE)]); + + assert!(!result.raised_irq); + assert_eq!(result.used_index, 0); + assert!(result.console.queues.iter().all(Option::is_some)); + assert!(result.console.control.queue_pop().is_none()); + } }