Skip to content

Commit 5513c71

Browse files
roypatroypat
committed
refactor: Hoist panic out of virtio queue code
virtio/queue.rs has a panic in pop()/try_enable_notification(), to avoid DoS scenarios of the guest asking firecracker to process the same virtio descriptor multiple times. However, this panic is not only triggered at VM runtime, but also by various snapshot calls (parsing rx buffers on net restore, vsock notifying used buffers), where ideally we shouldn't panic on malformed snapshots, but instead report an error back to the user. It also make fuzz-testing of firecracker more difficult, because this panic represents a false-positive. To avoid all of this, turn the panic into an error variant, and bubble it out of the virtio stack. This way, the event loop and unwrap()/panic!() when it encounters this error, while other usecases and report the error properly (snapshot code) or ignore it (fuzzing). Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
1 parent fbd29ae commit 5513c71

File tree

21 files changed

+225
-141
lines changed

21 files changed

+225
-141
lines changed

‎src/vmm/benches/block_request.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ pub fn block_request_benchmark(c: &mut Criterion) {
2424
chain.set_header(request_header);
2525

2626
let mut queue = virt_queue.create_queue();
27-
let desc = queue.pop().unwrap();
27+
let desc = queue.pop().unwrap().unwrap();
2828

2929
c.bench_function("request_parse", |b| {
3030
b.iter(|| {

‎src/vmm/benches/queue.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ pub fn queue_benchmark(c: &mut Criterion) {
6161

6262
set_dtable_one_chain(&rxq, 16);
6363
queue.next_avail = Wrapping(0);
64-
let desc = queue.pop().unwrap();
64+
let desc = queue.pop().unwrap().unwrap();
6565
c.bench_function("next_descriptor_16", |b| {
6666
b.iter(|| {
6767
let mut head = Some(desc);
@@ -75,7 +75,7 @@ pub fn queue_benchmark(c: &mut Criterion) {
7575
c.bench_function("queue_pop_16", |b| {
7676
b.iter(|| {
7777
queue.next_avail = Wrapping(0);
78-
while let Some(desc) = queue.pop() {
78+
while let Some(desc) = queue.pop().unwrap() {
7979
std::hint::black_box(desc);
8080
}
8181
})

‎src/vmm/src/device_manager/mmio.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -461,7 +461,7 @@ impl MMIODeviceManager {
461461
// Stats queue doesn't need kicking as it is notified via a `timer_fd`.
462462
if balloon.is_activated() {
463463
info!("kick balloon {}.", id);
464-
balloon.process_virtio_queues();
464+
balloon.process_virtio_queues().unwrap();
465465
}
466466
}
467467
TYPE_BLOCK => {
@@ -475,7 +475,7 @@ impl MMIODeviceManager {
475475
// any inflight `timer_fd` events can be safely discarded.
476476
if block.is_activated() {
477477
info!("kick block {}.", id);
478-
block.process_virtio_queues();
478+
block.process_virtio_queues().unwrap()
479479
}
480480
}
481481
}
@@ -487,7 +487,7 @@ impl MMIODeviceManager {
487487
// any inflight `timer_fd` events can be safely discarded.
488488
if net.is_activated() {
489489
info!("kick net {}.", id);
490-
net.process_virtio_queues();
490+
net.process_virtio_queues().unwrap();
491491
}
492492
}
493493
TYPE_VSOCK => {
@@ -510,7 +510,7 @@ impl MMIODeviceManager {
510510
let entropy = virtio.as_mut_any().downcast_mut::<Entropy>().unwrap();
511511
if entropy.is_activated() {
512512
info!("kick entropy {id}.");
513-
entropy.process_virtio_queues();
513+
entropy.process_virtio_queues().unwrap();
514514
}
515515
}
516516
_ => (),

‎src/vmm/src/devices/mod.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ pub use bus::{Bus, BusDevice, BusError};
1919
use log::error;
2020

2121
use crate::devices::virtio::net::metrics::NetDeviceMetrics;
22-
use crate::devices::virtio::queue::QueueError;
22+
use crate::devices::virtio::queue::{InvalidAvailIdx, QueueError};
2323
use crate::devices::virtio::vsock::VsockError;
2424
use crate::logger::IncMetric;
2525

@@ -28,6 +28,9 @@ use crate::logger::IncMetric;
2828
// network metrics is reported per device so we need a handle to each net device's
2929
// metrics `net_iface_metrics` to report metrics for that device.
3030
pub(crate) fn report_net_event_fail(net_iface_metrics: &NetDeviceMetrics, err: DeviceError) {
31+
if let DeviceError::InvalidAvailIdx(err) = err {
32+
panic!("{}", err);
33+
}
3134
error!("{:?}", err);
3235
net_iface_metrics.event_fails.inc();
3336
}
@@ -46,6 +49,8 @@ pub enum DeviceError {
4649
MalformedDescriptor,
4750
/// Error during queue processing: {0}
4851
QueueError(#[from] QueueError),
52+
/// {0}
53+
InvalidAvailIdx(#[from] InvalidAvailIdx),
4954
/// Vsock device error: {0}
5055
VsockError(#[from] VsockError),
5156
}

‎src/vmm/src/devices/virtio/balloon/device.rs

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ use super::{
2626
use crate::devices::virtio::balloon::BalloonError;
2727
use crate::devices::virtio::device::{IrqTrigger, IrqType};
2828
use crate::devices::virtio::generated::virtio_config::VIRTIO_F_VERSION_1;
29+
use crate::devices::virtio::queue::InvalidAvailIdx;
2930
use crate::logger::IncMetric;
3031
use crate::utils::u64_to_usize;
3132
use crate::vstate::memory::{Address, ByteValued, Bytes, GuestAddress, GuestMemoryMmap};
@@ -297,7 +298,7 @@ impl Balloon {
297298
// Internal loop processes descriptors and acummulates the pfns in `pfn_buffer`.
298299
// Breaks out when there is not enough space in `pfn_buffer` to completely process
299300
// the next descriptor.
300-
while let Some(head) = queue.pop() {
301+
while let Some(head) = queue.pop()? {
301302
let len = head.len as usize;
302303
let max_len = MAX_PAGES_IN_DESC * SIZE_OF_U32;
303304
valid_descs_found = true;
@@ -375,7 +376,7 @@ impl Balloon {
375376
let queue = &mut self.queues[DEFLATE_INDEX];
376377
let mut needs_interrupt = false;
377378

378-
while let Some(head) = queue.pop() {
379+
while let Some(head) = queue.pop()? {
379380
queue.add_used(head.index, 0)?;
380381
needs_interrupt = true;
381382
}
@@ -392,7 +393,7 @@ impl Balloon {
392393
let mem = self.device_state.mem().unwrap();
393394
METRICS.stats_updates_count.inc();
394395

395-
while let Some(head) = self.queues[STATS_INDEX].pop() {
396+
while let Some(head) = self.queues[STATS_INDEX].pop()? {
396397
if let Some(prev_stats_desc) = self.stats_desc_index {
397398
// We shouldn't ever have an extra buffer if the driver follows
398399
// the protocol, but return it if we find one.
@@ -431,9 +432,15 @@ impl Balloon {
431432
}
432433

433434
/// Process device virtio queue(s).
434-
pub fn process_virtio_queues(&mut self) {
435-
let _ = self.process_inflate();
436-
let _ = self.process_deflate_queue();
435+
pub fn process_virtio_queues(&mut self) -> Result<(), InvalidAvailIdx> {
436+
if let Err(BalloonError::InvalidAvailIdx(err)) = self.process_inflate() {
437+
return Err(err);
438+
}
439+
if let Err(BalloonError::InvalidAvailIdx(err)) = self.process_deflate_queue() {
440+
return Err(err);
441+
}
442+
443+
Ok(())
437444
}
438445

439446
/// Provides the ID of this balloon device.
@@ -1080,7 +1087,7 @@ pub(crate) mod tests {
10801087
balloon.set_queue(DEFLATE_INDEX, defq.create_queue());
10811088

10821089
balloon.activate(mem).unwrap();
1083-
balloon.process_virtio_queues()
1090+
balloon.process_virtio_queues().unwrap();
10841091
}
10851092

10861093
#[test]

‎src/vmm/src/devices/virtio/balloon/mod.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ mod util;
1313
use log::error;
1414

1515
pub use self::device::{Balloon, BalloonConfig, BalloonStats};
16-
use super::queue::QueueError;
16+
use super::queue::{InvalidAvailIdx, QueueError};
1717
use crate::devices::virtio::balloon::metrics::METRICS;
1818
use crate::devices::virtio::queue::FIRECRACKER_MAX_QUEUE_SIZE;
1919
use crate::logger::IncMetric;
@@ -89,6 +89,8 @@ pub enum BalloonError {
8989
TooManyPagesRequested,
9090
/// Error while processing the virt queues: {0}
9191
Queue(#[from] QueueError),
92+
/// {0}
93+
InvalidAvailIdx(#[from] InvalidAvailIdx),
9294
/// Error creating the statistics timer: {0}
9395
Timer(std::io::Error),
9496
}
@@ -108,6 +110,9 @@ pub enum RemoveRegionError {
108110
}
109111

110112
pub(super) fn report_balloon_event_fail(err: BalloonError) {
113+
if let BalloonError::InvalidAvailIdx(err) = err {
114+
panic!("{}", err);
115+
}
111116
error!("{:?}", err);
112117
METRICS.event_fails.inc();
113118
}

‎src/vmm/src/devices/virtio/block/device.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use super::persist::{BlockConstructorArgs, BlockState};
99
use super::vhost_user::device::{VhostUserBlock, VhostUserBlockConfig};
1010
use super::virtio::device::{VirtioBlock, VirtioBlockConfig};
1111
use crate::devices::virtio::device::{IrqTrigger, VirtioDevice};
12-
use crate::devices::virtio::queue::Queue;
12+
use crate::devices::virtio::queue::{InvalidAvailIdx, Queue};
1313
use crate::devices::virtio::{ActivateError, TYPE_BLOCK};
1414
use crate::rate_limiter::BucketUpdate;
1515
use crate::snapshot::Persist;
@@ -83,10 +83,10 @@ impl Block {
8383
}
8484
}
8585

86-
pub fn process_virtio_queues(&mut self) {
86+
pub fn process_virtio_queues(&mut self) -> Result<(), InvalidAvailIdx> {
8787
match self {
8888
Self::Virtio(b) => b.process_virtio_queues(),
89-
Self::VhostUser(_) => {}
89+
Self::VhostUser(_) => Ok(()),
9090
}
9191
}
9292

‎src/vmm/src/devices/virtio/block/virtio/device.rs

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ use crate::devices::virtio::generated::virtio_blk::{
2929
};
3030
use crate::devices::virtio::generated::virtio_config::VIRTIO_F_VERSION_1;
3131
use crate::devices::virtio::generated::virtio_ring::VIRTIO_RING_F_EVENT_IDX;
32-
use crate::devices::virtio::queue::Queue;
32+
use crate::devices::virtio::queue::{InvalidAvailIdx, Queue};
3333
use crate::devices::virtio::{ActivateError, TYPE_BLOCK};
3434
use crate::logger::{IncMetric, error, warn};
3535
use crate::rate_limiter::{BucketUpdate, RateLimiter};
@@ -366,21 +366,21 @@ impl VirtioBlock {
366366
} else if self.is_io_engine_throttled {
367367
self.metrics.io_engine_throttled_events.inc();
368368
} else {
369-
self.process_virtio_queues();
369+
self.process_virtio_queues().unwrap()
370370
}
371371
}
372372

373373
/// Process device virtio queue(s).
374-
pub fn process_virtio_queues(&mut self) {
375-
self.process_queue(0);
374+
pub fn process_virtio_queues(&mut self) -> Result<(), InvalidAvailIdx> {
375+
self.process_queue(0)
376376
}
377377

378378
pub(crate) fn process_rate_limiter_event(&mut self) {
379379
self.metrics.rate_limiter_event_count.inc();
380380
// Upon rate limiter event, call the rate limiter handler
381381
// and restart processing the queue.
382382
if self.rate_limiter.event_handler().is_ok() {
383-
self.process_queue(0);
383+
self.process_queue(0).unwrap()
384384
}
385385
}
386386

@@ -403,14 +403,14 @@ impl VirtioBlock {
403403
}
404404

405405
/// Device specific function for peaking inside a queue and processing descriptors.
406-
pub fn process_queue(&mut self, queue_index: usize) {
406+
pub fn process_queue(&mut self, queue_index: usize) -> Result<(), InvalidAvailIdx> {
407407
// This is safe since we checked in the event handler that the device is activated.
408408
let mem = self.device_state.mem().unwrap();
409409

410410
let queue = &mut self.queues[queue_index];
411411
let mut used_any = false;
412412

413-
while let Some(head) = queue.pop_or_enable_notification() {
413+
while let Some(head) = queue.pop_or_enable_notification()? {
414414
self.metrics.remaining_reqs_count.add(queue.len().into());
415415
let processing_result = match Request::parse(&head, mem, self.disk.nsectors) {
416416
Ok(request) => {
@@ -463,6 +463,8 @@ impl VirtioBlock {
463463
if !used_any {
464464
self.metrics.no_avail_buffer.inc();
465465
}
466+
467+
Ok(())
466468
}
467469

468470
fn process_async_completion_queue(&mut self) {
@@ -516,7 +518,7 @@ impl VirtioBlock {
516518

517519
if self.is_io_engine_throttled {
518520
self.is_io_engine_throttled = false;
519-
self.process_queue(0);
521+
self.process_queue(0).unwrap()
520522
}
521523
}
522524
}

‎src/vmm/src/devices/virtio/block/virtio/request.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -484,15 +484,16 @@ mod tests {
484484
let memory = self.driver_queue.memory();
485485

486486
assert!(matches!(
487-
Request::parse(&q.pop().unwrap(), memory, NUM_DISK_SECTORS),
487+
Request::parse(&q.pop().unwrap().unwrap(), memory, NUM_DISK_SECTORS),
488488
Err(_e)
489489
));
490490
}
491491

492492
fn check_parse(&self, check_data: bool) {
493493
let mut q = self.driver_queue.create_queue();
494494
let memory = self.driver_queue.memory();
495-
let request = Request::parse(&q.pop().unwrap(), memory, NUM_DISK_SECTORS).unwrap();
495+
let request =
496+
Request::parse(&q.pop().unwrap().unwrap(), memory, NUM_DISK_SECTORS).unwrap();
496497
let expected_header = self.header();
497498

498499
assert_eq!(
@@ -959,7 +960,7 @@ mod tests {
959960
fn parse_random_requests() {
960961
let cfg = ProptestConfig::with_cases(1000);
961962
proptest!(cfg, |(mut request in random_request_parse())| {
962-
let result = Request::parse(&request.2.pop().unwrap(), &request.1, NUM_DISK_SECTORS);
963+
let result = Request::parse(&request.2.pop().unwrap().unwrap(), &request.1, NUM_DISK_SECTORS);
963964
match result {
964965
Ok(r) => prop_assert!(r == request.0.unwrap()),
965966
Err(err) => {

‎src/vmm/src/devices/virtio/iovec.rs

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -611,22 +611,22 @@ mod tests {
611611
fn test_access_mode() {
612612
let mem = default_mem();
613613
let (mut q, _) = read_only_chain(&mem);
614-
let head = q.pop().unwrap();
614+
let head = q.pop().unwrap().unwrap();
615615
// SAFETY: This descriptor chain is only loaded into one buffer
616616
unsafe { IoVecBuffer::from_descriptor_chain(&mem, head).unwrap() };
617617

618618
let (mut q, _) = write_only_chain(&mem);
619-
let head = q.pop().unwrap();
619+
let head = q.pop().unwrap().unwrap();
620620
// SAFETY: This descriptor chain is only loaded into one buffer
621621
unsafe { IoVecBuffer::from_descriptor_chain(&mem, head).unwrap_err() };
622622

623623
let (mut q, _) = read_only_chain(&mem);
624-
let head = q.pop().unwrap();
624+
let head = q.pop().unwrap().unwrap();
625625
// SAFETY: This descriptor chain is only loaded into one buffer
626626
unsafe { IoVecBufferMutDefault::from_descriptor_chain(&mem, head).unwrap_err() };
627627

628628
let (mut q, _) = write_only_chain(&mem);
629-
let head = q.pop().unwrap();
629+
let head = q.pop().unwrap().unwrap();
630630
// SAFETY: This descriptor chain is only loaded into one buffer
631631
unsafe { IoVecBufferMutDefault::from_descriptor_chain(&mem, head).unwrap() };
632632
}
@@ -635,7 +635,7 @@ mod tests {
635635
fn test_iovec_length() {
636636
let mem = default_mem();
637637
let (mut q, _) = read_only_chain(&mem);
638-
let head = q.pop().unwrap();
638+
let head = q.pop().unwrap().unwrap();
639639

640640
// SAFETY: This descriptor chain is only loaded once in this test
641641
let iovec = unsafe { IoVecBuffer::from_descriptor_chain(&mem, head).unwrap() };
@@ -646,7 +646,7 @@ mod tests {
646646
fn test_iovec_mut_length() {
647647
let mem = default_mem();
648648
let (mut q, _) = write_only_chain(&mem);
649-
let head = q.pop().unwrap();
649+
let head = q.pop().unwrap().unwrap();
650650

651651
// SAFETY: This descriptor chain is only loaded once in this test
652652
let mut iovec =
@@ -658,7 +658,7 @@ mod tests {
658658
// (concpetually) associated with a single `Queue`. We just do this here to be able to test
659659
// the appending logic.
660660
let (mut q, _) = write_only_chain(&mem);
661-
let head = q.pop().unwrap();
661+
let head = q.pop().unwrap().unwrap();
662662
// SAFETY: it is actually unsafe, but we just want to check the length of the
663663
// `IoVecBufferMut` after appending.
664664
let _ = unsafe { iovec.append_descriptor_chain(&mem, head).unwrap() };
@@ -669,7 +669,7 @@ mod tests {
669669
fn test_iovec_read_at() {
670670
let mem = default_mem();
671671
let (mut q, _) = read_only_chain(&mem);
672-
let head = q.pop().unwrap();
672+
let head = q.pop().unwrap().unwrap();
673673

674674
// SAFETY: This descriptor chain is only loaded once in this test
675675
let iovec = unsafe { IoVecBuffer::from_descriptor_chain(&mem, head).unwrap() };
@@ -724,7 +724,7 @@ mod tests {
724724
let (mut q, vq) = write_only_chain(&mem);
725725

726726
// This is a descriptor chain with 4 elements 64 bytes long each.
727-
let head = q.pop().unwrap();
727+
let head = q.pop().unwrap().unwrap();
728728

729729
// SAFETY: This descriptor chain is only loaded into one buffer
730730
let mut iovec =

0 commit comments

Comments
 (0)