diff mbox

[ovs-dev,V5] datapath-windows: Handle possible NULL pointer dereference in STT

Message ID 508339EC0242094682895ED3EC4EBA312E5452F4@CBSEX1.cloudbase.local
State Superseded
Headers show

Commit Message

Paul Boca June 27, 2016, 8:43 p.m. UTC
Hi!

I will rebase it in a second.

Thanks,
Paul

From: Guru Shetty [mailto:guru@ovn.org]

Sent: Monday, June 27, 2016 11:30 PM
To: Paul Boca
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH V5] datapath-windows: Handle possible NULL pointer dereference in STT



On 23 June 2016 at 02:16, Paul Boca <pboca@cloudbasesolutions.com<mailto:pboca@cloudbasesolutions.com>> wrote:
Check if OvsAllocatememoryWithTag succeeded or not.
In case of failure propagate cleanup and return.

Signed-off-by: Paul-Daniel Boca <pboca@cloudbasesolutions.com<mailto:pboca@cloudbasesolutions.com>>

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


v5 does not apply on master. Can you please rebase.

---
V2: Checked also NdisGetDataBuffer and MmGetSystemAddressForMdlSafe
      if they return NULL and handle the error
V3: Don't deallocate and remove failed packets on STT reassembly.
      They will be retransmited by sender or removed by the cleanup thread.
V4: Removed comment that doesn't apply anymore.
V5: Added Acked-by. This was already acked by Sairam
---
 datapath-windows/ovsext/Stt.c | 52 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 9 deletions(-)

--
2.7.2.windows.1
_______________________________________________
dev mailing list
dev@openvswitch.org<mailto:dev@openvswitch.org>
http://openvswitch.org/mailman/listinfo/dev
diff mbox

Patch

diff --git a/datapath-windows/ovsext/Stt.c b/datapath-windows/ovsext/Stt.c
index 0bac5f2..93bc503 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,6 +648,9 @@  OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
         POVS_STT_PKT_ENTRY entry;
         entry = OvsAllocateMemoryWithTag(sizeof(OVS_STT_PKT_ENTRY),
                                          OVS_STT_POOL_TAG);
+        if (entry == NULL) {
+            goto handle_error;
+        }
         RtlZeroMemory(entry, sizeof (OVS_STT_PKT_ENTRY));

         /* Update Key, timestamp and recvdLen */
@@ -663,6 +673,10 @@  OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
         entry->allocatedLen = innerPacketLen;
         entry->packetBuf = OvsAllocateMemoryWithTag(innerPacketLen,
                                                     OVS_STT_POOL_TAG);
+        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");
@@ -677,9 +691,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;
@@ -694,6 +705,10 @@  OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
             OVS_LOG_ERROR("Error when obtaining bytes from Packet");
             goto handle_error;
         }
+
+        /* Add to received length to identify if this is the last fragment */
+        pktFragEntry->recvdLen += fragmentLength;
+        lastPacket = (pktFragEntry->recvdLen == innerPacketLen);
     }

 handle_error:
@@ -737,7 +752,7 @@  handle_error:
 *----------------------------------------------------------------------------
 */
 NDIS_STATUS
-OvsDecapSetOffloads(PNET_BUFFER_LIST *curNbl,
+OvsDecapSetOffloads(PNET_BUFFER_LIST *curNbl,
                     SttHdr *sttHdr,
                     OVS_PACKET_HDR_INFO *layers)
 {
@@ -797,6 +812,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
@@ -876,7 +894,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);

@@ -906,6 +926,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);
@@ -922,8 +945,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);
@@ -942,8 +965,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) {
@@ -966,6 +989,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;
@@ -980,6 +1006,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;
             }
         }
     }
@@ -998,4 +1027,9 @@  OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
     OvsDecapSetOffloads(newNbl, sttHdr, &layers);

     return NDIS_STATUS_SUCCESS;
+
+dropNbl:
+    OvsCompleteNBL(switchContext, *newNbl, TRUE);
+    *newNbl = NULL;
+    return status;
 }