devices: virtio: add copy_config() helper function

Add a new virtio configuration copying function to replace all of the
slightly varying read_config() and write_config() implementations in our
virtio devices.  This replaces a lot of tricky bounds-checking code with
a single central implementation, simplifying the devices to a single
call to copy_config() in most cases.

The balloon device is also changed to represent its config space as a
DataInit struct to match most other devices and remove several unwrap()
calls.

BUG=None
TEST=./build_test
TEST=Boot vm_kernel+vm_rootfs in crosvm
TEST=Start Crostini on nami

Change-Id: Ia49bd6dbe609d17455b9562086bc0b24f327be3f
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/1749562
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>

8 files changed, 89 insertions, 148 deletions

diff --git a/devices/src/virtio/balloon.rs b/devices/src/virtio/balloon.rs
index 5dc0967..45deffb 100644
--- a/devices/src/virtio/balloon.rs
+++ b/devices/src/virtio/balloon.rs
@@ -3,15 +3,13 @@
 // found in the LICENSE file.
 
 use std;
-use std::cmp;
 use std::fmt::{self, Display};
-use std::io::Write;
 use std::os::unix::io::{AsRawFd, RawFd};
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::sync::Arc;
 use std::thread;
 
-use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt};
+use data_model::{DataInit, Le32};
 use msg_socket::MsgReceiver;
 use sys_util::{
     self, error, info, warn, EventFd, GuestAddress, GuestMemory, PollContext, PollToken,
@@ -19,7 +17,7 @@ use sys_util::{
 use vm_control::{BalloonControlCommand, BalloonControlResponseSocket};
 
 use super::{
-    DescriptorChain, Queue, VirtioDevice, INTERRUPT_STATUS_CONFIG_CHANGED,
+    copy_config, DescriptorChain, Queue, VirtioDevice, INTERRUPT_STATUS_CONFIG_CHANGED,
     INTERRUPT_STATUS_USED_RING, TYPE_BALLOON, VIRTIO_F_VERSION_1,
 };
 
@@ -54,6 +52,17 @@ const VIRTIO_BALLOON_PFN_SHIFT: u32 = 12;
 const VIRTIO_BALLOON_F_MUST_TELL_HOST: u32 = 0; // Tell before reclaiming pages
 const VIRTIO_BALLOON_F_DEFLATE_ON_OOM: u32 = 2; // Deflate balloon on OOM
 
+// virtio_balloon_config is the ballon device configuration space defined by the virtio spec.
+#[derive(Copy, Clone, Debug, Default)]
+#[repr(C)]
+struct virtio_balloon_config {
+    num_pages: Le32,
+    actual: Le32,
+}
+
+// Safe because it only has data and has no implicit padding.
+unsafe impl DataInit for virtio_balloon_config {}
+
 // BalloonConfig is modified by the worker and read from the device thread.
 #[derive(Default)]
 struct BalloonConfig {
@@ -242,6 +251,15 @@ impl Balloon {
             features: 1 << VIRTIO_BALLOON_F_MUST_TELL_HOST | 1 << VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
         })
     }
+
+    fn get_config(&self) -> virtio_balloon_config {
+        let num_pages = self.config.num_pages.load(Ordering::Relaxed) as u32;
+        let actual_pages = self.config.actual_pages.load(Ordering::Relaxed) as u32;
+        virtio_balloon_config {
+            num_pages: num_pages.into(),
+            actual: actual_pages.into(),
+        }
+    }
 }
 
 impl Drop for Balloon {
@@ -266,37 +284,16 @@ impl VirtioDevice for Balloon {
         QUEUE_SIZES
     }
 
-    fn read_config(&self, offset: u64, mut data: &mut [u8]) {
-        if offset >= 8 {
-            return;
-        }
-        let num_pages = self.config.num_pages.load(Ordering::Relaxed) as u32;
-        let actual_pages = self.config.actual_pages.load(Ordering::Relaxed) as u32;
-        let mut config = [0u8; 8];
-        // These writes can't fail as they fit in the declared array so unwrap is fine.
-        (&mut config[0..])
-            .write_u32::<LittleEndian>(num_pages)
-            .unwrap();
-        (&mut config[4..])
-            .write_u32::<LittleEndian>(actual_pages)
-            .unwrap();
-        if let Some(end) = offset.checked_add(data.len() as u64) {
-            // This write can't fail, offset and end are checked against the length of config.
-            data.write_all(&config[offset as usize..cmp::min(end, 8) as usize])
-                .unwrap();
-        }
+    fn read_config(&self, offset: u64, data: &mut [u8]) {
+        copy_config(data, 0, self.get_config().as_slice(), offset);
     }
 
-    fn write_config(&mut self, offset: u64, mut data: &[u8]) {
-        // Only allow writing to `actual` pages from the guest.
-        if offset != 4 || data.len() != 4 {
-            return;
-        }
-        // This read can't fail as it fits in the declared array so unwrap is fine.
-        let new_actual: u32 = data.read_u32::<LittleEndian>().unwrap();
+    fn write_config(&mut self, offset: u64, data: &[u8]) {
+        let mut config = self.get_config();
+        copy_config(config.as_mut_slice(), offset, data, 0);
         self.config
             .actual_pages
-            .store(new_actual as usize, Ordering::Relaxed);
+            .store(config.actual.to_native() as usize, Ordering::Relaxed);
     }
 
     fn features(&self) -> u64 {
diff --git a/devices/src/virtio/block.rs b/devices/src/virtio/block.rs
index 2cf577f..a6d7a89 100644
--- a/devices/src/virtio/block.rs
+++ b/devices/src/virtio/block.rs
@@ -2,10 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-use std::cmp;
 use std::fmt::{self, Display};
-use std::io::{self, Seek, SeekFrom, Write};
-use std::mem::{size_of, size_of_val};
+use std::io::{self, Seek, SeekFrom};
+use std::mem::size_of;
 use std::os::unix::io::{AsRawFd, RawFd};
 use std::result;
 use std::sync::atomic::{AtomicUsize, Ordering};
@@ -27,7 +26,7 @@ use msg_socket::{MsgReceiver, MsgSender};
 use vm_control::{DiskControlCommand, DiskControlResponseSocket, DiskControlResult};
 
 use super::{
-    DescriptorChain, DescriptorError, Queue, Reader, VirtioDevice, Writer,
+    copy_config, DescriptorChain, DescriptorError, Queue, Reader, VirtioDevice, Writer,
     INTERRUPT_STATUS_CONFIG_CHANGED, INTERRUPT_STATUS_USED_RING, TYPE_BLOCK, VIRTIO_F_VERSION_1,
 };
 
@@ -742,23 +741,12 @@ impl<T: 'static + AsRawFd + DiskFile + Send> VirtioDevice for Block<T> {
         QUEUE_SIZES
     }
 
-    fn read_config(&self, offset: u64, mut data: &mut [u8]) {
+    fn read_config(&self, offset: u64, data: &mut [u8]) {
         let config_space = {
             let disk_size = self.disk_size.lock();
             build_config_space(*disk_size)
         };
-        let config_len = size_of_val(&config_space) as u64;
-        if offset >= config_len {
-            return;
-        }
-
-        if let Some(end) = offset.checked_add(data.len() as u64) {
-            let offset = offset as usize;
-            let end = cmp::min(end, config_len) as usize;
-            // This write can't fail, offset and end are checked against config_len.
-            data.write_all(&config_space.as_slice()[offset..end])
-                .unwrap();
-        }
+        copy_config(data, 0, config_space.as_slice(), offset);
     }
 
     fn activate(
@@ -816,6 +804,7 @@ impl<T: 'static + AsRawFd + DiskFile + Send> VirtioDevice for Block<T> {
 #[cfg(test)]
 mod tests {
     use std::fs::{File, OpenOptions};
+    use std::mem::size_of_val;
     use sys_util::GuestAddress;
     use tempfile::TempDir;
 
diff --git a/devices/src/virtio/gpu/mod.rs b/devices/src/virtio/gpu/mod.rs
index 2c09761..135732d 100644
--- a/devices/src/virtio/gpu/mod.rs
+++ b/devices/src/virtio/gpu/mod.rs
@@ -29,8 +29,8 @@ use gpu_display::*;
 use gpu_renderer::{Renderer, RendererFlags};
 
 use super::{
-    resource_bridge::*, AvailIter, Queue, VirtioDevice, INTERRUPT_STATUS_USED_RING, TYPE_GPU,
-    VIRTIO_F_VERSION_1,
+    copy_config, resource_bridge::*, AvailIter, Queue, VirtioDevice, INTERRUPT_STATUS_USED_RING,
+    TYPE_GPU, VIRTIO_F_VERSION_1,
 };
 
 use self::backend::Backend;
@@ -790,23 +790,12 @@ impl VirtioDevice for Gpu {
     }
 
     fn read_config(&self, offset: u64, data: &mut [u8]) {
-        let offset = offset as usize;
-        let len = data.len();
-        let cfg = self.get_config();
-        let cfg_slice = cfg.as_slice();
-        if offset + len <= cfg_slice.len() {
-            data.copy_from_slice(&cfg_slice[offset..offset + len]);
-        }
+        copy_config(data, 0, self.get_config().as_slice(), offset);
     }
 
     fn write_config(&mut self, offset: u64, data: &[u8]) {
-        let offset = offset as usize;
-        let len = data.len();
         let mut cfg = self.get_config();
-        let cfg_slice = cfg.as_mut_slice();
-        if offset + len <= cfg_slice.len() {
-            cfg_slice[offset..offset + len].copy_from_slice(data);
-        }
+        copy_config(cfg.as_mut_slice(), offset, data, 0);
         if (cfg.events_clear.to_native() & VIRTIO_GPU_EVENT_DISPLAY) != 0 {
             self.config_event = false;
         }
diff --git a/devices/src/virtio/input/mod.rs b/devices/src/virtio/input/mod.rs
index 3bed5ef..29152a2 100644
--- a/devices/src/virtio/input/mod.rs
+++ b/devices/src/virtio/input/mod.rs
@@ -16,7 +16,7 @@ use data_model::{DataInit, Le16, Le32};
 use sys_util::{error, warn, EventFd, GuestAddress, GuestMemory, PollContext, PollToken};
 
 use self::event_source::{input_event, EvdevEventSource, EventSource, SocketEventSource};
-use super::{Queue, VirtioDevice, INTERRUPT_STATUS_USED_RING, TYPE_INPUT};
+use super::{copy_config, Queue, VirtioDevice, INTERRUPT_STATUS_USED_RING, TYPE_INPUT};
 use std::collections::BTreeMap;
 use std::fmt::{self, Display};
 use std::io::Read;
@@ -233,8 +233,6 @@ pub struct VirtioInputConfig {
 }
 
 impl VirtioInputConfig {
-    const CONFIG_MEM_SIZE: usize = size_of::<virtio_input_config>();
-
     fn new(
         device_ids: virtio_input_device_ids,
         name: Vec<u8>,
@@ -266,19 +264,6 @@ impl VirtioInputConfig {
         ))
     }
 
-    fn validate_read_offsets(&self, offset: usize, len: usize) -> bool {
-        if offset + len > VirtioInputConfig::CONFIG_MEM_SIZE {
-            error!(
-                "Attempt to read from invalid config range: [{}..{}], valid ranges in [0..{}]",
-                offset,
-                offset + len,
-                VirtioInputConfig::CONFIG_MEM_SIZE
-            );
-            return false;
-        }
-        true
-    }
-
     fn build_config_memory(&self) -> virtio_input_config {
         let mut cfg = virtio_input_config::new();
         cfg.select = self.select;
@@ -322,35 +307,19 @@ impl VirtioInputConfig {
     }
 
     fn read(&self, offset: usize, data: &mut [u8]) {
-        let data_len = data.len();
-        if self.validate_read_offsets(offset, data_len) {
-            let config = self.build_config_memory();
-            data.clone_from_slice(&config.as_slice()[offset..offset + data_len]);
-        }
-    }
-
-    fn validate_write_offsets(&self, offset: usize, len: usize) -> bool {
-        const MAX_WRITABLE_BYTES: usize = 2;
-        if offset + len > MAX_WRITABLE_BYTES {
-            error!(
-                "Attempt to write to invalid config range: [{}..{}], valid ranges in [0..{}]",
-                offset,
-                offset + len,
-                MAX_WRITABLE_BYTES
-            );
-            return false;
-        }
-        true
+        copy_config(
+            data,
+            0,
+            self.build_config_memory().as_slice(),
+            offset as u64,
+        );
     }
 
     fn write(&mut self, offset: usize, data: &[u8]) {
-        let len = data.len();
-        if self.validate_write_offsets(offset, len) {
-            let mut selectors: [u8; 2] = [self.select, self.subsel];
-            selectors[offset..offset + len].clone_from_slice(&data);
-            self.select = selectors[0];
-            self.subsel = selectors[1];
-        }
+        let mut config = self.build_config_memory();
+        copy_config(config.as_mut_slice(), offset as u64, data, 0);
+        self.select = config.select;
+        self.subsel = config.subsel;
     }
 }
 
diff --git a/devices/src/virtio/mod.rs b/devices/src/virtio/mod.rs
index 0970b86..49f4355 100644
--- a/devices/src/virtio/mod.rs
+++ b/devices/src/virtio/mod.rs
@@ -43,6 +43,9 @@ pub use self::virtio_device::*;
 pub use self::virtio_pci_device::*;
 pub use self::wl::*;
 
+use std::cmp;
+use std::convert::TryFrom;
+
 const DEVICE_ACKNOWLEDGE: u32 = 0x01;
 const DEVICE_DRIVER: u32 = 0x02;
 const DEVICE_DRIVER_OK: u32 = 0x04;
@@ -88,3 +91,27 @@ pub fn type_to_str(type_: u32) -> Option<&'static str> {
         _ => return None,
     })
 }
+
+/// Copy virtio device configuration data from a subslice of `src` to a subslice of `dst`.
+/// Unlike std::slice::copy_from_slice(), this function copies as much as possible within
+/// the common subset of the two slices, truncating the requested range instead of
+/// panicking if the slices do not match in size.
+///
+/// `dst_offset` and `src_offset` specify the starting indexes of the `dst` and `src`
+/// slices, respectively; if either index is out of bounds, this function is a no-op
+/// rather than panicking.  This makes it safe to call with arbitrary user-controlled
+/// inputs.
+pub fn copy_config(dst: &mut [u8], dst_offset: u64, src: &[u8], src_offset: u64) {
+    if let Ok(dst_offset) = usize::try_from(dst_offset) {
+        if let Ok(src_offset) = usize::try_from(src_offset) {
+            if let Some(dst_slice) = dst.get_mut(dst_offset..) {
+                if let Some(src_slice) = src.get(src_offset..) {
+                    let len = cmp::min(dst_slice.len(), src_slice.len());
+                    let dst_subslice = &mut dst_slice[0..len];
+                    let src_subslice = &src_slice[0..len];
+                    dst_subslice.copy_from_slice(src_subslice);
+                }
+            }
+        }
+    }
+}
diff --git a/devices/src/virtio/p9.rs b/devices/src/virtio/p9.rs
index a3cb007..6968733 100644
--- a/devices/src/virtio/p9.rs
+++ b/devices/src/virtio/p9.rs
@@ -2,7 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-use std::cmp::min;
 use std::fmt::{self, Display};
 use std::io::{self, Write};
 use std::mem;
@@ -17,7 +16,9 @@ use p9;
 use sys_util::{error, warn, Error as SysError, EventFd, GuestMemory, PollContext, PollToken};
 use virtio_sys::vhost::VIRTIO_F_VERSION_1;
 
-use super::{Queue, Reader, VirtioDevice, Writer, INTERRUPT_STATUS_USED_RING, TYPE_9P};
+use super::{
+    copy_config, Queue, Reader, VirtioDevice, Writer, INTERRUPT_STATUS_USED_RING, TYPE_9P,
+};
 
 const QUEUE_SIZE: u16 = 128;
 const QUEUE_SIZES: &[u16] = &[QUEUE_SIZE];
@@ -222,17 +223,7 @@ impl VirtioDevice for P9 {
     }
 
     fn read_config(&self, offset: u64, data: &mut [u8]) {
-        if offset >= self.config.len() as u64 {
-            // Nothing to see here.
-            return;
-        }
-
-        // The config length cannot be more than ::std::u16::MAX + mem::size_of::<u16>(), which
-        // is significantly smaller than ::std::usize::MAX on the architectures we care about so
-        // if we reach this point then we know that `offset` will fit into a usize.
-        let offset = offset as usize;
-        let len = min(data.len(), self.config.len() - offset);
-        data[..len].copy_from_slice(&self.config[offset..offset + len])
+        copy_config(data, 0, self.config.as_slice(), offset);
     }
 
     fn activate(
diff --git a/devices/src/virtio/pmem.rs b/devices/src/virtio/pmem.rs
index 607ce58..b9e42cc 100644
--- a/devices/src/virtio/pmem.rs
+++ b/devices/src/virtio/pmem.rs
@@ -2,11 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-use std::cmp;
 use std::fmt::{self, Display};
 use std::fs::File;
-use std::io::Write;
-use std::mem::{size_of, size_of_val};
+use std::mem::size_of;
 use std::os::unix::io::{AsRawFd, RawFd};
 use std::result;
 use std::sync::atomic::{AtomicUsize, Ordering};
@@ -21,7 +19,8 @@ use sys_util::{
 use data_model::{DataInit, Le32, Le64};
 
 use super::{
-    DescriptorChain, Queue, VirtioDevice, INTERRUPT_STATUS_USED_RING, TYPE_PMEM, VIRTIO_F_VERSION_1,
+    copy_config, DescriptorChain, Queue, VirtioDevice, INTERRUPT_STATUS_USED_RING, TYPE_PMEM,
+    VIRTIO_F_VERSION_1,
 };
 
 const QUEUE_SIZE: u16 = 256;
@@ -297,22 +296,12 @@ impl VirtioDevice for Pmem {
         1 << VIRTIO_F_VERSION_1
     }
 
-    fn read_config(&self, offset: u64, mut data: &mut [u8]) {
+    fn read_config(&self, offset: u64, data: &mut [u8]) {
         let config = virtio_pmem_config {
             start_address: Le64::from(self.mapping_address.offset()),
             size: Le64::from(self.mapping_size as u64),
         };
-        let config_len = size_of_val(&config) as u64;
-        if offset >= config_len {
-            return;
-        }
-
-        if let Some(end) = offset.checked_add(data.len() as u64) {
-            let offset = offset as usize;
-            let end = cmp::min(end, config_len) as usize;
-            // This write can't fail, offset and end are checked against config_len.
-            data.write_all(&config.as_slice()[offset..end]).unwrap();
-        }
+        copy_config(data, 0, config.as_slice(), offset);
     }
 
     fn activate(
diff --git a/devices/src/virtio/vhost/vsock.rs b/devices/src/virtio/vhost/vsock.rs
index d07b639..a61bce8 100644
--- a/devices/src/virtio/vhost/vsock.rs
+++ b/devices/src/virtio/vhost/vsock.rs
@@ -7,7 +7,7 @@ use std::sync::atomic::AtomicUsize;
 use std::sync::Arc;
 use std::thread;
 
-use byteorder::{ByteOrder, LittleEndian};
+use data_model::{DataInit, Le64};
 
 use ::vhost::Vsock as VhostVsockHandle;
 use sys_util::{error, warn, EventFd, GuestMemory};
@@ -15,7 +15,7 @@ use virtio_sys::vhost;
 
 use super::worker::Worker;
 use super::{Error, Result};
-use crate::virtio::{Queue, VirtioDevice, TYPE_VSOCK};
+use crate::virtio::{copy_config, Queue, VirtioDevice, TYPE_VSOCK};
 
 const QUEUE_SIZE: u16 = 256;
 const NUM_QUEUES: usize = 3;
@@ -116,18 +116,8 @@ impl VirtioDevice for Vsock {
     }
 
     fn read_config(&self, offset: u64, data: &mut [u8]) {
-        match offset {
-            0 if data.len() == 8 => LittleEndian::write_u64(data, self.cid),
-            0 if data.len() == 4 => LittleEndian::write_u32(data, (self.cid & 0xffffffff) as u32),
-            4 if data.len() == 4 => {
-                LittleEndian::write_u32(data, ((self.cid >> 32) & 0xffffffff) as u32)
-            }
-            _ => warn!(
-                "vsock: virtio-vsock received invalid read request of {} bytes at offset {}",
-                data.len(),
-                offset
-            ),
-        }
+        let cid = Le64::from(self.cid);
+        copy_config(data, 0, DataInit::as_slice(&cid), offset);
     }
 
     fn ack_features(&mut self, value: u64) {