Hello Sai! I don't think we should print an error; this doesn't happen during normal usage. Only happens if someone forges the STT segments and injects them into network in order to crash our driver.
Thanks, Paul > -----Original Message----- > From: Sairam Venugopal [mailto:vsai...@vmware.com] > Sent: Thursday, June 2, 2016 4:16 AM > To: Paul Boca; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH 3/4] datapath-windows: STT reassemble small fix > > 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
