summary refs log tree commit diff
diff options
context:
space:
mode:
authorDaniel Verkamp <dverkamp@chromium.org>2019-10-14 15:21:50 -0700
committerCommit Bot <commit-bot@chromium.org>2019-10-25 17:49:40 +0000
commit7f64f5030b40acded00631465cc3f8b122317b04 (patch)
treee618b69017caf335d30411f810b908de7270912c
parent67bdbc1a57a6e62a5d162d8eb43508b20fd0acda (diff)
downloadcrosvm-7f64f5030b40acded00631465cc3f8b122317b04.tar
crosvm-7f64f5030b40acded00631465cc3f8b122317b04.tar.gz
crosvm-7f64f5030b40acded00631465cc3f8b122317b04.tar.bz2
crosvm-7f64f5030b40acded00631465cc3f8b122317b04.tar.lz
crosvm-7f64f5030b40acded00631465cc3f8b122317b04.tar.xz
crosvm-7f64f5030b40acded00631465cc3f8b122317b04.tar.zst
crosvm-7f64f5030b40acded00631465cc3f8b122317b04.zip
descriptor_utils: check for size overflow in new()
Move the check for length overflow that was in available_bytes() into
Reader::new() and Writer::new().  This simplifies callers, since they
can assume that once a valid Reader or Writer has been constructed,
available_bytes() cannot fail.  Since we are walking the descriptor
chain during new() anyway, this extra check should be essentially free.

BUG=None
TEST=cargo test -p devices descriptor_utils

Change-Id: Ibeb1defd3728e7b71356650094b0885f3419ed47
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/1873142
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Stephen Barber <smbarber@chromium.org>
-rw-r--r--devices/src/virtio/balloon.rs10
-rw-r--r--devices/src/virtio/block.rs11
-rw-r--r--devices/src/virtio/descriptor_utils.rs182
-rw-r--r--devices/src/virtio/gpu/mod.rs45
-rw-r--r--devices/src/virtio/gpu/protocol.rs2
-rw-r--r--devices/src/virtio/input/mod.rs8
-rw-r--r--devices/src/virtio/tpm.rs4
7 files changed, 59 insertions, 203 deletions
diff --git a/devices/src/virtio/balloon.rs b/devices/src/virtio/balloon.rs
index 4f2e692..460aba5 100644
--- a/devices/src/virtio/balloon.rs
+++ b/devices/src/virtio/balloon.rs
@@ -105,15 +105,7 @@ impl Worker {
                         continue;
                     }
                 };
