[win-pv-devel] [PATCH] Fix multiple completion vulnerability in transmit code

[Paul Durrant <pdurrant@xxxxxxxxx>]

My previous patch 7c3365d5 "Make transmitter robust against a possible
completion race" did not fix the problem. There is still the possibility
that a NET_BUFFER_LIST containing multiple NET_BUFFERs could lead to
multiple completions if the underlying transmit completes quickly (or indeed
synchrnously). This is because a reference is taken before sending each
NET_BUFFER but, if that transmission completes immediately the reference is
dropped back to zero (leading to the NET_BUFFER_LIST being completed) before
the reference is taken for the next NET_BUFFER.

This patch therefore takes an extra reference before sending any NET_BUFFERs
and then drops it when there are no more NET_BUFFERs to send. This ensures
that the reference count on the NET_BUFFER_LIST can only fall to zero once
the whole thing has been processed.

Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
---
 src/xennet/transmitter.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/src/xennet/transmitter.c b/src/xennet/transmitter.c
index 91ed5f2..eaf5267 100644
--- a/src/xennet/transmitter.c
+++ b/src/xennet/transmitter.c
@@ -90,6 +90,7 @@ TransmitterTeardown(
 
 typedef struct _NET_BUFFER_LIST_RESERVED {
     LONG    Reference;
+    LONG    Status;
 } NET_BUFFER_LIST_RESERVED, *PNET_BUFFER_LIST_RESERVED;
 
 C_ASSERT(sizeof (NET_BUFFER_LIST_RESERVED) <= RTL_FIELD_SIZE(NET_BUFFER_LIST, 
MiniportReserved));
@@ -120,6 +121,39 @@ __TransmitterCompleteNetBufferList(
                                     NDIS_SEND_COMPLETE_FLAGS_DISPATCH_LEVEL);
 }
 
+__TransmitterGetNetBufferList(
+    IN  PXENNET_TRANSMITTER     Transmitter,
+    IN  PNET_BUFFER_LIST        NetBufferList
+    )
+{
+    PNET_BUFFER_LIST_RESERVED   ListReserved;
+
+    UNREFERENCED_PARAMETER(Transmitter);
+
+    ListReserved = 
(PNET_BUFFER_LIST_RESERVED)NET_BUFFER_LIST_MINIPORT_RESERVED(NetBufferList);
+
+    if (InterlockedIncrement(&ListReserved->Reference) == 1)
+        ListReserved->Status = NDIS_STATUS_PENDING;
+}
+
+__TransmitterPutNetBufferList(
+    IN  PXENNET_TRANSMITTER     Transmitter,
+    IN  PNET_BUFFER_LIST        NetBufferList
+    )
+{
+    PNET_BUFFER_LIST_RESERVED   ListReserved;
+
+    UNREFERENCED_PARAMETER(Transmitter);
+
+    ListReserved = 
(PNET_BUFFER_LIST_RESERVED)NET_BUFFER_LIST_MINIPORT_RESERVED(NetBufferList);
+
+    ASSERT(ListReserved->Reference != 0);
+    if (InterlockedDecrement(&ListReserved->Reference) == 0)
+        __TransmitterCompleteNetBufferList(Transmitter,
+                                           NetBufferList,
+                                           ListReserved->Status);
+}
+
 static VOID
 __TransmitterReturnPacket(
     IN  PXENNET_TRANSMITTER     Transmitter,
@@ -134,9 +168,8 @@ __TransmitterReturnPacket(
 
     ListReserved = 
(PNET_BUFFER_LIST_RESERVED)NET_BUFFER_LIST_MINIPORT_RESERVED(NetBufferList);
 
-    ASSERT(ListReserved->Reference != 0);
-    if (InterlockedDecrement(&ListReserved->Reference) == 0)
-        __TransmitterCompleteNetBufferList(Transmitter, NetBufferList, Status);
+    (VOID) InterlockedExchange(&ListReserved->Status, Status);
+    __TransmitterPutNetBufferList(Transmitter, NetBufferList);
 }
 
 static VOID
@@ -248,6 +281,8 @@ TransmitterSendNetBufferLists(
         ListReserved = 
(PNET_BUFFER_LIST_RESERVED)NET_BUFFER_LIST_MINIPORT_RESERVED(NetBufferList);
         RtlZeroMemory(ListReserved, sizeof (NET_BUFFER_LIST_RESERVED));
 
+        __TransmitterGetNetBufferList(Transmitter, NetBufferList);
+
         NetBuffer = NET_BUFFER_LIST_FIRST_NB(NetBufferList);
         while (NetBuffer != NULL) {
             PNET_BUFFER         NetBufferListNext = 
NET_BUFFER_NEXT_NB(NetBuffer);
@@ -255,7 +290,7 @@ TransmitterSendNetBufferLists(
             XENVIF_PACKET_HASH  Hash;
             NTSTATUS            status;
 
-            InterlockedIncrement(&ListReserved->Reference);
+            __TransmitterGetNetBufferList(Transmitter, NetBufferList);
 
             Hash.Algorithm = XENVIF_PACKET_HASH_ALGORITHM_NONE;
 
@@ -278,6 +313,8 @@ TransmitterSendNetBufferLists(
             NetBuffer = NetBufferListNext;
         }
 
+        __TransmitterPutNetBufferList(Transmitter, NetBufferList);
+
         NetBufferList = ListNext;
     }
 
-- 
2.1.1


_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel