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

Sairam Venugopal vsairam at vmware.com
Thu Jun 2 01:15:58 UTC 2016


Thanks for fixing this. Should we log an error if we try to copy more than
allocated packet length?

Acked-by: Sairam Venugopal <vsairam at vmware.com>


On 5/17/16, 8:23 AM, "Paul Boca" <pboca at cloudbasesolutions.com> wrote:

>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
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma
>n_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Dc
>ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=ks6H7c0dJA42Wc_Cm_8KRvAlNMZCZ1
>q0OcbL35AInvU&s=rCUHnvtDWaBmIb-8bKkBv6GpeDHX48S_WwQUS2yXLuc&e= 




More information about the dev mailing list