From 053cafa41b8dbce7bac9dd5c8c45e2eb4ad6447e Mon Sep 17 00:00:00 2001 From: Matt LaFayette Date: Mon, 23 Dec 2024 14:26:02 -0800 Subject: [PATCH 1/3] First commit of a storvsp fuzzer. This begs, borrows, and steals from other test code to get something that compiles and runs. Coverage shows that the fuzzer (at this point) does hit much of the core storvsp packet processing, _except_ for the IO path. That remains a work in progress. --- Cargo.lock | 24 ++ Cargo.toml | 1 + vm/devices/storage/scsi_defs/Cargo.toml | 5 + vm/devices/storage/scsi_defs/src/lib.rs | 1 + vm/devices/storage/scsi_defs/src/srb.rs | 1 + vm/devices/storage/storvsp/Cargo.toml | 5 + vm/devices/storage/storvsp/fuzz/Cargo.toml | 51 ++++ .../storage/storvsp/fuzz/fuzz_storvsp.rs | 230 ++++++++++++++++++ vm/devices/storage/storvsp/src/ioperf.rs | 2 +- vm/devices/storage/storvsp/src/lib.rs | 37 ++- vm/devices/storage/storvsp/src/protocol.rs | 4 + .../storage/storvsp/src/test_helpers.rs | 12 +- .../storage/storvsp_resources/Cargo.toml | 5 + .../storage/storvsp_resources/src/lib.rs | 1 + 14 files changed, 359 insertions(+), 20 deletions(-) create mode 100644 vm/devices/storage/storvsp/fuzz/Cargo.toml create mode 100644 vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs diff --git a/Cargo.lock b/Cargo.lock index ce0c623f3e..4d48dbb9eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2140,6 +2140,27 @@ dependencies = [ "xtask_fuzz", ] +[[package]] +name = "fuzz_storvsp" +version = "0.0.0" +dependencies = [ + "anyhow", + "arbitrary", + "disklayer_ram", + "guestmem", + "libfuzzer-sys", + "pal_async", + "scsi_defs", + "scsidisk", + "storvsp", + "storvsp_resources", + "vmbus_async", + "vmbus_channel", + "vmbus_ring", + "xtask_fuzz", + "zerocopy", +] + [[package]] name = "fuzz_ucs2" version = "0.0.0" @@ -5670,6 +5691,7 @@ dependencies = [ name = "scsi_defs" version = "0.0.0" dependencies = [ + "arbitrary", "bitfield-struct", "open_enum", "zerocopy", @@ -6162,6 +6184,7 @@ name = "storvsp" version = "0.0.0" dependencies = [ "anyhow", + "arbitrary", "async-trait", "criterion", "disklayer_ram", @@ -6203,6 +6226,7 @@ dependencies = [ name = "storvsp_resources" version = "0.0.0" dependencies = [ + "arbitrary", "guid", "mesh", "vm_resource", diff --git a/Cargo.toml b/Cargo.toml index c566ed71a3..b5e525e7e2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,6 +41,7 @@ members = [ "vm/vmgs/vmgs_lib", "vm/vmgs/vmgstool", "hyperv/tools/hypestv", + "vm/devices/storage/storvsp/fuzz", ] exclude = [ "xsync", diff --git a/vm/devices/storage/scsi_defs/Cargo.toml b/vm/devices/storage/scsi_defs/Cargo.toml index 4bf61fa6e7..74d4a467cf 100644 --- a/vm/devices/storage/scsi_defs/Cargo.toml +++ b/vm/devices/storage/scsi_defs/Cargo.toml @@ -6,7 +6,12 @@ name = "scsi_defs" edition = "2021" rust-version.workspace = true +[features] +# Enable generating arbitrary values of types useful for fuzzing. +arbitrary = ["dep:arbitrary"] + [dependencies] +arbitrary = { workspace = true, optional = true, features = ["derive"] } zerocopy.workspace = true bitfield-struct.workspace = true open_enum.workspace = true diff --git a/vm/devices/storage/scsi_defs/src/lib.rs b/vm/devices/storage/scsi_defs/src/lib.rs index 3d02f257a7..0aa0678340 100644 --- a/vm/devices/storage/scsi_defs/src/lib.rs +++ b/vm/devices/storage/scsi_defs/src/lib.rs @@ -713,6 +713,7 @@ pub const SCSI_SENSEQ_OPERATING_DEFINITION_CHANGED: u8 = 0x02; open_enum! { #[derive(AsBytes, FromBytes, FromZeroes)] + #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] pub enum ScsiStatus: u8 { GOOD = 0x00, CHECK_CONDITION = 0x02, diff --git a/vm/devices/storage/scsi_defs/src/srb.rs b/vm/devices/storage/scsi_defs/src/srb.rs index 3f2317d869..4330100745 100644 --- a/vm/devices/storage/scsi_defs/src/srb.rs +++ b/vm/devices/storage/scsi_defs/src/srb.rs @@ -9,6 +9,7 @@ use zerocopy::FromZeroes; #[bitfield(u8)] #[derive(AsBytes, FromBytes, FromZeroes)] +#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] pub struct SrbStatusAndFlags { #[bits(6)] status_bits: u8, diff --git a/vm/devices/storage/storvsp/Cargo.toml b/vm/devices/storage/storvsp/Cargo.toml index db546422b4..95fb087fea 100644 --- a/vm/devices/storage/storvsp/Cargo.toml +++ b/vm/devices/storage/storvsp/Cargo.toml @@ -9,7 +9,12 @@ rust-version.workspace = true [features] ioperf = ["dep:disklayer_ram"] +# Enable generating arbitrary values of types useful for fuzzing. +arbitrary = ["dep:arbitrary"] + [dependencies] +arbitrary = { workspace = true, optional = true, features = ["derive"] } + disklayer_ram = { workspace = true, optional = true } # For `ioperf` modules scsi_buffers.workspace = true scsi_core.workspace = true diff --git a/vm/devices/storage/storvsp/fuzz/Cargo.toml b/vm/devices/storage/storvsp/fuzz/Cargo.toml new file mode 100644 index 0000000000..0fb8bf4fb0 --- /dev/null +++ b/vm/devices/storage/storvsp/fuzz/Cargo.toml @@ -0,0 +1,51 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +[package] +name = "fuzz_storvsp" +publish = false +edition = "2021" +rust-version.workspace = true + +[dependencies] +anyhow.workspace = true +storvsp = {workspace = true, features = ["arbitrary"]} +xtask_fuzz.workspace = true +disklayer_ram.workspace = true +scsi_defs = {workspace = true, features = ["arbitrary"]} +scsidisk.workspace = true +storvsp_resources = {workspace = true, features = ["arbitrary"]} + +vmbus_async.workspace = true +vmbus_channel.workspace = true +vmbus_ring.workspace = true + +guestmem.workspace = true +pal_async.workspace = true + +zerocopy.workspace = true + +arbitrary = { workspace = true, features = ["derive"] } + +[target.'cfg(all(target_os = "linux", target_env = "gnu"))'.dependencies] +libfuzzer-sys.workspace = true + +[package.metadata.xtask.unused-deps] +# required for the xtask_fuzz macro, but unused_deps doesn't know that +ignored = [] + +[package.metadata] +cargo-fuzz = true + +[package.metadata.xtask.fuzz.onefuzz-allowlist] +fuzz_storvsp = ["**/*.rs", "../src/**/*.rs"] + +[[bin]] +name = "fuzz_storvsp" +path = "fuzz_storvsp.rs" +test = false +doc = false +doctest = false + +[lints] +workspace = true diff --git a/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs b/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs new file mode 100644 index 0000000000..f033522e50 --- /dev/null +++ b/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs @@ -0,0 +1,230 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#![cfg_attr(all(target_os = "linux", target_env = "gnu"), no_main)] + +use arbitrary::Arbitrary; +use arbitrary::Unstructured; +use guestmem::ranges::PagedRange; +use guestmem::GuestMemory; +use libfuzzer_sys::fuzz_target; +use pal_async::DefaultPool; +use scsi_defs::Cdb10; +use scsi_defs::ScsiOp; +use std::sync::Arc; +use storvsp::protocol; +use storvsp::test_helpers::TestGuest; +use storvsp::test_helpers::TestWorker; +use storvsp::ScsiController; +use storvsp::ScsiControllerDisk; +use storvsp_resources::ScsiPath; +use vmbus_async::queue::OutgoingPacket; +use vmbus_async::queue::Queue; +use vmbus_channel::connected_async_channels; +use vmbus_ring::OutgoingPacketType; +use vmbus_ring::PAGE_SIZE; +use zerocopy::AsBytes; +use zerocopy::FromZeroes; + +#[derive(Arbitrary)] +pub enum StovspFuzzAction { + SendDataPacket, + SendReadWritePacket, + SendRawPacket, +} + +#[derive(Arbitrary)] +enum FuzzOutgoingPacketType { + AnyOutgoingPacket, + GpaDirectPacket, +} + +/// Return an arbitrary byte length that can be sent in a GPA direct +/// packet. The byte length is limited to the maximum number of pages +/// that could fit into a `PagedRange` (at least with how we store the +/// list of pages in the fuzzer ...). +fn arbitrary_byte_len(u: &mut Unstructured<'_>) -> Result { + let max_byte_len = u.arbitrary_len::()? * PAGE_SIZE; + u.int_in_range(0..=max_byte_len) +} + +/// Sends a GPA direct packet (a type of vmbus packet that references guest memory, +/// the typical packet type used for SCSI requests) to storvsp. +async fn send_gpa_direct_packet( + guest: &mut TestGuest, + payload: &[&[u8]], + gpa_start: u64, + byte_len: usize, + transaction_id: u64, +) -> Result<(), anyhow::Error> { + let start_page: u64 = gpa_start / PAGE_SIZE as u64; + let end_page = start_page + .checked_add(byte_len.try_into()?) + .and_then(|v| v.checked_add(PAGE_SIZE as u64)) + .and_then(|v| v.checked_sub(1)) + .and_then(|v| v.checked_div(PAGE_SIZE as u64)) + .ok_or(arbitrary::Error::IncorrectFormat)?; + + let gpns: Vec = (start_page..end_page).collect(); + let pages = PagedRange::new(gpa_start as usize % PAGE_SIZE, byte_len, gpns.as_slice()) + .ok_or(arbitrary::Error::IncorrectFormat)?; + + guest + .queue + .split() + .1 + .write(OutgoingPacket { + packet_type: OutgoingPacketType::GpaDirect(&[pages]), + transaction_id, + payload, + }) + .await + .map_err(|e| e.into()) +} + +/// Send a reasonably well structured read or write packet to storvsp. +/// While the fuzzer should eventually discover these paths by poking at +/// arbitrary GpaDirect packet payload, make the search more efficient by +/// generating a packet that is more likely to pass basic parsing checks. +async fn send_arbitrary_readwrite_packet( + u: &mut Unstructured<'_>, + guest: &mut TestGuest, +) -> Result<(), anyhow::Error> { + let path: ScsiPath = u.arbitrary()?; + let gpa = u.arbitrary::()?; + let byte_len = arbitrary_byte_len(u)?; + + let block: u32 = u.arbitrary()?; + let transaction_id: u64 = u.arbitrary()?; + + let packet = protocol::Packet { + operation: protocol::Operation::EXECUTE_SRB, + flags: 0, + status: protocol::NtStatus::SUCCESS, + }; + + // TODO: read6, read12, read16, write6, write12, write16, etc. (READ is read10, WRITE is write10) + let scsiop_choices = [ScsiOp::READ, ScsiOp::WRITE]; + let cdb = Cdb10 { + operation_code: *(u.choose(&scsiop_choices)?), + logical_block: block.into(), + transfer_blocks: ((byte_len / 512) as u16).try_into()?, + ..FromZeroes::new_zeroed() + }; + + let mut scsi_req = protocol::ScsiRequest { + target_id: path.target, + path_id: path.path, + lun: path.lun, + length: protocol::SCSI_REQUEST_LEN_V2 as u16, + cdb_length: size_of::() as u8, + data_transfer_length: byte_len.try_into()?, + data_in: 1, + ..FromZeroes::new_zeroed() + }; + + scsi_req.payload[0..10].copy_from_slice(cdb.as_bytes()); + + // send the gpa packet + send_gpa_direct_packet( + guest, + &[packet.as_bytes(), scsi_req.as_bytes()], + gpa, + byte_len.try_into()?, + transaction_id, + ) + .await +} + +fn do_fuzz(u: &mut Unstructured<'_>) -> Result<(), anyhow::Error> { + DefaultPool::run_with(|driver| async move { + let (host, guest_channel) = connected_async_channels(16 * 1024); // TODO: [use-arbitrary-input] + let guest_queue = Queue::new(guest_channel).unwrap(); + + let test_guest_mem = GuestMemory::allocate(u.int_in_range(1..=256)? * PAGE_SIZE); + let controller = ScsiController::new(); + let disk_len_sectors = u.int_in_range(1..=1048576)?; // up to 512mb in 512 byte sectors + let disk = scsidisk::SimpleScsiDisk::new( + disklayer_ram::ram_disk(disk_len_sectors * 512, false).unwrap(), + Default::default(), + ); + controller.attach(u.arbitrary()?, ScsiControllerDisk::new(Arc::new(disk)))?; + + let _test_worker = TestWorker::start( + controller, + driver.clone(), + test_guest_mem.clone(), + host, + None, + ); + + let mut guest = TestGuest { + queue: guest_queue, + transaction_id: 0, + }; + + if u.ratio(9, 10)? { + // TODO: [use-arbitrary-input] (e.g., munge the negotiation packets) + guest.perform_protocol_negotiation().await; + } + + while !u.is_empty() { + let action = u.arbitrary::()?; + match action { + StovspFuzzAction::SendDataPacket => { + let packet = u.arbitrary::()?; + let _ = guest.send_data_packet_sync(&[packet.as_bytes()]).await; + } + StovspFuzzAction::SendReadWritePacket => { + send_arbitrary_readwrite_packet(u, &mut guest).await?; + } + StovspFuzzAction::SendRawPacket => { + let packet_type = u.arbitrary()?; + match packet_type { + FuzzOutgoingPacketType::AnyOutgoingPacket => { + let packet_types = [ + OutgoingPacketType::InBandNoCompletion, + OutgoingPacketType::InBandWithCompletion, + OutgoingPacketType::Completion, + ]; + let packet = OutgoingPacket { + transaction_id: u.arbitrary()?, + packet_type: *u.choose(&packet_types)?, + payload: &[&[0u8; 0]], // TODO: [use-arbitrary-input] + }; + + guest.queue.split().1.write(packet).await?; + } + FuzzOutgoingPacketType::GpaDirectPacket => { + let header = u.arbitrary::()?; + let scsi_req = u.arbitrary::()?; + + send_gpa_direct_packet( + &mut guest, + &[header.as_bytes(), scsi_req.as_bytes()], + u.arbitrary()?, + arbitrary_byte_len(u)?, + u.arbitrary()?, + ) + .await? + } + } + } + } + } + + Ok::<(), anyhow::Error>(()) + })?; + + Ok::<(), anyhow::Error>(()) +} + +fuzz_target!(|input: &[u8]| { + xtask_fuzz::init_tracing_if_repro(); + + let _ = do_fuzz(&mut Unstructured::new(input)); + + // Always keep the corpus, since errors are a reasonable outcome. + // A future optimization would be to reject any corpus entries that + // result in the inability to generate arbitrary data from the Unstructured... +}); diff --git a/vm/devices/storage/storvsp/src/ioperf.rs b/vm/devices/storage/storvsp/src/ioperf.rs index 0898d03e70..c1a763c57d 100644 --- a/vm/devices/storage/storvsp/src/ioperf.rs +++ b/vm/devices/storage/storvsp/src/ioperf.rs @@ -54,7 +54,7 @@ impl PerfTester { let test_guest_mem = GuestMemory::allocate(16 * 1024); let worker = TestWorker::start( - controller.state.clone(), + controller, driver, test_guest_mem.clone(), host, diff --git a/vm/devices/storage/storvsp/src/lib.rs b/vm/devices/storage/storvsp/src/lib.rs index d3dbed8a90..6454f879dc 100644 --- a/vm/devices/storage/storvsp/src/lib.rs +++ b/vm/devices/storage/storvsp/src/lib.rs @@ -6,10 +6,10 @@ #[cfg(feature = "ioperf")] pub mod ioperf; -mod protocol; +pub mod protocol; pub mod resolver; mod save_restore; -mod test_helpers; +pub mod test_helpers; use crate::ring::gparange::GpnList; use crate::ring::gparange::MultiPagedRangeBuf; @@ -274,7 +274,7 @@ impl Future for ScsiRequest { } #[derive(Debug, Error)] -enum WorkerError { +pub enum WorkerError { #[error("packet error")] PacketError(#[source] PacketError), #[error("queue error")] @@ -284,7 +284,7 @@ enum WorkerError { } #[derive(Debug, Error)] -enum PacketError { +pub enum PacketError { #[error("Not transactional")] NotTransactional, #[error("Unrecognized operation {0:?}")] @@ -1544,7 +1544,7 @@ impl ScsiControllerDisk { } } -struct ScsiControllerState { +pub struct ScsiControllerState { disks: RwLock>, rescan_notification_source: Mutex>>, } @@ -1735,6 +1735,17 @@ mod tests { use test_with_tracing::test; use vmbus_channel::connected_async_channels; + // Discourage `Clone` for `ScsiController` outside the crate, but it is + // necessary for testing. The fuzzer also uses `TestWorker`, which needs + // a `clone` of the inner state, but is not in this crate. + impl Clone for ScsiController { + fn clone(&self) -> Self { + ScsiController { + state: self.state.clone(), + } + } + } + #[async_test] async fn test_channel_working(driver: DefaultDriver) { // set up the channels and worker @@ -1759,7 +1770,7 @@ mod tests { .unwrap(); let test_worker = TestWorker::start( - controller.state.clone(), + controller.clone(), driver.clone(), test_guest_mem.clone(), host, @@ -1814,7 +1825,7 @@ mod tests { let controller = ScsiController::new(); let _worker = TestWorker::start( - controller.state.clone(), + controller.clone(), driver.clone(), test_guest_mem, host, @@ -1885,7 +1896,7 @@ mod tests { let controller = ScsiController::new(); let _worker = TestWorker::start( - controller.state.clone(), + controller.clone(), driver.clone(), test_guest_mem, host, @@ -1935,7 +1946,7 @@ mod tests { let controller = ScsiController::new(); let worker = TestWorker::start( - controller.state.clone(), + controller.clone(), driver.clone(), test_guest_mem, host, @@ -1975,7 +1986,7 @@ mod tests { let controller = ScsiController::new(); let _worker = TestWorker::start( - controller.state.clone(), + controller.clone(), driver.clone(), test_guest_mem, host, @@ -2060,7 +2071,7 @@ mod tests { let controller = ScsiController::new(); let _worker = TestWorker::start( - controller.state.clone(), + controller.clone(), driver.clone(), test_guest_mem, host, @@ -2102,7 +2113,7 @@ mod tests { let controller = ScsiController::new(); let test_worker = TestWorker::start( - controller.state.clone(), + controller.clone(), driver.clone(), test_guest_mem.clone(), host, @@ -2283,7 +2294,7 @@ mod tests { let test_guest_mem = GuestMemory::allocate(16384); let worker = TestWorker::start( - controller.state.clone(), + controller.clone(), &driver, test_guest_mem.clone(), host, diff --git a/vm/devices/storage/storvsp/src/protocol.rs b/vm/devices/storage/storvsp/src/protocol.rs index 76a3e92370..6e0c7d414f 100644 --- a/vm/devices/storage/storvsp/src/protocol.rs +++ b/vm/devices/storage/storvsp/src/protocol.rs @@ -45,6 +45,7 @@ pub const VERSION_THRESHOLD: u16 = version(6, 2); open_enum! { #[derive(AsBytes, FromBytes, FromZeroes)] + #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] pub enum NtStatus: u32 { SUCCESS = 0x0000_0000, BUFFER_OVERFLOW = 0x8000_0005, // The data was too large to fit into the specified buffer. @@ -66,6 +67,7 @@ open_enum! { #[repr(C)] #[derive(Debug, Copy, Clone, AsBytes, FromBytes, FromZeroes)] +#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] pub struct Packet { // Requested operation type pub operation: Operation, @@ -77,6 +79,7 @@ pub struct Packet { open_enum! { #[derive(AsBytes, FromBytes, FromZeroes)] + #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] pub enum Operation: u32 { COMPLETE_IO = 1, REMOVE_DEVICE = 2, @@ -101,6 +104,7 @@ pub const VMSCSI_SENSE_BUFFER_SIZE: usize = 0x14; #[repr(C)] #[derive(Copy, Clone, Debug, AsBytes, FromBytes, FromZeroes)] +#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] pub struct ScsiRequest { pub length: u16, pub srb_status: SrbStatusAndFlags, diff --git a/vm/devices/storage/storvsp/src/test_helpers.rs b/vm/devices/storage/storvsp/src/test_helpers.rs index 3f5d5a323f..80366c5ddc 100644 --- a/vm/devices/storage/storvsp/src/test_helpers.rs +++ b/vm/devices/storage/storvsp/src/test_helpers.rs @@ -13,7 +13,7 @@ use crate::InitState; use crate::PacketError; use crate::Protocol; use crate::ProtocolState; -use crate::ScsiControllerState; +use crate::ScsiController; use crate::ScsiPath; use crate::Worker; use crate::WorkerError; @@ -38,7 +38,7 @@ use vmbus_ring::PAGE_SIZE; use zerocopy::AsBytes; use zerocopy::FromZeroes; -pub(crate) struct TestWorker { +pub struct TestWorker { task: Task>, } @@ -48,7 +48,7 @@ impl TestWorker { } pub fn start( - controller: Arc, + controller: ScsiController, spawner: impl Spawn, mem: GuestMemory, channel: RawAsyncChannel, @@ -56,7 +56,7 @@ impl TestWorker { ) -> Self { let task = spawner.spawn("test", async move { let mut worker = Worker::new( - controller.clone(), + controller.state.clone(), channel, 0, mem, @@ -164,7 +164,7 @@ pub(crate) fn parse_guest_enumerate_bus( } } -pub(crate) struct TestGuest { +pub struct TestGuest { pub queue: Queue, pub transaction_id: u64, } @@ -343,7 +343,7 @@ impl TestGuest { } // Send protocol negotiation packets for a test guest. - pub(crate) async fn perform_protocol_negotiation(&mut self) { + pub async fn perform_protocol_negotiation(&mut self) { let negotiate_packet = protocol::Packet { operation: protocol::Operation::BEGIN_INITIALIZATION, flags: 0, diff --git a/vm/devices/storage/storvsp_resources/Cargo.toml b/vm/devices/storage/storvsp_resources/Cargo.toml index ee50f6bed1..390bec70ef 100644 --- a/vm/devices/storage/storvsp_resources/Cargo.toml +++ b/vm/devices/storage/storvsp_resources/Cargo.toml @@ -6,7 +6,12 @@ name = "storvsp_resources" edition = "2021" rust-version.workspace = true +[features] +# Enable generating arbitrary values of types useful for fuzzing. +arbitrary = ["dep:arbitrary"] + [dependencies] +arbitrary = { workspace = true, optional = true, features = ["derive"] } vm_resource.workspace = true guid = { workspace = true, features = ["mesh"] } diff --git a/vm/devices/storage/storvsp_resources/src/lib.rs b/vm/devices/storage/storvsp_resources/src/lib.rs index df0b849f4c..d08b82ff3e 100644 --- a/vm/devices/storage/storvsp_resources/src/lib.rs +++ b/vm/devices/storage/storvsp_resources/src/lib.rs @@ -17,6 +17,7 @@ use vm_resource::ResourceId; /// A path at which to enumerate a SCSI logical unit. #[derive(Debug, Default, Copy, Clone, Eq, PartialEq, Hash, Protobuf)] +#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] pub struct ScsiPath { /// The SCSI path number. pub path: u8, From b6eb8813aee3506803725a7f4732ee34e0e92410 Mon Sep 17 00:00:00 2001 From: Matt LaFayette Date: Mon, 6 Jan 2025 11:55:46 -0800 Subject: [PATCH 2/3] Review feedback, clippy fixes, and add reading completions from the ring. --- Cargo.toml | 2 +- vm/devices/storage/storvsp/fuzz/Cargo.toml | 22 +++++++--------- .../storage/storvsp/fuzz/fuzz_storvsp.rs | 26 +++++++++++++------ 3 files changed, 28 insertions(+), 22 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index b5e525e7e2..9a16622c89 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,7 @@ members = [ "vm/devices/storage/disk_nvme/nvme_driver/fuzz", "vm/devices/storage/ide/fuzz", "vm/devices/storage/scsi_buffers/fuzz", + "vm/devices/storage/storvsp/fuzz", "vm/vmcore/guestmem/fuzz", "vm/x86/x86emu/fuzz", # in-guest test bins @@ -41,7 +42,6 @@ members = [ "vm/vmgs/vmgs_lib", "vm/vmgs/vmgstool", "hyperv/tools/hypestv", - "vm/devices/storage/storvsp/fuzz", ] exclude = [ "xsync", diff --git a/vm/devices/storage/storvsp/fuzz/Cargo.toml b/vm/devices/storage/storvsp/fuzz/Cargo.toml index 0fb8bf4fb0..33d2a3bba1 100644 --- a/vm/devices/storage/storvsp/fuzz/Cargo.toml +++ b/vm/devices/storage/storvsp/fuzz/Cargo.toml @@ -9,37 +9,33 @@ rust-version.workspace = true [dependencies] anyhow.workspace = true -storvsp = {workspace = true, features = ["arbitrary"]} -xtask_fuzz.workspace = true +arbitrary = { workspace = true, features = ["derive"] } disklayer_ram.workspace = true +guestmem.workspace = true +pal_async.workspace = true scsi_defs = {workspace = true, features = ["arbitrary"]} scsidisk.workspace = true +storvsp = {workspace = true, features = ["arbitrary"]} storvsp_resources = {workspace = true, features = ["arbitrary"]} - vmbus_async.workspace = true vmbus_channel.workspace = true vmbus_ring.workspace = true - -guestmem.workspace = true -pal_async.workspace = true - +xtask_fuzz.workspace = true zerocopy.workspace = true -arbitrary = { workspace = true, features = ["derive"] } - [target.'cfg(all(target_os = "linux", target_env = "gnu"))'.dependencies] libfuzzer-sys.workspace = true -[package.metadata.xtask.unused-deps] -# required for the xtask_fuzz macro, but unused_deps doesn't know that -ignored = [] - [package.metadata] cargo-fuzz = true [package.metadata.xtask.fuzz.onefuzz-allowlist] fuzz_storvsp = ["**/*.rs", "../src/**/*.rs"] +[package.metadata.xtask.unused-deps] +# required for the xtask_fuzz macro, but unused_deps doesn't know that +ignored = ["libfuzzer-sys"] + [[bin]] name = "fuzz_storvsp" path = "fuzz_storvsp.rs" diff --git a/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs b/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs index f033522e50..a16dcb42d0 100644 --- a/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs +++ b/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs @@ -7,7 +7,6 @@ use arbitrary::Arbitrary; use arbitrary::Unstructured; use guestmem::ranges::PagedRange; use guestmem::GuestMemory; -use libfuzzer_sys::fuzz_target; use pal_async::DefaultPool; use scsi_defs::Cdb10; use scsi_defs::ScsiOp; @@ -23,14 +22,16 @@ use vmbus_async::queue::Queue; use vmbus_channel::connected_async_channels; use vmbus_ring::OutgoingPacketType; use vmbus_ring::PAGE_SIZE; +use xtask_fuzz::fuzz_target; use zerocopy::AsBytes; use zerocopy::FromZeroes; #[derive(Arbitrary)] -pub enum StovspFuzzAction { +pub enum StorvspFuzzAction { SendDataPacket, SendReadWritePacket, SendRawPacket, + ReadCompletion } #[derive(Arbitrary)] @@ -108,7 +109,7 @@ async fn send_arbitrary_readwrite_packet( let cdb = Cdb10 { operation_code: *(u.choose(&scsiop_choices)?), logical_block: block.into(), - transfer_blocks: ((byte_len / 512) as u16).try_into()?, + transfer_blocks: ((byte_len / 512) as u16).into(), ..FromZeroes::new_zeroed() }; @@ -130,7 +131,7 @@ async fn send_arbitrary_readwrite_packet( guest, &[packet.as_bytes(), scsi_req.as_bytes()], gpa, - byte_len.try_into()?, + byte_len, transaction_id, ) .await @@ -169,16 +170,16 @@ fn do_fuzz(u: &mut Unstructured<'_>) -> Result<(), anyhow::Error> { } while !u.is_empty() { - let action = u.arbitrary::()?; + let action = u.arbitrary::()?; match action { - StovspFuzzAction::SendDataPacket => { + StorvspFuzzAction::SendDataPacket => { let packet = u.arbitrary::()?; let _ = guest.send_data_packet_sync(&[packet.as_bytes()]).await; } - StovspFuzzAction::SendReadWritePacket => { + StorvspFuzzAction::SendReadWritePacket => { send_arbitrary_readwrite_packet(u, &mut guest).await?; } - StovspFuzzAction::SendRawPacket => { + StorvspFuzzAction::SendRawPacket => { let packet_type = u.arbitrary()?; match packet_type { FuzzOutgoingPacketType::AnyOutgoingPacket => { @@ -210,6 +211,15 @@ fn do_fuzz(u: &mut Unstructured<'_>) -> Result<(), anyhow::Error> { } } } + StorvspFuzzAction::ReadCompletion => { + // Read completion(s) from the storvsp -> guest queue. This shouldn't + // evoke any specific storvsp behavior, but is important to eventually + // allow forward progress of various code paths. + // + // Ignore the result, since vmbus returns error if the queue is empty, + // but that's fine for the fuzzer ... + let _ = guest.queue.split().0.try_read(); + } } } From bcf4792b5359967d06378c93df11d5a262cfb99d Mon Sep 17 00:00:00 2001 From: Matt LaFayette Date: Mon, 6 Jan 2025 16:42:26 -0800 Subject: [PATCH 3/3] Review feedback: * Don't promote as many things to `pub`, and make it explicit when that happens. * .div_ceil rather than doing it manually * Arbitrary fields in enums. * cargo xtask fmt fix. --- vm/devices/storage/storvsp/Cargo.toml | 3 +++ vm/devices/storage/storvsp/fuzz/Cargo.toml | 2 +- .../storage/storvsp/fuzz/fuzz_storvsp.rs | 24 +++++++------------ vm/devices/storage/storvsp/src/lib.rs | 16 +++++++++---- .../storage/storvsp/src/test_helpers.rs | 2 +- 5 files changed, 26 insertions(+), 21 deletions(-) diff --git a/vm/devices/storage/storvsp/Cargo.toml b/vm/devices/storage/storvsp/Cargo.toml index 95fb087fea..ce446c4125 100644 --- a/vm/devices/storage/storvsp/Cargo.toml +++ b/vm/devices/storage/storvsp/Cargo.toml @@ -12,6 +12,9 @@ ioperf = ["dep:disklayer_ram"] # Enable generating arbitrary values of types useful for fuzzing. arbitrary = ["dep:arbitrary"] +# Expose some implementation details publicly, used for fuzzing. +fuzz_helpers = [] + [dependencies] arbitrary = { workspace = true, optional = true, features = ["derive"] } diff --git a/vm/devices/storage/storvsp/fuzz/Cargo.toml b/vm/devices/storage/storvsp/fuzz/Cargo.toml index 33d2a3bba1..24f1d4b033 100644 --- a/vm/devices/storage/storvsp/fuzz/Cargo.toml +++ b/vm/devices/storage/storvsp/fuzz/Cargo.toml @@ -15,7 +15,7 @@ guestmem.workspace = true pal_async.workspace = true scsi_defs = {workspace = true, features = ["arbitrary"]} scsidisk.workspace = true -storvsp = {workspace = true, features = ["arbitrary"]} +storvsp = {workspace = true, features = ["arbitrary", "fuzz_helpers"]} storvsp_resources = {workspace = true, features = ["arbitrary"]} vmbus_async.workspace = true vmbus_channel.workspace = true diff --git a/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs b/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs index a16dcb42d0..84e1b483bf 100644 --- a/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs +++ b/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs @@ -27,11 +27,10 @@ use zerocopy::AsBytes; use zerocopy::FromZeroes; #[derive(Arbitrary)] -pub enum StorvspFuzzAction { - SendDataPacket, +enum StorvspFuzzAction { SendReadWritePacket, - SendRawPacket, - ReadCompletion + SendRawPacket(FuzzOutgoingPacketType), + ReadCompletion, } #[derive(Arbitrary)] @@ -61,9 +60,7 @@ async fn send_gpa_direct_packet( let start_page: u64 = gpa_start / PAGE_SIZE as u64; let end_page = start_page .checked_add(byte_len.try_into()?) - .and_then(|v| v.checked_add(PAGE_SIZE as u64)) - .and_then(|v| v.checked_sub(1)) - .and_then(|v| v.checked_div(PAGE_SIZE as u64)) + .map(|v| v.div_ceil(PAGE_SIZE as u64)) .ok_or(arbitrary::Error::IncorrectFormat)?; let gpns: Vec = (start_page..end_page).collect(); @@ -126,7 +123,6 @@ async fn send_arbitrary_readwrite_packet( scsi_req.payload[0..10].copy_from_slice(cdb.as_bytes()); - // send the gpa packet send_gpa_direct_packet( guest, &[packet.as_bytes(), scsi_req.as_bytes()], @@ -172,15 +168,10 @@ fn do_fuzz(u: &mut Unstructured<'_>) -> Result<(), anyhow::Error> { while !u.is_empty() { let action = u.arbitrary::()?; match action { - StorvspFuzzAction::SendDataPacket => { - let packet = u.arbitrary::()?; - let _ = guest.send_data_packet_sync(&[packet.as_bytes()]).await; - } StorvspFuzzAction::SendReadWritePacket => { send_arbitrary_readwrite_packet(u, &mut guest).await?; } - StorvspFuzzAction::SendRawPacket => { - let packet_type = u.arbitrary()?; + StorvspFuzzAction::SendRawPacket(packet_type) => { match packet_type { FuzzOutgoingPacketType::AnyOutgoingPacket => { let packet_types = [ @@ -188,10 +179,13 @@ fn do_fuzz(u: &mut Unstructured<'_>) -> Result<(), anyhow::Error> { OutgoingPacketType::InBandWithCompletion, OutgoingPacketType::Completion, ]; + let payload = u.arbitrary::()?; + // TODO: [use-arbitrary-input] (send a byte blob of arbitrary length rather + // than a fixed-size arbitrary packet) let packet = OutgoingPacket { transaction_id: u.arbitrary()?, packet_type: *u.choose(&packet_types)?, - payload: &[&[0u8; 0]], // TODO: [use-arbitrary-input] + payload: &[payload.as_bytes()], // TODO: [use-arbitrary-input] }; guest.queue.split().1.write(packet).await?; diff --git a/vm/devices/storage/storvsp/src/lib.rs b/vm/devices/storage/storvsp/src/lib.rs index 6454f879dc..f6ecfaec65 100644 --- a/vm/devices/storage/storvsp/src/lib.rs +++ b/vm/devices/storage/storvsp/src/lib.rs @@ -6,10 +6,18 @@ #[cfg(feature = "ioperf")] pub mod ioperf; +#[cfg(feature = "fuzz_helpers")] pub mod protocol; +#[cfg(feature = "fuzz_helpers")] +pub mod test_helpers; + +#[cfg(not(feature = "fuzz_helpers"))] +mod protocol; +#[cfg(not(feature = "fuzz_helpers"))] +mod test_helpers; + pub mod resolver; mod save_restore; -pub mod test_helpers; use crate::ring::gparange::GpnList; use crate::ring::gparange::MultiPagedRangeBuf; @@ -274,7 +282,7 @@ impl Future for ScsiRequest { } #[derive(Debug, Error)] -pub enum WorkerError { +enum WorkerError { #[error("packet error")] PacketError(#[source] PacketError), #[error("queue error")] @@ -284,7 +292,7 @@ pub enum WorkerError { } #[derive(Debug, Error)] -pub enum PacketError { +enum PacketError { #[error("Not transactional")] NotTransactional, #[error("Unrecognized operation {0:?}")] @@ -1544,7 +1552,7 @@ impl ScsiControllerDisk { } } -pub struct ScsiControllerState { +struct ScsiControllerState { disks: RwLock>, rescan_notification_source: Mutex>>, } diff --git a/vm/devices/storage/storvsp/src/test_helpers.rs b/vm/devices/storage/storvsp/src/test_helpers.rs index 80366c5ddc..12bfb8468c 100644 --- a/vm/devices/storage/storvsp/src/test_helpers.rs +++ b/vm/devices/storage/storvsp/src/test_helpers.rs @@ -43,7 +43,7 @@ pub struct TestWorker { } impl TestWorker { - pub async fn teardown(self) -> Result<(), WorkerError> { + pub(crate) async fn teardown(self) -> Result<(), WorkerError> { self.task.await }