-                let data_length = match reader.available_bytes() {
-                    Ok(l) => l,
-                    Err(e) => {
-                        error!("balloon: failed to get available bytes: {}", e);
-                        queue.add_used(&self.mem, index, 0);
-                        needs_interrupt = true;
-                        continue;
-                    }
-                };
+                let data_length = reader.available_bytes();
 
                 if data_length % 4 != 0 {
                     error!("invalid inflate buffer size: {}", data_length);
diff --git a/devices/src/virtio/block.rs b/devices/src/virtio/block.rs
index 554898c..58e00d7 100644
--- a/devices/src/virtio/block.rs
+++ b/devices/src/virtio/block.rs
@@ -293,9 +293,7 @@ impl Worker {
     ) -> result::Result<usize, ExecuteError> {
         let mut status_writer =
             Writer::new(mem, avail_desc.clone()).map_err(ExecuteError::Descriptor)?;
-        let available_bytes = status_writer
-            .available_bytes()
-            .map_err(ExecuteError::Descriptor)?;
+        let available_bytes = status_writer.available_bytes();
         let status_offset = available_bytes
             .checked_sub(1)
             .ok_or(ExecuteError::MissingStatus)?;
@@ -616,7 +614,6 @@ impl Block {
                 // The last byte of writer is virtio_blk_req::status, so subtract it from data_len.
                 let data_len = writer
                     .available_bytes()
-                    .map_err(ExecuteError::Descriptor)?
                     .checked_sub(1)
                     .ok_or(ExecuteError::MissingStatus)?;
                 let offset = sector
@@ -641,7 +638,7 @@ impl Block {
                 }
             }
             VIRTIO_BLK_T_OUT => {
-                let data_len = reader.available_bytes().map_err(ExecuteError::Descriptor)?;
+                let data_len = reader.available_bytes();
                 let offset = sector
                     .checked_shl(u32::from(SECTOR_SHIFT))
                     .ok_or(ExecuteError::OutOfRange)?;
@@ -671,9 +668,7 @@ impl Block {
                 }
             }
             VIRTIO_BLK_T_DISCARD | VIRTIO_BLK_T_WRITE_ZEROES => {
-                while reader.available_bytes().map_err(ExecuteError::Descriptor)?
-                    >= size_of::<virtio_blk_discard_write_zeroes>()
-                {
+                while reader.available_bytes() >= size_of::<virtio_blk_discard_write_zeroes>() {
                     let seg: virtio_blk_discard_write_zeroes =
                         reader.read_obj().map_err(ExecuteError::Read)?;
 
diff --git a/devices/src/virtio/descriptor_utils.rs b/devices/src/virtio/descriptor_utils.rs
index b4eff2d..80cc530 100644
--- a/devices/src/virtio/descriptor_utils.rs
+++ b/devices/src/virtio/descriptor_utils.rs
@@ -53,11 +53,13 @@ struct DescriptorChainConsumer<'a> {
 }
 
 impl<'a> DescriptorChainConsumer<'a> {
-    fn available_bytes(&self) -> Result<usize> {
+    fn available_bytes(&self) -> usize {
+        // This is guaranteed not to overflow because the total length of the chain
+        // is checked during all creations of `DescriptorChainConsumer` (see
+        // `Reader::new()` and `Writer::new()`).
         self.buffers
             .iter()
-            .try_fold(0usize, |count, vs| count.checked_add(vs.size() as usize))
-            .ok_or(Error::DescriptorChainOverflow)
+            .fold(0usize, |count, vs| count + vs.size() as usize)
     }
 
     fn bytes_consumed(&self) -> usize {
@@ -192,10 +194,18 @@ impl<'a> Reader<'a> {
     /// Construct a new Reader wrapper over `desc_chain`.
     pub fn new(mem: &'a GuestMemory, desc_chain: DescriptorChain<'a>) -> Result<Reader<'a>> {
         // TODO(jstaron): Update this code to take the indirect descriptors into account.
+        let mut total_len: usize = 0;
         let buffers = desc_chain
             .into_iter()
             .readable()
             .map(|desc| {
+                // Verify that summing the descriptor sizes does not overflow.
+                // This can happen if a driver tricks a device into reading more data than
+                // fits in a `usize`.
+                total_len = total_len
+                    .checked_add(desc.len as usize)
+                    .ok_or(Error::DescriptorChainOverflow)?;
+
                 mem.get_slice(desc.addr.offset(), desc.len.into())
                     .map_err(Error::VolatileMemoryError)
             })
@@ -276,7 +286,7 @@ impl<'a> Reader<'a> {
 
     /// Returns number of bytes available for reading.  May return an error if the combined
     /// lengths of all the buffers in the DescriptorChain would cause an integer overflow.
-    pub fn available_bytes(&self) -> Result<usize> {
+    pub fn available_bytes(&self) -> usize {
         self.buffer.available_bytes()
     }
 
@@ -328,10 +338,18 @@ pub struct Writer<'a> {
 impl<'a> Writer<'a> {
     /// Construct a new Writer wrapper over `desc_chain`.
     pub fn new(mem: &'a GuestMemory, desc_chain: DescriptorChain<'a>) -> Result<Writer<'a>> {
+        let mut total_len: usize = 0;
         let buffers = desc_chain
             .into_iter()
             .writable()
             .map(|desc| {
+                // Verify that summing the descriptor sizes does not overflow.
+                // This can happen if a driver tricks a device into writing more data than
+                // fits in a `usize`.
+                total_len = total_len
+                    .checked_add(desc.len as usize)
+                    .ok_or(Error::DescriptorChainOverflow)?;
+
                 mem.get_slice(desc.addr.offset(), desc.len.into())
                     .map_err(Error::VolatileMemoryError)
             })
@@ -351,7 +369,7 @@ impl<'a> Writer<'a> {
 
     /// Returns number of bytes available for writing.  May return an error if the combined
     /// lengths of all the buffers in the DescriptorChain would cause an overflow.
-    pub fn available_bytes(&self) -> Result<usize> {
+    pub fn available_bytes(&self) -> usize {
         self.buffer.available_bytes()
     }
 
@@ -532,12 +550,7 @@ mod tests {
         )
         .expect("create_descriptor_chain failed");
         let mut reader = Reader::new(&memory, chain).expect("failed to create Reader");
-        assert_eq!(
-            reader
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            106
-        );
+        assert_eq!(reader.available_bytes(), 106);
         assert_eq!(reader.bytes_read(), 0);
 
         let mut buffer = [0 as u8; 64];
@@ -545,12 +558,7 @@ mod tests {
             panic!("read_exact should not fail here");
         }
 
-        assert_eq!(
-            reader
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            42
-        );
+        assert_eq!(reader.available_bytes(), 42);
         assert_eq!(reader.bytes_read(), 64);
 
         match reader.read(&mut buffer) {
@@ -558,12 +566,7 @@ mod tests {
             Ok(length) => assert_eq!(length, 42),
         }
 
-        assert_eq!(
-            reader
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            0
-        );
+        assert_eq!(reader.available_bytes(), 0);
         assert_eq!(reader.bytes_read(), 106);
     }
 
@@ -588,12 +591,7 @@ mod tests {
         )
         .expect("create_descriptor_chain failed");;
         let mut writer = Writer::new(&memory, chain).expect("failed to create Writer");
-        assert_eq!(
-            writer
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            106
-        );
+        assert_eq!(writer.available_bytes(), 106);
         assert_eq!(writer.bytes_written(), 0);
 
         let mut buffer = [0 as u8; 64];
@@ -601,12 +599,7 @@ mod tests {
             panic!("write_all should not fail here");
         }
 
-        assert_eq!(
-            writer
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            42
-        );
+        assert_eq!(writer.available_bytes(), 42);
         assert_eq!(writer.bytes_written(), 64);
 
         match writer.write(&mut buffer) {
@@ -614,12 +607,7 @@ mod tests {
             Ok(length) => assert_eq!(length, 42),
         }
 
-        assert_eq!(
-            writer
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            0
-        );
+        assert_eq!(writer.available_bytes(), 0);
         assert_eq!(writer.bytes_written(), 106);
     }
 
@@ -639,22 +627,12 @@ mod tests {
         )
         .expect("create_descriptor_chain failed");;
         let mut reader = Reader::new(&memory, chain).expect("failed to create Reader");
-        assert_eq!(
-            reader
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            0
-        );
+        assert_eq!(reader.available_bytes(), 0);
         assert_eq!(reader.bytes_read(), 0);
 
         assert!(reader.read_obj::<u8>().is_err());
 
-        assert_eq!(
-            reader
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            0
-        );
+        assert_eq!(reader.available_bytes(), 0);
         assert_eq!(reader.bytes_read(), 0);
     }
 
@@ -674,22 +652,12 @@ mod tests {
         )
         .expect("create_descriptor_chain failed");;
         let mut writer = Writer::new(&memory, chain).expect("failed to create Writer");
-        assert_eq!(
-            writer
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            0
-        );
+        assert_eq!(writer.available_bytes(), 0);
         assert_eq!(writer.bytes_written(), 0);
 
         assert!(writer.write_obj(0u8).is_err());
 
-        assert_eq!(
-            writer
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            0
-        );
+        assert_eq!(writer.available_bytes(), 0);
         assert_eq!(writer.bytes_written(), 0);
     }
 
@@ -727,12 +695,7 @@ mod tests {
         // Linux doesn't do partial writes if you give a buffer larger than the remaining length of
         // the shared memory. And since we passed an iovec with the full contents of the
         // DescriptorChain we ended up not writing any bytes at all.
-        assert_eq!(
-            reader
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            512
-        );
+        assert_eq!(reader.available_bytes(), 512);
         assert_eq!(reader.bytes_read(), 0);
     }
 
@@ -762,12 +725,7 @@ mod tests {
             .write_all_from(&mut shm, 512)
             .expect_err("successfully wrote more bytes than in SharedMemory");
 
-        assert_eq!(
-            writer
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            128
-        );
+        assert_eq!(writer.available_bytes(), 128);
         assert_eq!(writer.bytes_written(), 384);
     }
 
@@ -813,19 +771,9 @@ mod tests {
             .write_all(&buffer[..68])
             .expect("write should not fail here");
 
-        assert_eq!(
-            reader
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            0
-        );
+        assert_eq!(reader.available_bytes(), 0);
         assert_eq!(reader.bytes_read(), 128);
-        assert_eq!(
-            writer
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            0
-        );
+        assert_eq!(writer.available_bytes(), 0);
         assert_eq!(writer.bytes_written(), 68);
     }
 
@@ -923,18 +871,8 @@ mod tests {
         let mut reader = Reader::new(&memory, chain).expect("failed to create Reader");
 
         let other = reader.split_at(32).expect("failed to split Reader");
-        assert_eq!(
-            reader
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            32
-        );
-        assert_eq!(
-            other
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            96
-        );
+        assert_eq!(reader.available_bytes(), 32);
+        assert_eq!(other.available_bytes(), 96);
     }
 
     #[test]
@@ -962,18 +900,8 @@ mod tests {
         let mut reader = Reader::new(&memory, chain).expect("failed to create Reader");
 
         let other = reader.split_at(24).expect("failed to split Reader");
-        assert_eq!(
-            reader
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            24
-        );
-        assert_eq!(
-            other
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            104
-        );
+        assert_eq!(reader.available_bytes(), 24);
+        assert_eq!(other.available_bytes(), 104);
     }
 
     #[test]
@@ -1001,18 +929,8 @@ mod tests {
         let mut reader = Reader::new(&memory, chain).expect("failed to create Reader");
 
         let other = reader.split_at(128).expect("failed to split Reader");
-        assert_eq!(
-            reader
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            128
-        );
-        assert_eq!(
-            other
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            0
-        );
+        assert_eq!(reader.available_bytes(), 128);
+        assert_eq!(other.available_bytes(), 0);
     }
 
     #[test]
@@ -1040,18 +958,8 @@ mod tests {
         let mut reader = Reader::new(&memory, chain).expect("failed to create Reader");
 
         let other = reader.split_at(0).expect("failed to split Reader");
-        assert_eq!(
-            reader
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            0
-        );
-        assert_eq!(
-            other
-                .available_bytes()
-                .expect("failed to get available bytes"),
-            128
-        );
+        assert_eq!(reader.available_bytes(), 0);
+        assert_eq!(other.available_bytes(), 128);
     }
 
     #[test]
diff --git a/devices/src/virtio/gpu/mod.rs b/devices/src/virtio/gpu/mod.rs
index 8be0e3b..1e545eb 100644
--- a/devices/src/virtio/gpu/mod.rs
+++ b/devices/src/virtio/gpu/mod.rs
@@ -126,13 +126,7 @@ impl Frontend {
                 mem,
             ),
             GpuCommand::ResourceAttachBacking(info) => {
-                let available_bytes = match reader.available_bytes() {
-                    Ok(count) => count,
-                    Err(e) => {
-                        debug!("invalid descriptor: {}", e);
-                        0
-                    }
-                };
+                let available_bytes = reader.available_bytes();
                 if available_bytes != 0 {
                     let entry_count = info.nr_entries.to_native() as usize;
                     let mut iovecs = Vec::with_capacity(entry_count);
@@ -256,14 +250,7 @@ impl Frontend {
                 )
             }
             GpuCommand::CmdSubmit3d(info) => {
-                let available_bytes = match reader.available_bytes() {
-                    Ok(count) => count,
-                    Err(e) => {
-                        debug!("invalid descriptor: {}", e);
-                        0
-                    }
-                };
-                if available_bytes != 0 {
+                if reader.available_bytes() != 0 {
                     let cmd_size = info.size.to_native() as usize;
                     let mut cmd_buf = vec![0; cmd_size];
                     if reader.read_exact(&mut cmd_buf[..]).is_ok() {
@@ -279,14 +266,7 @@ impl Frontend {
                 }
             }
             GpuCommand::AllocationMetadata(info) => {
-                let available_bytes = match reader.available_bytes() {
-                    Ok(count) => count,
-                    Err(e) => {
-                        debug!("invalid descriptor: {}", e);
-                        0
-                    }
-                };
-                if available_bytes != 0 {
+                if reader.available_bytes() != 0 {
                     let id = info.request_id.to_native();
                     let request_size = info.request_size.to_native();
                     let response_size = info.response_size.to_native();
@@ -309,14 +289,7 @@ impl Frontend {
                 }
             }
             GpuCommand::ResourceCreateV2(info) => {
-                let available_bytes = match reader.available_bytes() {
-                    Ok(count) => count,
-                    Err(e) => {
-                        debug!("invalid descriptor: {}", e);
-                        0
-                    }
-                };
-                if available_bytes != 0 {
+                if reader.available_bytes() != 0 {
                     let resource_id = info.resource_id.to_native();
                     let guest_memory_type = info.guest_memory_type.to_native();
                     let size = info.size.to_native();
@@ -432,15 +405,7 @@ impl Frontend {
             debug!("{:?} -> {:?}", gpu_cmd, resp);
         }
 
-        let available_bytes = match writer.available_bytes() {
-            Ok(count) => count,
-            Err(e) => {
-                debug!("invalid descriptor: {}", e);
-                0
-            }
-        };
-
-        if available_bytes != 0 {
+        if writer.available_bytes() != 0 {
             let mut fence_id = 0;
             let mut ctx_id = 0;
             let mut flags = 0;
diff --git a/devices/src/virtio/gpu/protocol.rs b/devices/src/virtio/gpu/protocol.rs
index dacc850..b4610b2 100644
--- a/devices/src/virtio/gpu/protocol.rs
+++ b/devices/src/virtio/gpu/protocol.rs
@@ -866,7 +866,7 @@ impl GpuResponse {
                     strides,
                     offsets,
                 };
-                if resp.available_bytes()? >= size_of_val(&plane_info) {
+                if resp.available_bytes() >= size_of_val(&plane_info) {
                     resp.write_obj(plane_info)?;
                     size_of_val(&plane_info)
                 } else {
diff --git a/devices/src/virtio/input/mod.rs b/devices/src/virtio/input/mod.rs
index 2694d46..706a563 100644
--- a/devices/src/virtio/input/mod.rs
+++ b/devices/src/virtio/input/mod.rs
@@ -387,9 +387,7 @@ impl<T: EventSource> Worker<T> {
     ) -> Result<usize> {
         let mut writer = Writer::new(mem, avail_desc).map_err(InputError::Descriptor)?;
 
-        while writer.available_bytes().map_err(InputError::Descriptor)?
-            >= virtio_input_event::EVENT_SIZE
-        {
+        while writer.available_bytes() >= virtio_input_event::EVENT_SIZE {
             if let Some(evt) = event_source.pop_available_event() {
                 writer.write_obj(evt).map_err(InputError::WriteQueue)?;
             } else {
@@ -445,9 +443,7 @@ impl<T: EventSource> Worker<T> {
         mem: &GuestMemory,
     ) -> Result<usize> {
         let mut reader = Reader::new(mem, avail_desc).map_err(InputError::Descriptor)?;
-        while reader.available_bytes().map_err(InputError::Descriptor)?
-            >= virtio_input_event::EVENT_SIZE
-        {
+        while reader.available_bytes() >= virtio_input_event::EVENT_SIZE {
             let evt: virtio_input_event = reader.read_obj().map_err(InputError::ReadQueue)?;
             event_source.send_event(&evt)?;
         }
diff --git a/devices/src/virtio/tpm.rs b/devices/src/virtio/tpm.rs
index 2267fed..f5637d3 100644
--- a/devices/src/virtio/tpm.rs
+++ b/devices/src/virtio/tpm.rs
@@ -54,7 +54,7 @@ impl Device {
         let mut reader = Reader::new(mem, desc.clone()).map_err(Error::Descriptor)?;
         let mut writer = Writer::new(mem, desc).map_err(Error::Descriptor)?;
 
-        let available_bytes = reader.available_bytes().map_err(Error::Descriptor)?;
+        let available_bytes = reader.available_bytes();
         if available_bytes > TPM_BUFSIZE {
             return Err(Error::CommandTooLong {
                 size: available_bytes,
@@ -72,7 +72,7 @@ impl Device {
             });
         }
 
-        let writer_len = writer.available_bytes().map_err(Error::Descriptor)?;
+        let writer_len = writer.available_bytes();
         if response.len() > writer_len {
             return Err(Error::BufferTooSmall {
                 size: writer_len,