From 8b2c7b995b634bbabe325157e269a88af2781ee2 Mon Sep 17 00:00:00 2001
From: Woody Chow <woodychow@google.com>
Date: Thu, 27 Jan 2022 10:15:38 +0900
Subject: [PATCH] virtqueue: DescriptorChain::checked_new returns anyhow::Error

checked_new fails when guest gives invalid descriptor entries.
The error makes debugging easier.

BUG=None
TEST=cargo test

Change-Id: I6c6f5b04e7f39a685d95ca738b9cb01be643fa21
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3418620
Reviewed-by: David Stevens <stevensd@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Woody Chow <woodychow@google.com>
---
 devices/src/virtio/descriptor_utils.rs | 13 +++++----
 devices/src/virtio/queue.rs            | 38 ++++++++++++++++++--------
 2 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/devices/src/virtio/descriptor_utils.rs b/devices/src/virtio/descriptor_utils.rs
index 74a36764ac..7279ee9108 100644
--- a/devices/src/virtio/descriptor_utils.rs
+++ b/devices/src/virtio/descriptor_utils.rs
@@ -12,6 +12,7 @@ use std::ptr::copy_nonoverlapping;
 use std::result;
 use std::sync::Arc;
 
+use anyhow::Context;
 use base::{FileReadWriteAtVolatile, FileReadWriteVolatile};
 use cros_async::MemRegion;
 use data_model::{DataInit, Le16, Le32, Le64, VolatileMemoryError, VolatileSlice};
@@ -30,8 +31,8 @@ pub enum Error {
     DescriptorChainOverflow,
     #[error("descriptor guest memory error: {0}")]
     GuestMemoryError(vm_memory::GuestMemoryError),
-    #[error("invalid descriptor chain")]
-    InvalidChain,
+    #[error("invalid descriptor chain: {0:#}")]
+    InvalidChain(anyhow::Error),
     #[error("descriptor I/O error: {0}")]
     IoError(io::Error),
     #[error("`DescriptorChain` split is out of bounds: {0}")]
@@ -802,18 +803,20 @@ pub fn create_descriptor_chain(
         let offset = size + spaces_between_regions;
         buffers_start_addr = buffers_start_addr
             .checked_add(offset as u64)
-            .ok_or(Error::InvalidChain)?;
+            .context("Invalid buffers_start_addr)")
+            .map_err(Error::InvalidChain)?;
 
         let _ = memory.write_obj_at_addr(
             desc,
             descriptor_array_addr
                 .checked_add(index as u64 * std::mem::size_of::<virtq_desc>() as u64)
-                .ok_or(Error::InvalidChain)?,
+                .context("Invalid descriptor_array_addr")
+                .map_err(Error::InvalidChain)?,
         );
     }
 
     DescriptorChain::checked_new(memory, descriptor_array_addr, 0x100, 0, 0)
-        .ok_or(Error::InvalidChain)
+        .map_err(Error::InvalidChain)
 }
 
 #[cfg(test)]
diff --git a/devices/src/virtio/queue.rs b/devices/src/virtio/queue.rs
index da92eca885..aec687faa6 100644
--- a/devices/src/virtio/queue.rs
+++ b/devices/src/virtio/queue.rs
@@ -6,6 +6,7 @@ use std::cmp::min;
 use std::num::Wrapping;
 use std::sync::atomic::{fence, Ordering};
 
+use anyhow::{bail, Context};
 use base::error;
 use cros_async::{AsyncError, EventAsync};
 use virtio_sys::virtio_ring::VIRTIO_RING_F_EVENT_IDX;
@@ -85,15 +86,18 @@ impl DescriptorChain {
         queue_size: u16,
         index: u16,
         required_flags: u16,
-    ) -> Option<DescriptorChain> {
+    ) -> anyhow::Result<DescriptorChain> {
         if index >= queue_size {
-            return None;
+            bail!("index ({}) >= queue_size ({})", index, queue_size);
         }
 
-        let desc_head = mem.checked_offset(desc_table, (index as u64) * 16)?;
+        let desc_head = mem
+            .checked_offset(desc_table, (index as u64) * 16)
+            .context("desc_table checked_offset failed")?;
         // These reads can't fail unless Guest memory is hopelessly broken.
         let addr = GuestAddress(mem.read_obj_from_addr::<u64>(desc_head).unwrap() as u64);
-        mem.checked_offset(desc_head, 16)?;
+        mem.checked_offset(desc_head, 16)
+            .context("desc_head checked_offset failed")?;
         let len: u32 = mem.read_obj_from_addr(desc_head.unchecked_add(8)).unwrap();
         let flags: u16 = mem.read_obj_from_addr(desc_head.unchecked_add(12)).unwrap();
         let next: u16 = mem.read_obj_from_addr(desc_head.unchecked_add(14)).unwrap();
@@ -110,9 +114,9 @@ impl DescriptorChain {
         };
 
         if chain.is_valid() && chain.flags & required_flags == required_flags {
-            Some(chain)
+            Ok(chain)
         } else {
-            None
+            bail!("chain is invalid")
         }
     }
 
@@ -161,17 +165,22 @@ impl DescriptorChain {
         if self.has_next() {
             // Once we see a write-only descriptor, all subsequent descriptors must be write-only.
             let required_flags = self.flags & VIRTQ_DESC_F_WRITE;
-            DescriptorChain::checked_new(
+            match DescriptorChain::checked_new(
                 &self.mem,
                 self.desc_table,
                 self.queue_size,
                 self.next,
                 required_flags,
-            )
-            .map(|mut c| {
-                c.ttl = self.ttl - 1;
-                c
-            })
+            ) {
+                Ok(mut c) => {
+                    c.ttl = self.ttl - 1;
+                    Some(c)
+                }
+                Err(e) => {
+                    error!("{:#}", e);
+                    None
+                }
+            }
         } else {
             None
         }
@@ -431,6 +440,11 @@ impl Queue {
         let descriptor_index: u16 = mem.read_obj_from_addr(desc_idx_addr).unwrap();
 
         DescriptorChain::checked_new(mem, self.desc_table, queue_size, descriptor_index, 0)
+            .map_err(|e| {
+                error!("{:#}", e);
+                e
+            })
+            .ok()
     }
 
     /// Remove the first available descriptor chain from the queue.