fix two bugs mishandling fragmented frames

microsoft · May 7, 2024 · e3cede5 · e3cede5
1 parent 286b5a7
commit e3cede5
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 14 deletions.
diff --git a/src/xdp/programinspect.c b/src/xdp/programinspect.c
@@ -284,12 +284,12 @@ XdpParseFragmentedFrame(
     _In_ XDP_EXTENSION *FragmentExtension,
     _In_ UINT32 FragmentIndex,
     _In_ XDP_EXTENSION *VirtualAddressExtension,
+    _In_ UINT32 BufferDataOffset,
     _Inout_ XDP_PROGRAM_FRAME_CACHE *Cache,
     _Inout_ XDP_PROGRAM_FRAME_STORAGE *Storage
     )
 {
     XDP_BUFFER *Buffer = &Frame->Buffer;
-    UINT32 BufferDataOffset = 0;
     UINT32 FragmentCount;
     IPPROTO IpProto = IPPROTO_MAX;
 
@@ -479,7 +479,7 @@ XdpParseFrame(
         ASSERT(FragmentExtension);
         XdpParseFragmentedFrame(
             Frame, FragmentRing, FragmentExtension, FragmentIndex, VirtualAddressExtension,
-            Cache, Storage);
+            Offset, Cache, Storage);
     }
 }
 

diff --git a/src/xdplwf/recv.c b/src/xdplwf/recv.c
@@ -10,6 +10,15 @@
 #define RECV_TX_INSPECT_BATCH_SIZE 64
 #define RECV_DEFAULT_MAX_TX_BUFFERS 256
 #define RECV_MAX_MAX_TX_BUFFERS 4096
