virtio net: check vnet header in tx frames

This commit removes a potential panic condition caused by
the virtio net device expecting to find a vnet header in
every frame. In case a malformed shorter frame shows up
on the TX path (large enough buffers make sure that it can't
happen on RX), error messages are now logged and a dedicated
metric is emitted. With this occasion, several unused device
error variants have been removed.

Signed-off-by: Alexandra Iordache <[email protected]>

Author: Alexandra Iordache

CHANGELOG.md

@@ -18,16 +18,17 @@
   requests.
 - Added metrics for the vsock device. 
 - Added devtool strip command which removes debug symbols from the release
-  binaries.
-- Any number of whitespace characters are accepted after ":" when parsing HTTP
-  headers.
+- Added the `tx_malformed_frames` metric for the virtio net device, emitted
+  when a TX frame missing the VNET header is encountered.
 
 ### Fixed
 
 - Added `--version` flag to both Firecracker and Jailer.
 - Return `405 Method Not Allowed` MMDS response for non HTTP `GET` MMDS
   requests originating from guest.
 - Fixed folder permissions in the jail (#1802).
+- Any number of whitespace characters are accepted after ":" when parsing HTTP
+  headers.
 
 ### Changed
 - Updated CVE-2019-3016 mitigation information in

src/devices/src/lib.rs

@@ -20,7 +20,6 @@ extern crate versionize;
 extern crate versionize_derive;
 extern crate vm_memory;
 
-use rate_limiter::Error as RateLimiterError;
 use std::io;
 
 mod bus;
@@ -39,15 +38,12 @@ pub(crate) fn report_net_event_fail(err: Error) {
 
 #[derive(Debug)]
 pub enum Error {
-    FailedReadingQueue {
-        event_type: &'static str,
-        underlying: io::Error,
-    },
+    /// Failed to read from the TAP device.
     FailedReadTap,
+    /// Failed to signal the virtio used queue.
     FailedSignalingUsedQueue(io::Error),
-    RateLimited(RateLimiterError),
-    PayloadExpected,
+    /// IO error.
     IoError(io::Error),
-    NoAvailBuffers,
-    SpuriousEvent,
+    /// Device received malformed payload.
+    MalformedPayload,
 }

src/devices/src/virtio/net/device.rs

@@ -17,13 +17,14 @@ use dumbo::{EthernetFrame, MacAddr, MAC_ADDR_LEN};
 use libc::EAGAIN;
 use logger::{Metric, METRICS};
 use rate_limiter::{BucketUpdate, RateLimiter, TokenType};
-#[cfg(not(test))]
-use std::io::Read;
+
 use std::io::Write;
+#[cfg(not(test))]
+use std::io::{self, Read};
 use std::sync::atomic::AtomicUsize;
 use std::sync::atomic::Ordering;
 use std::sync::Arc;
-use std::{cmp, io, mem, result};
+use std::{cmp, mem, result};
 use utils::eventfd::EventFd;
 use utils::net::Tap;
 use virtio_gen::virtio_net::{
@@ -39,12 +40,20 @@ fn vnet_hdr_len() -> usize {
 
 // Frames being sent/received through the network device model have a VNET header. This
 // function returns a slice which holds the L2 frame bytes without this header.
-fn frame_bytes_from_buf(buf: &[u8]) -> &[u8] {
-    &buf[vnet_hdr_len()..]
+fn frame_bytes_from_buf(buf: &[u8]) -> Result<&[u8]> {
+    if buf.len() < vnet_hdr_len() {
+        Err(Error::VnetHeaderMissing)
+    } else {
+        Ok(&buf[vnet_hdr_len()..])
+    }
 }
 
-fn frame_bytes_from_buf_mut(buf: &mut [u8]) -> &mut [u8] {
-    &mut buf[vnet_hdr_len()..]
+fn frame_bytes_from_buf_mut(buf: &mut [u8]) -> Result<&mut [u8]> {
+    if buf.len() < vnet_hdr_len() {
+        Err(Error::VnetHeaderMissing)
+    } else {
+        Ok(&mut buf[vnet_hdr_len()..])
+    }
 }
 
 // This initializes to all 0 the VNET hdr part of a buf.
@@ -332,34 +341,40 @@ impl Net {
         frame_buf: &[u8],
         tap: &mut Tap,
         guest_mac: Option<MacAddr>,
-    ) -> bool {
+    ) -> Result<bool> {
+        let checked_frame = |frame_buf| {
+            frame_bytes_from_buf(frame_buf).map_err(|e| {
+                error!("VNET header missing in the TX frame.");
+                METRICS.net.tx_malformed_frames.inc();
+                e
+            })
+        };
         if let Some(ns) = mmds_ns {
-            if ns.detour_frame(frame_bytes_from_buf(frame_buf)) {
+            if ns.detour_frame(checked_frame(frame_buf)?) {
                 METRICS.mmds.rx_accepted.inc();
 
                 // MMDS frames are not accounted by the rate limiter.
                 rate_limiter.manual_replenish(frame_buf.len() as u64, TokenType::Bytes);
                 rate_limiter.manual_replenish(1, TokenType::Ops);
 
                 // MMDS consumed the frame.
-                return true;
+                return Ok(true);
             }
         }
 
         // This frame goes to the TAP.
 
         // Check for guest MAC spoofing.
         if let Some(mac) = guest_mac {
-            let _ = EthernetFrame::from_bytes(&frame_buf[vnet_hdr_len()..]).and_then(|eth_frame| {
+            let _ = EthernetFrame::from_bytes(checked_frame(frame_buf)?).and_then(|eth_frame| {
                 if mac != eth_frame.src_mac() {
                     METRICS.net.tx_spoofed_mac_count.inc();
                 }
                 Ok(())
             });
         }
 
-        let write_result = tap.write(frame_buf);
-        match write_result {
+        match tap.write(frame_buf) {
             Ok(_) => {
                 METRICS.net.tx_bytes_count.add(frame_buf.len());
                 METRICS.net.tx_packets_count.inc();
@@ -370,13 +385,14 @@ impl Net {
                 METRICS.net.tx_fails.inc();
             }
         };
-        false
+        Ok(false)
     }
 
     // We currently prioritize packets from the MMDS over regular network packets.
-    fn read_from_mmds_or_tap(&mut self) -> io::Result<usize> {
+    fn read_from_mmds_or_tap(&mut self) -> Result<usize> {
         if let Some(ns) = self.mmds_ns.as_mut() {
-            if let Some(len) = ns.write_next_frame(frame_bytes_from_buf_mut(&mut self.rx_frame_buf))
+            if let Some(len) =
+                ns.write_next_frame(frame_bytes_from_buf_mut(&mut self.rx_frame_buf)?)
             {
                 let len = len.get();
                 METRICS.mmds.tx_frames.inc();
@@ -386,7 +402,7 @@ impl Net {
             }
         }
 
-        self.read_tap()
+        self.read_tap().map_err(Error::IO)
     }
 
     fn process_rx(&mut self) -> result::Result<(), DeviceError> {
@@ -401,7 +417,7 @@ impl Net {
                         break;
                     }
                 }
-                Err(e) => {
+                Err(Error::IO(e)) => {
                     // The tap device is non-blocking, so any error aside from EAGAIN is
                     // unexpected.
                     match e.raw_os_error() {
@@ -414,6 +430,9 @@ impl Net {
                     };
                     break;
                 }
+                Err(e) => {
+                    error!("Spurious error in network RX: {:?}", e);
+                }
             }
         }
 
@@ -523,14 +542,15 @@ impl Net {
                 }
             }
 
-            if Self::write_to_mmds_or_tap(
+            let frame_consumed_by_mmds = Self::write_to_mmds_or_tap(
                 self.mmds_ns.as_mut(),
                 &mut self.tx_rate_limiter,
                 &self.tx_frame_buf[..read_count],
                 &mut self.tap,
                 self.guest_mac,
-            ) && !self.rx_deferred_frame
-            {
+            )
+            .unwrap_or_else(|_| false);
+            if frame_consumed_by_mmds && !self.rx_deferred_frame {
                 // MMDS consumed this frame/request, let's also try to process the response.
                 process_rx_for_mmds = true;
             }
@@ -893,6 +913,16 @@ pub(crate) mod tests {
 
     #[test]
     fn test_vnet_helpers() {
+        let mut frame_buf = vec![42u8; vnet_hdr_len() - 1];
+        assert_eq!(
+            format!("{:?}", frame_bytes_from_buf(&frame_buf)),
+            "Err(VnetHeaderMissing)"
+        );
+        assert_eq!(
+            format!("{:?}", frame_bytes_from_buf_mut(&mut frame_buf)),
+            "Err(VnetHeaderMissing)"
+        );
+
         let mut frame_buf: [u8; MAX_BUFFER_SIZE] = [42u8; MAX_BUFFER_SIZE];
 
         let vnet_hdr_len_ = mem::size_of::<virtio_net_hdr_v1>();
@@ -903,10 +933,10 @@ pub(crate) mod tests {
         assert_eq!(zero_vnet_hdr, &frame_buf[..vnet_hdr_len_]);
 
         let payload = vec![42u8; MAX_BUFFER_SIZE - vnet_hdr_len_];
-        assert_eq!(payload, frame_bytes_from_buf(&frame_buf));
+        assert_eq!(payload, frame_bytes_from_buf(&frame_buf).unwrap());
 
         {
-            let payload = frame_bytes_from_buf_mut(&mut frame_buf);
+            let payload = frame_bytes_from_buf_mut(&mut frame_buf).unwrap();
             payload[0] = 15;
         }
         assert_eq!(frame_buf[vnet_hdr_len_], 15);
@@ -996,6 +1026,7 @@ pub(crate) mod tests {
     }
 
     #[test]
+    #[allow(clippy::cognitive_complexity)]
     fn test_event_processing() {
         let mut event_manager = EventManager::new().unwrap();
         let mut net = Net::default_net(TestMutators::default());
@@ -1050,20 +1081,39 @@ pub(crate) mod tests {
             net.rx_bytes_read = 0;
         }
 
-        // Now let's move on to the actual device events.
         {
-            // testing TX_QUEUE_EVENT
+            // Send an invalid frame (too small, VNET header missing).
             txq.avail.idx.set(1);
             txq.avail.ring[0].set(0);
-            txq.dtable[0].set(daddr, 0x1000, 0, 0);
+            txq.dtable[0].set(daddr, 1, 0, 0);
 
+            // Trigger the TX handler.
             net.queue_evts[TX_INDEX].write(1).unwrap();
             let event = EpollEvent::new(EventSet::IN, net.queue_evts[TX_INDEX].as_raw_fd() as u64);
-            net.process(&event, &mut event_manager);
+            check_metric_after_block!(
+                &METRICS.net.tx_malformed_frames,
+                1,
+                net.process(&event, &mut event_manager)
+            );
+
             // Make sure the data queue advanced.
             assert_eq!(txq.used.idx.get(), 1);
         }
 
+        // Now let's move on to the actual device events.
+        {
+            // testing TX_QUEUE_EVENT
+            txq.avail.idx.set(2);
+            txq.avail.ring[1].set(1);
+            txq.dtable[1].set(daddr, 0x1000, 0, 0);
+
+            net.queue_evts[TX_INDEX].write(1).unwrap();
+            let event = EpollEvent::new(EventSet::IN, net.queue_evts[TX_INDEX].as_raw_fd() as u64);
+            net.process(&event, &mut event_manager);
+            // Make sure the data queue advanced.
+            assert_eq!(txq.used.idx.get(), 2);
+        }
+
         {
             // testing RX_TAP_EVENT
 
@@ -1078,7 +1128,7 @@ pub(crate) mod tests {
             let tap_event = EpollEvent::new(EventSet::IN, net.tap.as_raw_fd() as u64);
             net.process(&tap_event, &mut event_manager);
             assert!(net.rx_deferred_frame);
-            assert_eq!(net.interrupt_evt.read().unwrap(), 3);
+            assert_eq!(net.interrupt_evt.read().unwrap(), 4);
             // The #cfg(test) enabled version of read_tap always returns 1234 bytes (or the len of
             // the buffer, whichever is smaller).
             assert_eq!(rxq.used.ring[0].get().len, 1234);
@@ -1162,7 +1212,7 @@ pub(crate) mod tests {
         {
             // Create an ethernet frame.
             let eth_frame_i = EthernetFrame::write_incomplete(
-                frame_bytes_from_buf_mut(&mut net.tx_frame_buf),
+                frame_bytes_from_buf_mut(&mut net.tx_frame_buf).unwrap(),
                 tha,
                 sha,
                 ETHERTYPE_ARP,
@@ -1198,7 +1248,8 @@ pub(crate) mod tests {
                 &net.tx_frame_buf[..packet_len],
                 &mut net.tap,
                 Some(sha),
-            ))
+            )
+            .unwrap())
         );
 
         // Validate that MMDS has a response and we can retrieve it.
@@ -1223,7 +1274,7 @@ pub(crate) mod tests {
         {
             // Create an ethernet frame.
             let eth_frame_i = EthernetFrame::write_incomplete(
-                frame_bytes_from_buf_mut(&mut net.tx_frame_buf),
+                frame_bytes_from_buf_mut(&mut net.tx_frame_buf).unwrap(),
                 dst_mac,
                 guest_mac,
                 ETHERTYPE_ARP,

src/devices/src/virtio/net/mod.rs

@@ -30,8 +30,12 @@ pub enum Error {
     TapSetVnetHdrSize(TapError),
     /// Enabling tap interface failed.
     TapEnable(TapError),
-    /// EventFd
+    /// EventFd error.
     EventFd(io::Error),
+    /// IO error.
+    IO(io::Error),
+    /// The VNET header is missing from the frame.
+    VnetHeaderMissing,
 }
 
 pub type Result<T> = result::Result<T, Error>;

src/logger/src/metrics.rs

@@ -440,6 +440,8 @@ pub struct NetDeviceMetrics {
     pub rx_count: SharedMetric,
     /// Number of transmitted bytes.
     pub tx_bytes_count: SharedMetric,
+    /// Number of malformed TX frames.
+    pub tx_malformed_frames: SharedMetric,
     /// Number of errors while transmitting data.
     pub tx_fails: SharedMetric,
     /// Number of successful write operations while transmitting data.