[ovs-dev] [PATCH V2 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>
Acked-by: Sairam Venugopal <vsairam at vmware.com>
---
V2: No changes
---
 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 9345255..fd9b32b 100644
--- a/datapath-windows/ovsext/Stt.c
+++ b/datapath-windows/ovsext/Stt.c
@@ -612,9 +612,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);
@@ -626,6 +623,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);
@@ -652,6 +657,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,
@@ -664,6 +670,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 a3e3915..faa00d7 100644
--- a/datapath-windows/ovsext/Stt.h
+++ b/datapath-windows/ovsext/Stt.h
@@ -68,6 +68,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