[ovs-dev] [PATCH 3/4] datapath-windows: STT reassemble small fix

[Paul Boca]

Fixed possible deadlock in case NdisGetDataBuffer fails
Validate the segment length and offset on reassemble to avoid buffer overflow

Signed-off-by: Paul-Daniel Boca <pboca at cloudbasesolutions.com>
---
 datapath-windows/ovsext/Stt.c | 16 +++++++++++++---
 datapath-windows/ovsext/Stt.h |  1 +
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/datapath-windows/ovsext/Stt.c b/datapath-windows/ovsext/Stt.c
index 5b5d950..8a1b1a9 100644
--- a/datapath-windows/ovsext/Stt.c
+++ b/datapath-windows/ovsext/Stt.c
@@ -607,9 +607,6 @@ OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
     SttHdr *sttHdr = NULL;
     sourceNb = NET_BUFFER_LIST_FIRST_NB(curNbl);
 
-    /* XXX optimize this lock */
-    NdisAcquireSpinLock(&OvsSttSpinLock);
-
     /* If this is the first fragment, copy the STT header */
     if (segOffset == 0) {
         sttHdr = NdisGetDataBuffer(sourceNb, sizeof(SttHdr), &stt, 1, 0);
@@ -621,6 +618,14 @@ OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
         startOffset = startOffset + STT_HDR_LEN;
     }
 
+    if (offset + fragmentLength > innerPacketLen) {
+        // avoid buffer overflow on copy
+        return NULL;
+    }
+
+    /* XXX optimize this lock */
+    NdisAcquireSpinLock(&OvsSttSpinLock);
+
     /* Lookup fragment */
     OVS_STT_PKT_KEY pktKey = OvsGeneratePacketKey(ipHdr, tcp);
     UINT32 hash = OvsSttGetPktHash(&pktKey);
@@ -649,6 +654,7 @@ OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
         }
 
         /* Copy the data from Source to new buffer */
+        entry->allocatedLen = innerPacketLen;
         entry->packetBuf = OvsAllocateMemoryWithTag(innerPacketLen,
                                                     OVS_STT_POOL_TAG);
         if (OvsGetPacketBytes(curNbl, fragmentLength, startOffset,
@@ -661,6 +667,10 @@ OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
         InsertHeadList(&OvsSttPktFragHash[hash & STT_HASH_TABLE_MASK],
                        &entry->link);
     } else {
+        if (offset + fragmentLength > pktFragEntry->allocatedLen) {
+            // don't copy more than it is allocated
+            goto handle_error;
+        }
         /* Add to recieved length to identify if this is the last fragment */
         pktFragEntry->recvdLen += fragmentLength;
         lastPacket = (pktFragEntry->recvdLen == innerPacketLen);
diff --git a/datapath-windows/ovsext/Stt.h b/datapath-windows/ovsext/Stt.h
index 20066e6..8aea164 100644
--- a/datapath-windows/ovsext/Stt.h
+++ b/datapath-windows/ovsext/Stt.h
@@ -67,6 +67,7 @@ typedef struct _OVS_STT_PKT_ENTRY {
     OVS_STT_PKT_KEY     ovsPktKey;
     UINT64              timeout;
     UINT32              recvdLen;
+    UINT32              allocatedLen;
     SttHdr              sttHdr;
     PCHAR               packetBuf;
     LIST_ENTRY          link;
-- 
2.7.2.windows.1