[ovs-dev] [PATCH V2] datapath-windows: Handle possible NULL pointer dereference in STT
Sairam Venugopal
vsairam at vmware.com
Tue Jun 21 18:18:08 UTC 2016
Hi Paul,
Please find the comments inlined.
Thanks,
Sairam
On 6/21/16, 9:51 AM, "Paul Boca" <pboca at cloudbasesolutions.com> wrote:
>Check if OvsAllocatememoryWithTag succeeded or not.
>In case of failure propagate cleanup and return.
>---
>V2: Checked also NdisGetDataBuffer and MmGetSystemAddressForMdlSafe
> if they return NULL and handle the error
>---
> datapath-windows/ovsext/Stt.c | 55
>++++++++++++++++++++++++++++++++++---------
> 1 file changed, 44 insertions(+), 11 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Stt.c b/datapath-windows/ovsext/Stt.c
>index 86a719d..498d340 100644
>--- a/datapath-windows/ovsext/Stt.c
>+++ b/datapath-windows/ovsext/Stt.c
>@@ -191,6 +191,10 @@ OvsDoEncapStt(POVS_VPORT_ENTRY vport,
>
> bufferStart = (PUINT8)MmGetSystemAddressForMdlSafe(curMdl,
> LowPagePriority);
>+ if (bufferStart == NULL) {
>+ status = NDIS_STATUS_RESOURCES;
>+ goto ret_error;
>+ }
> bufferStart += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>
> if (layers->isIPv4) {
>@@ -402,6 +406,9 @@ OvsValidateTCPChecksum(PNET_BUFFER_LIST curNbl,
>PNET_BUFFER curNb)
>
> EthHdr *eth = (EthHdr *)NdisGetDataBuffer(curNb, sizeof(EthHdr),
> NULL, 1, 0);
>+ if (eth == NULL) {
>+ return NDIS_STATUS_RESOURCES;
>+ }
>
> if (eth->Type == ntohs(NDIS_ETH_TYPE_IPV4)) {
> IPHdr *ip = (IPHdr *)((PCHAR)eth + sizeof *eth);
>@@ -641,7 +648,7 @@ OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
> POVS_STT_PKT_ENTRY entry;
> entry = OvsAllocateMemoryWithTag(sizeof(OVS_STT_PKT_ENTRY),
> OVS_STT_POOL_TAG);
>- if (NULL == entry) {
>+ if (entry == NULL) {
> goto handle_error;
> }
> RtlZeroMemory(entry, sizeof (OVS_STT_PKT_ENTRY));
>@@ -666,13 +673,15 @@ OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
> entry->allocatedLen = innerPacketLen;
> entry->packetBuf = OvsAllocateMemoryWithTag(innerPacketLen,
> OVS_STT_POOL_TAG);
>- if (NULL == entry->packetBuf) {
>+ if (entry->packetBuf == NULL) {
> OvsFreeMemoryWithTag(entry, OVS_STT_POOL_TAG);
> goto handle_error;
> }
> if (OvsGetPacketBytes(curNbl, fragmentLength, startOffset,
> entry->packetBuf + offset) == NULL) {
> OVS_LOG_ERROR("Error when obtaining bytes from Packet");
Sai - Do we really need to discard other packets and entry if 1 packet
fails to contain data?
I think we should wait for retransmission to send the right packet. Also,
the cleanup thread will flush this entry on expiry.
>+ OvsFreeMemoryWithTag(entry->packetBuf, OVS_STT_POOL_TAG);
>+ OvsFreeMemoryWithTag(entry, OVS_STT_POOL_TAG);
> goto handle_error;
> }
>
>@@ -684,9 +693,6 @@ OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
> // 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);
>
> if (segOffset == 0) {
> pktFragEntry->sttHdr = *sttHdr;
>@@ -699,8 +705,16 @@ OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
> if (OvsGetPacketBytes(curNbl, fragmentLength, startOffset,
> pktFragEntry->packetBuf + offset) == NULL)
>{
> OVS_LOG_ERROR("Error when obtaining bytes from Packet");
Sai - Same as above.
>+ /* Remove from list and free the allocate buffers */
>+ RemoveEntryList(&pktFragEntry->link);
>+ OvsFreeMemoryWithTag(pktFragEntry->packetBuf,
>OVS_STT_POOL_TAG);
>+ OvsFreeMemoryWithTag(pktFragEntry, OVS_STT_POOL_TAG);
> goto handle_error;
> }
>+
>+ /* Add to received length to identify if this is the last
>fragment */
>+ pktFragEntry->recvdLen += fragmentLength;
>+ lastPacket = (pktFragEntry->recvdLen == innerPacketLen);
> }
>
> handle_error:
>@@ -744,7 +758,7 @@ handle_error:
>
>*-------------------------------------------------------------------------
>---
> */
> NDIS_STATUS
>-OvsDecapSetOffloads(PNET_BUFFER_LIST *curNbl,
>+OvsDecapSetOffloads(PNET_BUFFER_LIST *curNbl,
> SttHdr *sttHdr,
> OVS_PACKET_HDR_INFO *layers)
> {
>@@ -804,6 +818,9 @@ OvsDecapSetOffloads(PNET_BUFFER_LIST *curNbl,
>
> buf = (PUINT8)MmGetSystemAddressForMdlSafe(curMdl,
> LowPagePriority);
>+ if (buf == NULL) {
>+ return NDIS_STATUS_RESOURCES;
>+ }
> buf += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>
> // apply pseudo checksum on extracted packet
>@@ -883,7 +900,9 @@ OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
>
> ipHdr = NdisGetDataBuffer(curNb, sizeof *ipHdr, (PVOID) &ipBuf,
> 1 /*no align*/, 0);
>- ASSERT(ipHdr);
>+ if (ipHdr == NULL) {
>+ return NDIS_STATUS_RESOURCES;
>+ }
>
> TCPHdr *tcp = (TCPHdr *)((PCHAR)ipHdr + ipHdr->ihl * 4);
>
>@@ -913,6 +932,9 @@ OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
> /* STT Header */
> sttHdr = NdisGetDataBuffer(curNb, sizeof *sttHdr,
> (PVOID) &sttBuf, 1 /*no align*/, 0);
>+ if (sttHdr == NULL) {
>+ return NDIS_STATUS_RESOURCES;
>+ }
> /* Skip stt header, DataOffset points to inner pkt now. */
> hdrLen = STT_HDR_LEN;
> NdisAdvanceNetBufferDataStart(curNb, hdrLen, FALSE, NULL);
>@@ -929,8 +951,8 @@ OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
>
> status = NdisRetreatNetBufferDataStart(curNb, advanceCnt, 0, NULL);
> if (status != NDIS_STATUS_SUCCESS) {
>- OvsCompleteNBL(switchContext, *newNbl, TRUE);
>- return NDIS_STATUS_FAILURE;
>+ status = NDIS_STATUS_FAILURE;
>+ goto dropNbl;
> }
>
> ASSERT(sttHdr);
>@@ -949,8 +971,8 @@ OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
> if (0 != ipHdr->tos) {
> status = OvsExtractLayers(*newNbl, &layers);
> if (status != NDIS_STATUS_SUCCESS) {
>- OvsCompleteNBL(switchContext, *newNbl, TRUE);
>- return NDIS_STATUS_FAILURE;
>+ status = NDIS_STATUS_FAILURE;
>+ goto dropNbl;
> }
>
> if (layers.isIPv4) {
>@@ -973,6 +995,9 @@ OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
> /* fix IP checksum */
> innerIpHdr->check = IPChecksum((UINT8 *)innerIpHdr,
> innerIpHdr->ihl * 4, 0);
>+ } else {
>+ status = NDIS_STATUS_RESOURCES;
>+ goto dropNbl;
> }
> } else if (layers.isIPv6) {
> IPv6Hdr ipv6_storage;
>@@ -987,6 +1012,9 @@ OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
> | ((innerIpv6Hdr->flow_lbl[0] & 0x3)
><< 2);
> innerIpv6Hdr->flow_lbl[0] = (innerIpv6Hdr->flow_lbl[0] &
>0xF)
> | ((ipHdr->tos & 0xF) << 4);
>+ } else {
>+ status = NDIS_STATUS_RESOURCES;
>+ goto dropNbl;
> }
> }
> }
>@@ -1005,4 +1033,9 @@ OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
> OvsDecapSetOffloads(newNbl, sttHdr, &layers);
>
> return NDIS_STATUS_SUCCESS;
>+
>+dropNbl:
>+ OvsCompleteNBL(switchContext, *newNbl, TRUE);
>+ *newNbl = NULL;
>+ return status;
> }
>--
>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=1T9pUHBjmGvjdk15zPTb8zHKHwumzl
>fboOR4ioeT2WM&s=gur2UlIru4yzAJQsMwTAr1ZCyKz16XG66EIeRpCH5VM&e=
More information about the dev
mailing list