mirrors/crosvm
1
0
Fork 0
mirror of https://chromium.googlesource.com/crosvm/crosvm synced 2025-02-05 18:20:34 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

descriptor_utils: check for size overflow in new()

Browse source 
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>
This commit is contained in:
Daniel Verkamp 2019-10-14 15:21:50 -07:00 committed by Commit Bot
parent 67bdbc1a57
commit 7f64f5030b
7 changed files with 59 additions and 203 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
devices/src/virtio/balloon.rs
View file

@ -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);

11
devices/src/virtio/block.rs
View file

@ -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)?;

182
devices/src/virtio/descriptor_utils.rs
View file

@ -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]

45
devices/src/virtio/gpu/mod.rs
View file

@ -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;

2
devices/src/virtio/gpu/protocol.rs
View file

@ -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 {

8
devices/src/virtio/input/mod.rs
View file

@ -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)?;
}

4
devices/src/virtio/tpm.rs
View file

@ -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,