From 7f64f5030b40acded00631465cc3f8b122317b04 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Mon, 14 Oct 2019 15:21:50 -0700 Subject: 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 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/1873142 Reviewed-by: Dylan Reid Reviewed-by: Zach Reizner Tested-by: kokoro Commit-Queue: Stephen Barber --- devices/src/virtio/balloon.rs | 10 +- devices/src/virtio/block.rs | 11 +- devices/src/virtio/descriptor_utils.rs | 182 ++++++++------------------------- devices/src/virtio/gpu/mod.rs | 45 +------- devices/src/virtio/gpu/protocol.rs | 2 +- devices/src/virtio/input/mod.rs | 8 +- devices/src/virtio/tpm.rs | 4 +- 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 { 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::() - { + while reader.available_bytes() >= size_of::() { 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 { + 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> { // 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 { + 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> { + 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 { + 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::().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 Worker { ) -> Result { 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 Worker { mem: &GuestMemory, ) -> Result { 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, -- cgit 1.4.1