mirror of
https://github.com/cloud-hypervisor/cloud-hypervisor.git
synced 2026-08-05 02:19:16 +00:00
block: vhd: fix incomplete bounds check in sync I/O worker
The sync I/O worker only checked that the operation offset did not start past the end of the virtual disk (offset >= size) - did not verify that the operation end (offset + len) stays within bounds. A read or write that started inside the image but extended beyond the logical size was silently passed to the raw backend. The async io_uring worker already had the correct check (offset + len > size with overflow protection). I extracted it into a shared helper in worker/common.rs and reused inside the sync path to eliminate duplication and close the gap. Fixes #8311 Signed-off-by: Alexander Lvov <alexander.lvov.git@gmail.com>
This commit is contained in:
committed by
Rob Bradford
parent
7ca99204ed
commit
e0801bda3b
@@ -132,15 +132,12 @@ impl disk_file::AsyncDiskFile for VhdDisk {
|
|||||||
mod unit_tests {
|
mod unit_tests {
|
||||||
use std::fs::File;
|
use std::fs::File;
|
||||||
use std::io::{Seek, SeekFrom, Write};
|
use std::io::{Seek, SeekFrom, Write};
|
||||||
#[cfg(feature = "io_uring")]
|
|
||||||
use std::os::fd::AsRawFd;
|
use std::os::fd::AsRawFd;
|
||||||
|
|
||||||
use vmm_sys_util::tempfile::TempFile;
|
use vmm_sys_util::tempfile::TempFile;
|
||||||
|
|
||||||
use super::*;
|
use super::*;
|
||||||
use crate::async_io::AsyncIo;
|
use crate::async_io::{AsyncIo, AsyncIoError, AsyncIoOperation, OwnedIoBuffer};
|
||||||
#[cfg(feature = "io_uring")]
|
|
||||||
use crate::async_io::{AsyncIoError, AsyncIoOperation, OwnedIoBuffer};
|
|
||||||
use crate::disk_file::{AsyncDiskFile, DiskSize, PhysicalSize, Resizable};
|
use crate::disk_file::{AsyncDiskFile, DiskSize, PhysicalSize, Resizable};
|
||||||
|
|
||||||
/// Minimal fixed VHD footer (disk type = 2, current_size = 0x11223344).
|
/// Minimal fixed VHD footer (disk type = 2, current_size = 0x11223344).
|
||||||
@@ -204,9 +201,56 @@ mod unit_tests {
|
|||||||
assert_async_io(&disk, true);
|
assert_async_io(&disk, true);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn sync_rejects_read_straddling_logical_size() {
|
||||||
|
let file = TempFile::new().unwrap().into_file();
|
||||||
|
file.set_len(0x2000).unwrap();
|
||||||
|
let mut sync_io = FixedVhdSync::new(file.as_raw_fd(), 0x1000).unwrap();
|
||||||
|
let op = AsyncIoOperation::read_to_vec(0x800, OwnedIoBuffer::from_vec(vec![0; 0x900]), 1);
|
||||||
|
|
||||||
|
assert!(matches!(
|
||||||
|
sync_io.submit_data_operation(op),
|
||||||
|
Err(AsyncIoError::ReadVectored(_))
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn sync_rejects_write_straddling_logical_size() {
|
||||||
|
let file = TempFile::new().unwrap().into_file();
|
||||||
|
file.set_len(0x2000).unwrap();
|
||||||
|
let mut sync_io = FixedVhdSync::new(file.as_raw_fd(), 0x1000).unwrap();
|
||||||
|
let op =
|
||||||
|
AsyncIoOperation::write_from_vec(0x800, OwnedIoBuffer::from_vec(vec![0; 0x900]), 1);
|
||||||
|
|
||||||
|
assert!(matches!(
|
||||||
|
sync_io.submit_data_operation(op),
|
||||||
|
Err(AsyncIoError::WriteVectored(_))
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn sync_accepts_operation_exactly_filling_logical_size() {
|
||||||
|
let file = TempFile::new().unwrap().into_file();
|
||||||
|
file.set_len(0x2000).unwrap();
|
||||||
|
let mut sync_io = FixedVhdSync::new(file.as_raw_fd(), 0x1000).unwrap();
|
||||||
|
// end == size: boundary must be accepted
|
||||||
|
let op = AsyncIoOperation::read_to_vec(0, OwnedIoBuffer::from_vec(vec![0; 0x1000]), 1);
|
||||||
|
sync_io.submit_data_operation(op).unwrap();
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn sync_accepts_operation_at_last_byte() {
|
||||||
|
let file = TempFile::new().unwrap().into_file();
|
||||||
|
file.set_len(0x2000).unwrap();
|
||||||
|
let mut sync_io = FixedVhdSync::new(file.as_raw_fd(), 0x1000).unwrap();
|
||||||
|
// end = 0xFFF + 1 = 0x1000 == size: boundary must be accepted
|
||||||
|
let op = AsyncIoOperation::read_to_vec(0xFFF, OwnedIoBuffer::from_vec(vec![0; 1]), 1);
|
||||||
|
sync_io.submit_data_operation(op).unwrap();
|
||||||
|
}
|
||||||
|
|
||||||
#[cfg(feature = "io_uring")]
|
#[cfg(feature = "io_uring")]
|
||||||
#[test]
|
#[test]
|
||||||
fn io_uring_batch_rejects_request_past_logical_size() {
|
fn io_uring_batch_rejects_request_straddling_logical_size() {
|
||||||
let file = TempFile::new().unwrap().into_file();
|
let file = TempFile::new().unwrap().into_file();
|
||||||
file.set_len(0x2000).unwrap();
|
file.set_len(0x2000).unwrap();
|
||||||
let mut async_io = FixedVhdAsync::new(file.as_raw_fd(), 8, 0x1000).unwrap();
|
let mut async_io = FixedVhdAsync::new(file.as_raw_fd(), 8, 0x1000).unwrap();
|
||||||
@@ -218,6 +262,20 @@ mod unit_tests {
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(feature = "io_uring")]
|
||||||
|
#[test]
|
||||||
|
fn io_uring_rejects_single_op_straddling_logical_size() {
|
||||||
|
let file = TempFile::new().unwrap().into_file();
|
||||||
|
file.set_len(0x2000).unwrap();
|
||||||
|
let mut async_io = FixedVhdAsync::new(file.as_raw_fd(), 8, 0x1000).unwrap();
|
||||||
|
let op = AsyncIoOperation::read_to_vec(0x800, OwnedIoBuffer::from_vec(vec![0; 0x900]), 1);
|
||||||
|
|
||||||
|
assert!(matches!(
|
||||||
|
async_io.submit_data_operation(op),
|
||||||
|
Err(AsyncIoError::ReadVectored(_))
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn try_clone_preserves_sync_dispatch() {
|
fn try_clone_preserves_sync_dispatch() {
|
||||||
let file = make_vhd_file();
|
let file = make_vhd_file();
|
||||||
|
|||||||
@@ -12,6 +12,7 @@ use vmm_sys_util::eventfd::EventFd;
|
|||||||
use crate::async_io::{AsyncIo, AsyncIoCompletion, AsyncIoError, AsyncIoOperation, AsyncIoResult};
|
use crate::async_io::{AsyncIo, AsyncIoCompletion, AsyncIoError, AsyncIoOperation, AsyncIoResult};
|
||||||
use crate::error::BlockResult;
|
use crate::error::BlockResult;
|
||||||
use crate::formats::raw::worker::async_uring::RawAsync;
|
use crate::formats::raw::worker::async_uring::RawAsync;
|
||||||
|
use crate::formats::vhd::worker::common::validate_operation_bounds;
|
||||||
|
|
||||||
pub struct FixedVhdAsync {
|
pub struct FixedVhdAsync {
|
||||||
raw_file_async: RawAsync,
|
raw_file_async: RawAsync,
|
||||||
@@ -27,37 +28,6 @@ impl FixedVhdAsync {
|
|||||||
size,
|
size,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
fn validate_operation_bounds(&self, op: &AsyncIoOperation) -> AsyncIoResult<()> {
|
|
||||||
let offset = u64::try_from(op.offset()).map_err(|_| self.bounds_error(op))?;
|
|
||||||
let len = u64::try_from(op.total_len()).map_err(|_| self.bounds_error(op))?;
|
|
||||||
let end = offset
|
|
||||||
.checked_add(len)
|
|
||||||
.ok_or_else(|| self.bounds_error(op))?;
|
|
||||||
|
|
||||||
if end > self.size {
|
|
||||||
return Err(self.bounds_error(op));
|
|
||||||
}
|
|
||||||
|
|
||||||
Ok(())
|
|
||||||
}
|
|
||||||
|
|
||||||
fn bounds_error(&self, op: &AsyncIoOperation) -> AsyncIoError {
|
|
||||||
let error = io::Error::new(
|
|
||||||
io::ErrorKind::InvalidData,
|
|
||||||
format!(
|
|
||||||
"Invalid request offset {} and length {}, can't exceed file size {}",
|
|
||||||
op.offset(),
|
|
||||||
op.total_len(),
|
|
||||||
self.size
|
|
||||||
),
|
|
||||||
);
|
|
||||||
if op.is_read() {
|
|
||||||
AsyncIoError::ReadVectored(error)
|
|
||||||
} else {
|
|
||||||
AsyncIoError::WriteVectored(error)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
impl AsyncIo for FixedVhdAsync {
|
impl AsyncIo for FixedVhdAsync {
|
||||||
@@ -66,7 +36,7 @@ impl AsyncIo for FixedVhdAsync {
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn submit_data_operation(&mut self, op: AsyncIoOperation) -> AsyncIoResult<()> {
|
fn submit_data_operation(&mut self, op: AsyncIoOperation) -> AsyncIoResult<()> {
|
||||||
self.validate_operation_bounds(&op)?;
|
validate_operation_bounds(&op, self.size)?;
|
||||||
self.raw_file_async.submit_data_operation(op)
|
self.raw_file_async.submit_data_operation(op)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -96,7 +66,7 @@ impl AsyncIo for FixedVhdAsync {
|
|||||||
|
|
||||||
fn submit_batch_requests(&mut self, batch_request: Vec<AsyncIoOperation>) -> AsyncIoResult<()> {
|
fn submit_batch_requests(&mut self, batch_request: Vec<AsyncIoOperation>) -> AsyncIoResult<()> {
|
||||||
for op in &batch_request {
|
for op in &batch_request {
|
||||||
self.validate_operation_bounds(op)?;
|
validate_operation_bounds(op, self.size)?;
|
||||||
}
|
}
|
||||||
|
|
||||||
self.raw_file_async.submit_batch_requests(batch_request)
|
self.raw_file_async.submit_batch_requests(batch_request)
|
||||||
|
|||||||
38
block/src/formats/vhd/worker/common.rs
Normal file
38
block/src/formats/vhd/worker/common.rs
Normal file
@@ -0,0 +1,38 @@
|
|||||||
|
// Copyright 2026 The Cloud Hypervisor Authors. All rights reserved.
|
||||||
|
//
|
||||||
|
// SPDX-License-Identifier: Apache-2.0
|
||||||
|
|
||||||
|
use std::io;
|
||||||
|
|
||||||
|
use crate::async_io::{AsyncIoError, AsyncIoOperation, AsyncIoResult};
|
||||||
|
|
||||||
|
pub(super) fn validate_operation_bounds(op: &AsyncIoOperation, size: u64) -> AsyncIoResult<()> {
|
||||||
|
let offset = u64::try_from(op.offset()).map_err(|_| bounds_error(op, size))?;
|
||||||
|
let len = u64::try_from(op.total_len()).map_err(|_| bounds_error(op, size))?;
|
||||||
|
let end = offset
|
||||||
|
.checked_add(len)
|
||||||
|
.ok_or_else(|| bounds_error(op, size))?;
|
||||||
|
|
||||||
|
if end > size {
|
||||||
|
return Err(bounds_error(op, size));
|
||||||
|
}
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
|
fn bounds_error(op: &AsyncIoOperation, size: u64) -> AsyncIoError {
|
||||||
|
let error = io::Error::new(
|
||||||
|
io::ErrorKind::InvalidData,
|
||||||
|
format!(
|
||||||
|
"Invalid request offset {} and length {}, can't exceed file size {}",
|
||||||
|
op.offset(),
|
||||||
|
op.total_len(),
|
||||||
|
size
|
||||||
|
),
|
||||||
|
);
|
||||||
|
if op.is_read() {
|
||||||
|
AsyncIoError::ReadVectored(error)
|
||||||
|
} else {
|
||||||
|
AsyncIoError::WriteVectored(error)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -9,4 +9,5 @@
|
|||||||
|
|
||||||
#[cfg(feature = "io_uring")]
|
#[cfg(feature = "io_uring")]
|
||||||
pub(crate) mod async_uring;
|
pub(crate) mod async_uring;
|
||||||
|
mod common;
|
||||||
pub(crate) mod sync;
|
pub(crate) mod sync;
|
||||||
|
|||||||
@@ -11,6 +11,7 @@ use vmm_sys_util::eventfd::EventFd;
|
|||||||
|
|
||||||
use crate::async_io::{AsyncIo, AsyncIoCompletion, AsyncIoError, AsyncIoOperation, AsyncIoResult};
|
use crate::async_io::{AsyncIo, AsyncIoCompletion, AsyncIoError, AsyncIoOperation, AsyncIoResult};
|
||||||
use crate::formats::raw::worker::sync::RawSync;
|
use crate::formats::raw::worker::sync::RawSync;
|
||||||
|
use crate::formats::vhd::worker::common::validate_operation_bounds;
|
||||||
|
|
||||||
pub struct FixedVhdSync {
|
pub struct FixedVhdSync {
|
||||||
raw_file_sync: RawSync,
|
raw_file_sync: RawSync,
|
||||||
@@ -32,22 +33,7 @@ impl AsyncIo for FixedVhdSync {
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn submit_data_operation(&mut self, op: AsyncIoOperation) -> AsyncIoResult<()> {
|
fn submit_data_operation(&mut self, op: AsyncIoOperation) -> AsyncIoResult<()> {
|
||||||
let offset = op.offset();
|
validate_operation_bounds(&op, self.size)?;
|
||||||
if offset as u64 >= self.size {
|
|
||||||
let error = io::Error::new(
|
|
||||||
io::ErrorKind::InvalidData,
|
|
||||||
format!(
|
|
||||||
"Invalid offset {}, can't be larger than file size {}",
|
|
||||||
offset, self.size
|
|
||||||
),
|
|
||||||
);
|
|
||||||
return Err(if op.is_read() {
|
|
||||||
AsyncIoError::ReadVectored(error)
|
|
||||||
} else {
|
|
||||||
AsyncIoError::WriteVectored(error)
|
|
||||||
});
|
|
||||||
}
|
|
||||||
|
|
||||||
self.raw_file_sync.submit_data_operation(op)
|
self.raw_file_sync.submit_data_operation(op)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user