diff --git a/CHANGELOG.md b/CHANGELOG.md index bca8729ab48..4a1dce4eb73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,15 @@ and this project adheres to ### Fixed +- [4526](https://github.com/firecracker-microvm/firecracker/pull/4526): Added a + check in the network TX path that the size of the network frames the guest + passes to us is not bigger than the maximum frame the device expects to + handle. On the TX path, we copy frames destined to MMDS from guest memory to + Firecracker memory. Without the check, a mis-behaving virtio-net driver could + cause an increase in the memory footprint of the Firecracker process. Now, if + we receive such a frame, we ignore it and increase `Net::tx_malformed_frames` + metric. + ## \[1.7.0\] ### Added diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index 749ccc2ec98..0e7a77aadb6 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -607,6 +607,17 @@ impl Net { continue; } }; + + // We only handle frames that are up to MAX_BUFFER_SIZE + if buffer.len() > MAX_BUFFER_SIZE { + error!("net: received too big frame from driver"); + self.metrics.tx_malformed_frames.inc(); + tx_queue + .add_used(mem, head_index, 0) + .map_err(DeviceError::QueueError)?; + continue; + } + if !Self::rate_limiter_consume_op(&mut self.tx_rate_limiter, buffer.len() as u64) { tx_queue.undo_pop(); self.metrics.tx_rate_limiter_throttled.inc(); @@ -1323,6 +1334,32 @@ pub mod tests { assert!(!tap_traffic_simulator.pop_rx_packet(&mut [])); } + #[test] + fn test_tx_big_frame() { + let mut th = TestHelper::get_default(); + th.activate_net(); + let tap_traffic_simulator = TapTrafficSimulator::new(if_index(&th.net().tap)); + + // Send an invalid frame (too big, maximum buffer is MAX_BUFFER_SIZE). + th.add_desc_chain( + NetQueue::Tx, + 0, + &[(0, (MAX_BUFFER_SIZE + 1).try_into().unwrap(), 0)], + ); + check_metric_after_block!( + th.net().metrics.tx_malformed_frames, + 1, + th.event_manager.run_with_timeout(100) + ); + + // Check that the used queue advanced. + assert_eq!(th.txq.used.idx.get(), 1); + assert!(&th.net().irq_trigger.has_pending_irq(IrqType::Vring)); + th.txq.check_used_elem(0, 0, 0); + // Check that the frame was skipped. + assert!(!tap_traffic_simulator.pop_rx_packet(&mut [])); + } + #[test] fn test_tx_empty_frame() { let mut th = TestHelper::get_default(); diff --git a/src/vmm/src/devices/virtio/net/test_utils.rs b/src/vmm/src/devices/virtio/net/test_utils.rs index b4222a8702d..c2bffa1d2a9 100644 --- a/src/vmm/src/devices/virtio/net/test_utils.rs +++ b/src/vmm/src/devices/virtio/net/test_utils.rs @@ -395,7 +395,7 @@ pub mod test { pub fn get_default() -> TestHelper<'a> { let mut event_manager = EventManager::new().unwrap(); let mut net = default_net(); - let mem = single_region_mem(MAX_BUFFER_SIZE); + let mem = single_region_mem(2 * MAX_BUFFER_SIZE); // transmute mem_ref lifetime to 'a let mem_ref = unsafe { mem::transmute::<&GuestMemoryMmap, &'a GuestMemoryMmap>(&mem) };