+//
+// Rather than tracking the current lookaside via OIDs, which is subject to
+// theoretical race conditions, simply set the minimum lookahead for forwarding
+// TX inspected packets onto the RX path based on the known minimums for TCPIP
+// and TCPIP6. Older OS builds also erroneously configure excessively large
+// lookasides. The value here is the maximum IPv4 header size, which is larger
+// than the IPv6 header, plus the L2 header.
+//
+#define RECV_TX_INSPECT_LOOKAHEAD (sizeof(ETHERNET_HEADER) + (0xF * sizeof(UINT32)))
 
 typedef struct _XDP_LWF_GENERIC_RX_FRAME_CONTEXT {
     NET_BUFFER *Nb;
@@ -559,7 +568,16 @@ XdpGenericReceiveEnqueueTxNb(
 
     ASSERT(TxNbl->FirstNetBuffer->Next == NULL);
 
-    if (CanPend) {
+    //
+    // If we are allowed to clone and pend NBLs, reuse the existing MDL chain.
+    //
+    // We cannot reuse the MDL chain if the NBL is being forwarded onto the
+    // local RX path and the frame is potentially discontiguous within L2 or L3
+    // headers.
+    //
+    if (CanPend &&
+            (!RxQueue->Flags.TxInspect ||
+                Nb->CurrentMdl->ByteCount - Nb->CurrentMdlOffset >= RECV_TX_INSPECT_LOOKAHEAD)) {
         TxNbl->FirstNetBuffer->MdlChain = Nb->MdlChain;
         TxNbl->FirstNetBuffer->CurrentMdl = Nb->CurrentMdl;
         TxNbl->FirstNetBuffer->DataLength = Nb->DataLength;

diff --git a/test/functional/lib/tests.cpp b/test/functional/lib/tests.cpp
@@ -3869,6 +3869,7 @@ GenericRxFragmentBuffer(
     UINT32 PacketBufferOffset = 0;
     UINT32 TotalOffset = 0;
     MY_SOCKET Xsk;
+    wil::unique_handle ProgramHandle;
     unique_fnmp_handle GenericMp;
     unique_fnlwf_handle FnLwf;
     const XDP_HOOK_ID *RxHookId = Params->IsTxInspect ? &XdpInspectTxL2 : &XdpInspectRxL2;
@@ -3899,7 +3900,7 @@ GenericRxFragmentBuffer(
         Rule.Redirect.Target = Xsk.Handle.get();
     }
 
-    wil::unique_handle ProgramHandle =
+    ProgramHandle =
         CreateXdpProg(If.GetIfIndex(), RxHookId, If.GetQueueId(), XDP_GENERIC, &Rule, 1);
 
     //
@@ -4039,7 +4040,12 @@ GenericRxFragmentBuffer(
         // infer this is true by the TX NBL having the same MDL layout as the
         // original NBL.
         //
-        if (!Params->LowResources) {
+        // For TX-inspect NBLs, allow XDP to copy data if the first buffer is
+        // too short to reliably contain all L2 and L3 headers due to NDIS
+        // lookahead requirements.
+        //
+        if (!Params->LowResources &&
+            (!Params->IsTxInspect || Frame.Frame.Buffers[0].DataLength > 128)) {
             TEST_EQUAL(Buffers.size(), TxFrame->BufferCount);
         }
 
@@ -4098,7 +4104,7 @@ GenericRxTooManyFragments(
 }
 
 VOID
-GenericRxHeaderFragments(
+GenericRxHeaderMultipleFragments(
     _In_ ADDRESS_FAMILY Af,
     _In_ XDP_RULE_ACTION ProgramAction,
     _In_ BOOLEAN IsUdp,
@@ -4119,15 +4125,40 @@ GenericRxHeaderFragments(
     SplitIndexes[3] = SplitIndexes[2] + ((Af == AF_INET) ? sizeof(IPV4_HEADER) : sizeof(IPV6_HEADER));
     SplitIndexes[4] = SplitIndexes[3] + (IsUdp ? sizeof(UDP_HDR) : sizeof(TCP_HDR)) / 2;
 
-    for (auto Split : {false, true}) {
-        if (Split) {
-            Params.SplitIndexes = SplitIndexes;
-            Params.SplitCount = RTL_NUMBER_OF(SplitIndexes);
-        } else {
-            Params.SplitIndexes = NULL;
-            Params.SplitCount = 0;
-        }
+    for (UINT16 i = 0; i < RTL_NUMBER_OF(SplitIndexes) - 1; i++) {
+        Params.SplitIndexes = &SplitIndexes[i];
+        Params.SplitCount = RTL_NUMBER_OF(SplitIndexes) - i;
+        Params.IsTxInspect = IsTxInspect;
+        Params.LowResources = IsLowResources;
+        GenericRxFragmentBuffer(Af, &Params);
+    }
+}
+
+VOID
+GenericRxHeaderFragments(
+    _In_ ADDRESS_FAMILY Af,
+    _In_ XDP_RULE_ACTION ProgramAction,
+    _In_ BOOLEAN IsUdp,
+    _In_ BOOLEAN IsTxInspect,
+    _In_ BOOLEAN IsLowResources
+    )
+{
+    GENERIC_RX_FRAGMENT_PARAMS Params = {0};
+    Params.Action = ProgramAction;
+    Params.IsUdp = IsUdp;
+    Params.PayloadLength = 43;
+    Params.Backfill = 13;
+    Params.Trailer = 17;
+    UINT16 HeadersLength =
+        sizeof(ETHERNET_HEADER) +
+            ((Af == AF_INET) ? sizeof(IPV4_HEADER) : sizeof(IPV6_HEADER)) +
+            (IsUdp ? sizeof(UDP_HDR) : sizeof(TCP_HDR));
+
+    GenericRxHeaderMultipleFragments(Af, ProgramAction, IsUdp, IsTxInspect, IsLowResources);
 
+    for (UINT16 i = 1; i < HeadersLength; i++) {
+        Params.SplitIndexes = &i;
+        Params.SplitCount = 1;
         Params.IsTxInspect = IsTxInspect;
         Params.LowResources = IsLowResources;
         GenericRxFragmentBuffer(Af, &Params);