From f589404f881bceb1d9a8f51077aa848ec3319f97 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 12 Jun 2024 10:49:26 +0100 Subject: [PATCH 1/8] Only read as much data from input config space as is available. The size of the data available for a particular selector is given by the device, so only read that much data rather than the full 128 bytes every time. --- src/device/input.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/device/input.rs b/src/device/input.rs index f8ee95a1..b3de875b 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -4,10 +4,10 @@ use super::common::Feature; use crate::hal::Hal; use crate::queue::VirtQueue; use crate::transport::Transport; -use crate::volatile::{volread, volwrite, ReadOnly, WriteOnly}; +use crate::volatile::{volread, volwrite, ReadOnly, VolatileReadable, WriteOnly}; use crate::Result; use alloc::boxed::Box; -use core::ptr::NonNull; +use core::ptr::{addr_of, NonNull}; use zerocopy::{AsBytes, FromBytes, FromZeroes}; /// Virtual human interface devices such as keyboards, mice and tablets. @@ -107,15 +107,16 @@ impl VirtIOInput { out: &mut [u8], ) -> u8 { let size; - let data; // Safe because config points to a valid MMIO region for the config space. unsafe { volwrite!(self.config, select, select as u8); volwrite!(self.config, subsel, subsel); size = volread!(self.config, size); - data = volread!(self.config, data); + for i in 0..size { + let i = usize::from(i); + out[i] = addr_of!((*self.config.as_ptr()).data[i]).vread(); + } } - out[..size as usize].copy_from_slice(&data[..size as usize]); size } } @@ -172,7 +173,7 @@ struct Config { subsel: WriteOnly, size: ReadOnly, _reversed: [ReadOnly; 5], - data: ReadOnly<[u8; 128]>, + data: [ReadOnly; 128], } #[repr(C)] From 99f969c09e202fd548571d5cf9682e2356965c68 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 12 Jun 2024 11:49:48 +0100 Subject: [PATCH 2/8] If buffer is too small, just use the first part of it. --- src/device/input.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/device/input.rs b/src/device/input.rs index b3de875b..be073b3b 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -7,6 +7,7 @@ use crate::transport::Transport; use crate::volatile::{volread, volwrite, ReadOnly, VolatileReadable, WriteOnly}; use crate::Result; use alloc::boxed::Box; +use core::cmp::min; use core::ptr::{addr_of, NonNull}; use zerocopy::{AsBytes, FromBytes, FromZeroes}; @@ -112,8 +113,8 @@ impl VirtIOInput { volwrite!(self.config, select, select as u8); volwrite!(self.config, subsel, subsel); size = volread!(self.config, size); - for i in 0..size { - let i = usize::from(i); + let size_to_copy = min(usize::from(size), out.len()); + for i in 0..size_to_copy { out[i] = addr_of!((*self.config.as_ptr()).data[i]).vread(); } } From 9214cf351961271906bf17623c53393732355ecc Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 12 Jun 2024 11:54:42 +0100 Subject: [PATCH 3/8] Add typed getters for input config. --- src/device/input.rs | 119 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 103 insertions(+), 16 deletions(-) diff --git a/src/device/input.rs b/src/device/input.rs index be073b3b..8060fa9f 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -5,9 +5,13 @@ use crate::hal::Hal; use crate::queue::VirtQueue; use crate::transport::Transport; use crate::volatile::{volread, volwrite, ReadOnly, VolatileReadable, WriteOnly}; -use crate::Result; -use alloc::boxed::Box; +use crate::Error; +use alloc::{ + boxed::Box, + string::{FromUtf8Error, String}, +}; use core::cmp::min; +use core::mem::size_of; use core::ptr::{addr_of, NonNull}; use zerocopy::{AsBytes, FromBytes, FromZeroes}; @@ -26,7 +30,7 @@ pub struct VirtIOInput { impl VirtIOInput { /// Create a new VirtIO-Input driver. - pub fn new(mut transport: T) -> Result { + pub fn new(mut transport: T) -> Result { let mut event_buf = Box::new([InputEvent::default(); QUEUE_SIZE]); let negotiated_features = transport.begin_init(SUPPORTED_FEATURES); @@ -120,6 +124,78 @@ impl VirtIOInput { } size } + + /// Queries a specific piece of information by `select` and `subsel`, allocates a sufficiently + /// large byte buffer for it, and returns it. + fn query_config_select_alloc(&mut self, select: InputConfigSelect, subsel: u8) -> Box<[u8]> { + // Safe because config points to a valid MMIO region for the config space. + unsafe { + volwrite!(self.config, select, select as u8); + volwrite!(self.config, subsel, subsel); + let size = usize::from(volread!(self.config, size)); + let mut buf = u8::new_box_slice_zeroed(size); + for i in 0..size { + buf[i] = addr_of!((*self.config.as_ptr()).data[i]).vread(); + } + buf + } + } + + /// Queries a specific piece of information by `select` and `subsel` into a newly-allocated + /// buffer, and tries to convert it to a string. + /// + /// Returns an error if it is not valid UTF-8. + fn query_config_string( + &mut self, + select: InputConfigSelect, + subsel: u8, + ) -> Result { + String::from_utf8(self.query_config_select_alloc(select, subsel).into()) + } + + /// Queries and returns the name of the device, or an error if it is not valid UTF-8. + pub fn name(&mut self) -> Result { + self.query_config_string(InputConfigSelect::IdName, 0) + } + + /// Queries and returns the serial number of the device, or an error if it is not valid UTF-8. + pub fn serial_number(&mut self) -> Result { + self.query_config_string(InputConfigSelect::IdSerial, 0) + } + + /// Queries and returns the ID information of the device. + pub fn ids(&mut self) -> Result { + let mut ids = DevIDs::default(); + let size = self.query_config_select(InputConfigSelect::IdDevids, 0, ids.as_bytes_mut()); + if usize::from(size) == size_of::() { + Ok(ids) + } else { + Err(Error::InvalidParam) + } + } + + /// Queries and returns the input properties of the device. + pub fn prop_bits(&mut self) -> Box<[u8]> { + self.query_config_select_alloc(InputConfigSelect::PropBits, 0) + } + + /// Queries and returns a bitmap of supported event codes for the given event type. + /// + /// If the event type is not supported an empty slice will be returned. + pub fn ev_bits(&mut self, event_type: u8) -> Box<[u8]> { + self.query_config_select_alloc(InputConfigSelect::EvBits, event_type) + } + + /// Queries and returns information about the given axis of the device. + pub fn abs_info(&mut self, axis: u8) -> Result { + let mut info = AbsInfo::default(); + let size = self.query_config_select(InputConfigSelect::AbsInfo, axis, info.as_bytes_mut()); + if usize::from(size) == size_of::() { + Ok(info) + } else { + Err(Error::InvalidParam) + } + } } // SAFETY: The config space can be accessed from any thread. @@ -177,23 +253,34 @@ struct Config { data: [ReadOnly; 128], } +/// Information about an axis of an input device, typically a joystick. #[repr(C)] -#[derive(Debug)] -struct AbsInfo { - min: u32, - max: u32, - fuzz: u32, - flat: u32, - res: u32, +#[derive(AsBytes, Clone, Debug, Default, Eq, PartialEq, FromBytes, FromZeroes)] +pub struct AbsInfo { + /// The minimum value for the axis. + pub min: u32, + /// The maximum value for the axis. + pub max: u32, + /// The fuzz value used to filter noise from the event stream. + pub fuzz: u32, + /// The size of the dead zone; values less than this will be reported as 0. + pub flat: u32, + /// The resolution for values reported for the axis. + pub res: u32, } +/// The identifiers of a VirtIO input device. #[repr(C)] -#[derive(Debug)] -struct DevIDs { - bustype: u16, - vendor: u16, - product: u16, - version: u16, +#[derive(AsBytes, Clone, Debug, Default, Eq, PartialEq, FromBytes, FromZeroes)] +pub struct DevIDs { + /// The bustype identifier. + pub bustype: u16, + /// The vendor identifier. + pub vendor: u16, + /// The product identifier. + pub product: u16, + /// The version identifier. + pub version: u16, } /// Both queues use the same `virtio_input_event` struct. `type`, `code` and `value` From 75b17bd9bb9cea4074d44a1df86d74713ae8db6d Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 12 Jun 2024 12:05:41 +0100 Subject: [PATCH 4/8] Volatile test initialisers can be const. --- src/volatile.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/volatile.rs b/src/volatile.rs index b7059d13..84751357 100644 --- a/src/volatile.rs +++ b/src/volatile.rs @@ -5,7 +5,7 @@ pub struct ReadOnly(T); impl ReadOnly { /// Construct a new instance for testing. - pub fn new(value: T) -> Self { + pub const fn new(value: T) -> Self { Self(value) } } @@ -22,7 +22,7 @@ pub struct Volatile(T); impl Volatile { /// Construct a new instance for testing. - pub fn new(value: T) -> Self { + pub const fn new(value: T) -> Self { Self(value) } } From 627233adf215e00b1aa2839b19c409e177e9ea96 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 12 Jun 2024 12:25:08 +0100 Subject: [PATCH 5/8] Fix name of reserved field. --- src/device/input.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/device/input.rs b/src/device/input.rs index 8060fa9f..82314cf3 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -249,7 +249,7 @@ struct Config { select: WriteOnly, subsel: WriteOnly, size: ReadOnly, - _reversed: [ReadOnly; 5], + _reserved: [ReadOnly; 5], data: [ReadOnly; 128], } From 934a2036c22f992a0d9315ed455659a3513afb34 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 12 Jun 2024 12:25:33 +0100 Subject: [PATCH 6/8] Add tests for input device config space access. --- src/device/input.rs | 89 +++++++++++++++++++++++++++++++++++++++++++++ src/volatile.rs | 4 +- 2 files changed, 91 insertions(+), 2 deletions(-) diff --git a/src/device/input.rs b/src/device/input.rs index 82314cf3..7f0845da 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -302,3 +302,92 @@ const SUPPORTED_FEATURES: Feature = Feature::RING_EVENT_IDX.union(Feature::RING_ // a parameter that can change const QUEUE_SIZE: usize = 32; + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + hal::fake::FakeHal, + transport::{ + fake::{FakeTransport, QueueStatus, State}, + DeviceType, + }, + }; + use alloc::{sync::Arc, vec}; + use core::convert::TryInto; + use std::sync::Mutex; + + #[test] + fn config() { + const DEFAULT_DATA: ReadOnly = ReadOnly::new(0); + let mut config_space = Config { + select: WriteOnly::default(), + subsel: WriteOnly::default(), + size: ReadOnly::new(0), + _reserved: Default::default(), + data: [DEFAULT_DATA; 128], + }; + let state = Arc::new(Mutex::new(State { + queues: vec![QueueStatus::default(), QueueStatus::default()], + ..Default::default() + })); + let transport = FakeTransport { + device_type: DeviceType::Block, + max_queue_size: QUEUE_SIZE.try_into().unwrap(), + device_features: 0, + config_space: NonNull::from(&mut config_space), + state: state.clone(), + }; + let mut input = VirtIOInput::>::new(transport).unwrap(); + + set_data(&mut config_space, "Test input device".as_bytes()); + assert_eq!(input.name().unwrap(), "Test input device"); + assert_eq!(config_space.select.0, InputConfigSelect::IdName as u8); + assert_eq!(config_space.subsel.0, 0); + + set_data(&mut config_space, "Serial number".as_bytes()); + assert_eq!(input.serial_number().unwrap(), "Serial number"); + assert_eq!(config_space.select.0, InputConfigSelect::IdSerial as u8); + assert_eq!(config_space.subsel.0, 0); + + let ids = DevIDs { + bustype: 0x4242, + product: 0x0067, + vendor: 0x1234, + version: 0x4321, + }; + set_data(&mut config_space, ids.as_bytes()); + assert_eq!(input.ids().unwrap(), ids); + assert_eq!(config_space.select.0, InputConfigSelect::IdDevids as u8); + assert_eq!(config_space.subsel.0, 0); + + set_data(&mut config_space, &[0x12, 0x34, 0x56]); + assert_eq!(input.prop_bits().as_ref(), &[0x12, 0x34, 0x56]); + assert_eq!(config_space.select.0, InputConfigSelect::PropBits as u8); + assert_eq!(config_space.subsel.0, 0); + + set_data(&mut config_space, &[0x42, 0x66]); + assert_eq!(input.ev_bits(3).as_ref(), &[0x42, 0x66]); + assert_eq!(config_space.select.0, InputConfigSelect::EvBits as u8); + assert_eq!(config_space.subsel.0, 3); + + let abs_info = AbsInfo { + min: 12, + max: 1234, + fuzz: 4, + flat: 10, + res: 2, + }; + set_data(&mut config_space, abs_info.as_bytes()); + assert_eq!(input.abs_info(5).unwrap(), abs_info); + assert_eq!(config_space.select.0, InputConfigSelect::AbsInfo as u8); + assert_eq!(config_space.subsel.0, 5); + } + + fn set_data(config_space: &mut Config, value: &[u8]) { + config_space.size.0 = value.len().try_into().unwrap(); + for (i, &byte) in value.into_iter().enumerate() { + config_space.data[i].0 = byte; + } + } +} diff --git a/src/volatile.rs b/src/volatile.rs index 84751357..74f527b3 100644 --- a/src/volatile.rs +++ b/src/volatile.rs @@ -1,7 +1,7 @@ /// An MMIO register which can only be read from. #[derive(Default)] #[repr(transparent)] -pub struct ReadOnly(T); +pub struct ReadOnly(pub(crate) T); impl ReadOnly { /// Construct a new instance for testing. @@ -13,7 +13,7 @@ impl ReadOnly { /// An MMIO register which can only be written to. #[derive(Default)] #[repr(transparent)] -pub struct WriteOnly(T); +pub struct WriteOnly(pub(crate) T); /// An MMIO register which may be both read and written. #[derive(Default)] From faedc28cb8450b16ddb30a9a11ab932faffb995c Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Thu, 20 Jun 2024 10:55:06 +0100 Subject: [PATCH 7/8] Fix clippy lint warnings. --- src/device/input.rs | 4 ++-- src/device/net/mod.rs | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/device/input.rs b/src/device/input.rs index 7f0845da..40f66c3c 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -118,8 +118,8 @@ impl VirtIOInput { volwrite!(self.config, subsel, subsel); size = volread!(self.config, size); let size_to_copy = min(usize::from(size), out.len()); - for i in 0..size_to_copy { - out[i] = addr_of!((*self.config.as_ptr()).data[i]).vread(); + for (i, out_item) in out.iter_mut().take(size_to_copy).enumerate() { + *out_item = addr_of!((*self.config.as_ptr()).data[i]).vread(); } } size diff --git a/src/device/net/mod.rs b/src/device/net/mod.rs index fa13b44b..3b97a182 100644 --- a/src/device/net/mod.rs +++ b/src/device/net/mod.rs @@ -64,7 +64,8 @@ bitflags! { const CTRL_RX = 1 << 18; /// Control channel VLAN filtering. const CTRL_VLAN = 1 << 19; - /// + /// Device supports VIRTIO_NET_CTRL_RX_ALLUNI, VIRTIO_NET_CTRL_RX_NOMULTI, + /// VIRTIO_NET_CTRL_RX_NOUNI and VIRTIO_NET_CTRL_RX_NOBCAST. const CTRL_RX_EXTRA = 1 << 20; /// Driver can send gratuitous packets. const GUEST_ANNOUNCE = 1 << 21; From 97c1d3fd4c76e9f3250aa8036059660b2f3529f8 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Thu, 20 Jun 2024 11:14:23 +0100 Subject: [PATCH 8/8] Check that config data size returned by device is no bigger than its buffer size. --- src/device/input.rs | 42 +++++++++++++++++++++++++----------------- src/lib.rs | 7 +++++++ 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/device/input.rs b/src/device/input.rs index 40f66c3c..ed690764 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -6,10 +6,7 @@ use crate::queue::VirtQueue; use crate::transport::Transport; use crate::volatile::{volread, volwrite, ReadOnly, VolatileReadable, WriteOnly}; use crate::Error; -use alloc::{ - boxed::Box, - string::{FromUtf8Error, String}, -}; +use alloc::{boxed::Box, string::String}; use core::cmp::min; use core::mem::size_of; use core::ptr::{addr_of, NonNull}; @@ -127,17 +124,24 @@ impl VirtIOInput { /// Queries a specific piece of information by `select` and `subsel`, allocates a sufficiently /// large byte buffer for it, and returns it. - fn query_config_select_alloc(&mut self, select: InputConfigSelect, subsel: u8) -> Box<[u8]> { + fn query_config_select_alloc( + &mut self, + select: InputConfigSelect, + subsel: u8, + ) -> Result, Error> { // Safe because config points to a valid MMIO region for the config space. unsafe { volwrite!(self.config, select, select as u8); volwrite!(self.config, subsel, subsel); let size = usize::from(volread!(self.config, size)); + if size > CONFIG_DATA_MAX_LENGTH { + return Err(Error::IoError); + } let mut buf = u8::new_box_slice_zeroed(size); for i in 0..size { buf[i] = addr_of!((*self.config.as_ptr()).data[i]).vread(); } - buf + Ok(buf) } } @@ -149,17 +153,19 @@ impl VirtIOInput { &mut self, select: InputConfigSelect, subsel: u8, - ) -> Result { - String::from_utf8(self.query_config_select_alloc(select, subsel).into()) + ) -> Result { + Ok(String::from_utf8( + self.query_config_select_alloc(select, subsel)?.into(), + )?) } /// Queries and returns the name of the device, or an error if it is not valid UTF-8. - pub fn name(&mut self) -> Result { + pub fn name(&mut self) -> Result { self.query_config_string(InputConfigSelect::IdName, 0) } /// Queries and returns the serial number of the device, or an error if it is not valid UTF-8. - pub fn serial_number(&mut self) -> Result { + pub fn serial_number(&mut self) -> Result { self.query_config_string(InputConfigSelect::IdSerial, 0) } @@ -170,19 +176,19 @@ impl VirtIOInput { if usize::from(size) == size_of::() { Ok(ids) } else { - Err(Error::InvalidParam) + Err(Error::IoError) } } /// Queries and returns the input properties of the device. - pub fn prop_bits(&mut self) -> Box<[u8]> { + pub fn prop_bits(&mut self) -> Result, Error> { self.query_config_select_alloc(InputConfigSelect::PropBits, 0) } /// Queries and returns a bitmap of supported event codes for the given event type. /// /// If the event type is not supported an empty slice will be returned. - pub fn ev_bits(&mut self, event_type: u8) -> Box<[u8]> { + pub fn ev_bits(&mut self, event_type: u8) -> Result, Error> { self.query_config_select_alloc(InputConfigSelect::EvBits, event_type) } @@ -193,7 +199,7 @@ impl VirtIOInput { if usize::from(size) == size_of::() { Ok(info) } else { - Err(Error::InvalidParam) + Err(Error::IoError) } } } @@ -219,6 +225,8 @@ impl Drop for VirtIOInput { } } +const CONFIG_DATA_MAX_LENGTH: usize = 128; + /// Select value used for [`VirtIOInput::query_config_select()`]. #[repr(u8)] #[derive(Debug, Clone, Copy)] @@ -250,7 +258,7 @@ struct Config { subsel: WriteOnly, size: ReadOnly, _reserved: [ReadOnly; 5], - data: [ReadOnly; 128], + data: [ReadOnly; CONFIG_DATA_MAX_LENGTH], } /// Information about an axis of an input device, typically a joystick. @@ -362,12 +370,12 @@ mod tests { assert_eq!(config_space.subsel.0, 0); set_data(&mut config_space, &[0x12, 0x34, 0x56]); - assert_eq!(input.prop_bits().as_ref(), &[0x12, 0x34, 0x56]); + assert_eq!(input.prop_bits().unwrap().as_ref(), &[0x12, 0x34, 0x56]); assert_eq!(config_space.select.0, InputConfigSelect::PropBits as u8); assert_eq!(config_space.subsel.0, 0); set_data(&mut config_space, &[0x42, 0x66]); - assert_eq!(input.ev_bits(3).as_ref(), &[0x42, 0x66]); + assert_eq!(input.ev_bits(3).unwrap().as_ref(), &[0x42, 0x66]); assert_eq!(config_space.select.0, InputConfigSelect::EvBits as u8); assert_eq!(config_space.subsel.0, 3); diff --git a/src/lib.rs b/src/lib.rs index f2f2f12b..2fa6ae1c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -95,6 +95,13 @@ pub enum Error { SocketDeviceError(device::socket::SocketError), } +#[cfg(feature = "alloc")] +impl From for Error { + fn from(_value: alloc::string::FromUtf8Error) -> Self { + Self::IoError + } +} + impl Display for Error { fn fmt(&self, f: &mut Formatter) -> fmt::Result { match self {