netvsp: handle invalid rndis packets (#1590) (#1646)

Netvsp driver does not accept an RNDIS header from Linux NetVSC, if that
RNDIS header crosses a 4KB page boundary. handle_rndis_packet_message
will return WorkerError::RndisMessageTooSmall. The error bubbles up out
of process_ring_buffer causing netvsp to stop processing packets.

RNDIS headers crossing the page boundary has been a longstanding issue
with the Linux NetVSC driver, but it has become more frequent recently
with the Linux v6.14 kernel due to a recent upstream commit in the Linux
networking subsystem that may have slightly affected the way SKBs are
constructed.

Making the OpenHCL Netvsp driver more resilient (and more like the
Windows Netvsp driver) by adding logic to log the error and complete the
packet with a Failure status.

Cherry-pick of #1590

Co-authored-by: erfrimod <31358361+erfrimod@users.noreply.github.com>

Author: benhillis

vm/devices/net/netvsp/src/lib.rs

@@ -454,6 +454,7 @@ struct QueueStats {
 struct PendingTxCompletion {
     transaction_id: u64,
     tx_id: Option<TxId>,
+    status: protocol::Status,
 }
 
 #[derive(Clone, Copy)]
@@ -934,11 +935,14 @@ impl ActiveState {
                 // This shouldn't be referenced, but set it in case it is in the future.
                 active.pending_tx_packets[id.0 as usize].transaction_id = transaction_id;
             }
+            // Save/Restore does not preserve the status of pending tx completions,
+            // completing any pending completions with 'success' to avoid making changes to saved_state.
             active
                 .pending_tx_completions
                 .push_back(PendingTxCompletion {
                     transaction_id,
                     tx_id,
+                    status: protocol::Status::SUCCESS,
                 });
         }
         Ok(active)
@@ -4956,7 +4960,7 @@ impl<T: 'static + RingMem> NetChannel<T> {
                     assert!(tx_packet.pending_packet_count > 0);
                     tx_packet.pending_packet_count -= 1;
                     if tx_packet.pending_packet_count == 0 {
-                        self.complete_tx_packet(state, id)?;
+                        self.complete_tx_packet(state, id, protocol::Status::SUCCESS)?;
                     }
                 }
 
@@ -4974,6 +4978,7 @@ impl<T: 'static + RingMem> NetChannel<T> {
                             Some(PendingTxCompletion {
                                 transaction_id: inflight.transaction_id,
                                 tx_id: Some(TxId(id as u32)),
+                                status: protocol::Status::SUCCESS,
                             })
                         } else {
                             None
@@ -5070,8 +5075,19 @@ impl<T: 'static + RingMem> NetChannel<T> {
                 PacketData::RndisPacket(_) => {
                     assert!(data.tx_segments.is_empty());
                     let id = state.free_tx_packets.pop().unwrap();
-                    let num_packets =
-                        self.handle_rndis(buffers, id, state, &packet, &mut data.tx_segments)?;
+                    let result: Result<usize, WorkerError> =
+                        self.handle_rndis(buffers, id, state, &packet, &mut data.tx_segments);
+                    let num_packets = match result {
+                        Ok(num_packets) => num_packets,
+                        Err(err) => {
+                            tracelimit::error_ratelimited!(
+                                err = &err as &dyn std::error::Error,
+                                "failed to handle RNDIS packet"
+                            );
+                            self.complete_tx_packet(state, id, protocol::Status::FAILURE)?;
+                            continue;
+                        }
+                    };
                     total_packets += num_packets as u64;
                     state.pending_tx_packets[id.0 as usize].pending_packet_count += num_packets;
 
@@ -5082,7 +5098,7 @@ impl<T: 'static + RingMem> NetChannel<T> {
                             state.stats.tx_stalled.increment();
                         }
                     } else {
-                        self.complete_tx_packet(state, id)?;
+                        self.complete_tx_packet(state, id, protocol::Status::SUCCESS)?;
                     }
                 }
                 PacketData::RndisPacketComplete(_completion) => {
@@ -5222,7 +5238,7 @@ impl<T: 'static + RingMem> NetChannel<T> {
         }
 
         if state.pending_tx_packets[id.0 as usize].pending_packet_count == 0 {
-            self.complete_tx_packet(state, id)?;
+            self.complete_tx_packet(state, id, protocol::Status::SUCCESS)?;
         }
 
         Ok(packets_sent)
