Thanks for fixing this. Should we log an error if we try to copy more than
allocated packet length?
Acked-by: Sairam Venugopal <vsai...@vmware.com>
On 5/17/16, 8:23 AM, "Paul Boca" <pb...@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 <pb...@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@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=
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
