Skip to content

Commit 42c8851

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 <[email protected]>
1 parent f8f66df commit 42c8851

File tree

20 files changed

+242
-141
lines changed

20 files changed

+242
-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: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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::QueueError(QueueError::InvalidAvailIdx(iai)) = err {
32+
panic!("{}", iai);
33+
}
3134
error!("{:?}", err);
3235
net_iface_metrics.event_fails.inc();
3336
}

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

Lines changed: 19 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, QueueError};
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().map_err(QueueError::InvalidAvailIdx)? {
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().map_err(QueueError::InvalidAvailIdx)? {
379380
queue.add_used(head.index, 0)?;
380381
needs_interrupt = true;
381382
}
@@ -392,7 +393,10 @@ 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]
397+
.pop()
398+
.map_err(QueueError::InvalidAvailIdx)?
399+
{
396400
if let Some(prev_stats_desc) = self.stats_desc_index {
397401
// We shouldn't ever have an extra buffer if the driver follows
398402
// the protocol, but return it if we find one.
@@ -431,9 +435,17 @@ impl Balloon {
431435
}
432436

433437
/// Process device virtio queue(s).
434-
pub fn process_virtio_queues(&mut self) {
435-
let _ = self.process_inflate();
436-
let _ = self.process_deflate_queue();
438+
pub fn process_virtio_queues(&mut self) -> Result<(), InvalidAvailIdx> {
439+
if let Err(BalloonError::Queue(QueueError::InvalidAvailIdx(iai))) = self.process_inflate() {
440+
return Err(iai);
441+
}
442+
if let Err(BalloonError::Queue(QueueError::InvalidAvailIdx(iai))) =
443+
self.process_deflate_queue()
444+
{
445+
return Err(iai);
446+
}
447+
448+
Ok(())
437449
}
438450

439451
/// Provides the ID of this balloon device.
@@ -1080,7 +1092,7 @@ pub(crate) mod tests {
10801092
balloon.set_queue(DEFLATE_INDEX, defq.create_queue());
10811093

10821094
balloon.activate(mem).unwrap();
1083-
balloon.process_virtio_queues()
1095+
balloon.process_virtio_queues().unwrap();
10841096
}
10851097

10861098
#[test]

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,9 @@ pub enum RemoveRegionError {
108108
}
109109

110110
pub(super) fn report_balloon_event_fail(err: BalloonError) {
111+
if let BalloonError::Queue(QueueError::InvalidAvailIdx(iai)) = err {
112+
panic!("{}", iai);
113+
}
111114
error!("{:?}", err);
112115
METRICS.event_fails.inc();
113116
}

‎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)