@@ -5276,12 +5292,14 @@ impl<T: 'static + RingMem> NetChannel<T> {
         Ok(num_packets)
     }
 
-    fn try_send_tx_packet(&mut self, transaction_id: u64) -> Result<bool, WorkerError> {
+    fn try_send_tx_packet(
+        &mut self,
+        transaction_id: u64,
+        status: protocol::Status,
+    ) -> Result<bool, WorkerError> {
         let message = self.message(
             protocol::MESSAGE1_TYPE_SEND_RNDIS_PACKET_COMPLETE,
-            protocol::Message1SendRndisPacketComplete {
-                status: protocol::Status::SUCCESS,
-            },
+            protocol::Message1SendRndisPacketComplete { status },
         );
         let result = self.queue.split().1.try_write(&queue::OutgoingPacket {
             transaction_id,
@@ -5302,7 +5320,7 @@ impl<T: 'static + RingMem> NetChannel<T> {
     fn send_pending_packets(&mut self, state: &mut ActiveState) -> Result<bool, WorkerError> {
         let mut did_some_work = false;
         while let Some(pending) = state.pending_tx_completions.front() {
-            if !self.try_send_tx_packet(pending.transaction_id)? {
+            if !self.try_send_tx_packet(pending.transaction_id, pending.status)? {
                 return Ok(did_some_work);
             }
             did_some_work = true;
@@ -5317,17 +5335,25 @@ impl<T: 'static + RingMem> NetChannel<T> {
         Ok(did_some_work)
     }
 
-    fn complete_tx_packet(&mut self, state: &mut ActiveState, id: TxId) -> Result<(), WorkerError> {
+    fn complete_tx_packet(
+        &mut self,
+        state: &mut ActiveState,
+        id: TxId,
+        status: protocol::Status,
+    ) -> Result<(), WorkerError> {
         let tx_packet = &mut state.pending_tx_packets[id.0 as usize];
         assert_eq!(tx_packet.pending_packet_count, 0);
-        if self.pending_send_size == 0 && self.try_send_tx_packet(tx_packet.transaction_id)? {
+        if self.pending_send_size == 0
+            && self.try_send_tx_packet(tx_packet.transaction_id, status)?
+        {
             tracing::trace!(id = id.0, "sent tx completion");
             state.free_tx_packets.push(id);
         } else {
             tracing::trace!(id = id.0, "pended tx completion");
             state.pending_tx_completions.push_back(PendingTxCompletion {
                 transaction_id: tx_packet.transaction_id,
                 tx_id: Some(id),
+                status,
             });
         }
         Ok(())

vm/devices/net/netvsp/src/test.rs

@@ -2296,6 +2296,134 @@ async fn initialize_rndis_with_prev_vf_switch_data_path(driver: DefaultDriver) {
     assert_eq!(endpoint_state.lock().use_vf.take().unwrap(), false);
 }
 
+#[async_test]
+async fn rndis_handle_packet_errors(driver: DefaultDriver) {
+    let endpoint_state = TestNicEndpointState::new();
+    let endpoint = TestNicEndpoint::new(Some(endpoint_state.clone()));
+    let builder = Nic::builder();
+    let nic = builder.build(
+        &VmTaskDriverSource::new(SingleDriverBackend::new(driver.clone())),
+        Guid::new_random(),
+        Box::new(endpoint),
+        [1, 2, 3, 4, 5, 6].into(),
+        0,
+    );
+
+    let mut nic = TestNicDevice::new_with_nic(&driver, nic).await;
+    nic.start_vmbus_channel();
+    let mut channel = nic.connect_vmbus_channel().await;
+    channel
+        .initialize(0, protocol::NdisConfigCapabilities::new())
+        .await;
+    channel
+        .send_rndis_control_message(
+            rndisprot::MESSAGE_TYPE_INITIALIZE_MSG,
+            rndisprot::InitializeRequest {
+                request_id: 123,
+                major_version: rndisprot::MAJOR_VERSION,
+                minor_version: rndisprot::MINOR_VERSION,
+                max_transfer_size: 0,
+            },
+            &[],
+        )
+        .await;
+
+    let initialize_complete: rndisprot::InitializeComplete = channel
+        .read_rndis_control_message(rndisprot::MESSAGE_TYPE_INITIALIZE_CMPLT)
+        .await
+        .unwrap();
+    assert_eq!(initialize_complete.request_id, 123);
+    assert_eq!(initialize_complete.status, rndisprot::STATUS_SUCCESS);
+    assert_eq!(initialize_complete.major_version, rndisprot::MAJOR_VERSION);
+    assert_eq!(initialize_complete.minor_version, rndisprot::MINOR_VERSION);
+
+    // Not expecting an association packet because virtual function is not present
+    assert!(
+        channel
+            .read_with(|_| panic!("No packet expected"))
+            .await
+            .is_err()
+    );
+
+    assert_eq!(endpoint_state.lock().stop_endpoint_counter, 1);
+
+    // Send a packet with an invalid data offset.
+    // Expecting the channel to handle WorkerError::RndisMessageTooSmall
+    channel
+        .send_rndis_control_message_no_completion(
+            rndisprot::MESSAGE_TYPE_PACKET_MSG,
+            rndisprot::Packet {
+                data_offset: 7777,
+                data_length: 0,
+                oob_data_offset: 0,
+                oob_data_length: 0,
+                num_oob_data_elements: 0,
+                per_packet_info_offset: 0,
+                per_packet_info_length: 0,
+                vc_handle: 0,
+                reserved: 0,
+            },
+            &[],
+        )
+        .await;
+
+    // Expect a completion message with failure status.
+    channel
+        .read_with(|packet| match packet {
+            IncomingPacket::Completion(completion) => {
+                let mut reader = completion.reader();
+                let header: protocol::MessageHeader = reader.read_plain().unwrap();
+                assert_eq!(
+                    header.message_type,
+                    protocol::MESSAGE1_TYPE_SEND_RNDIS_PACKET_COMPLETE
+                );
+                let completion_data: protocol::Message1SendRndisPacketComplete =
+                    reader.read_plain().unwrap();
+                assert_eq!(completion_data.status, protocol::Status::FAILURE);
+            }
+            _ => panic!("Unexpected packet"),
+        })
+        .await
+        .expect("completion message");
+
+    // Verify the channel is still processing packets by sending another (also invalid).
+    channel
+        .send_rndis_control_message_no_completion(
+            rndisprot::MESSAGE_TYPE_PACKET_MSG,
+            rndisprot::Packet {
+                data_offset: 8888,
+                data_length: 0,
+                oob_data_offset: 0,
+                oob_data_length: 0,
+                num_oob_data_elements: 0,
+                per_packet_info_offset: 0,
+                per_packet_info_length: 0,
+                vc_handle: 0,
+                reserved: 0,
+            },
+            &[],
+        )
+        .await;
+
+    channel
+        .read_with(|packet| match packet {
+            IncomingPacket::Completion(completion) => {
+                let mut reader = completion.reader();
+                let header: protocol::MessageHeader = reader.read_plain().unwrap();
+                assert_eq!(
+                    header.message_type,
+                    protocol::MESSAGE1_TYPE_SEND_RNDIS_PACKET_COMPLETE
+                );
+                let completion_data: protocol::Message1SendRndisPacketComplete =
+                    reader.read_plain().unwrap();
+                assert_eq!(completion_data.status, protocol::Status::FAILURE);
+            }
+            _ => panic!("Unexpected packet"),
+        })
+        .await
+        .expect("completion message");
+}
+
 #[async_test]
 async fn stop_start_with_vf(driver: DefaultDriver) {
     let endpoint_state = TestNicEndpointState::new();