diff mbox

[ovs-dev] datapath-windows: Add validations in fragmentation module

Message ID 20170525232121.5344-1-kumaranand@vmware.com
State Superseded
Headers show

Commit Message

Anand Kumar May 25, 2017, 11:21 p.m. UTC
- Minimum valid fragment size is 400 bytes, any fragment smaller
is likely to be intentionally crafted.

- Validate maximum length of an Ip datagram

- Added counters to keep track of number of fragments for a given
Ip datagram.

Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
 datapath-windows/ovsext/Actions.c    |  2 +-
 datapath-windows/ovsext/IpFragment.c | 41 +++++++++++++++++++++++++-----------
 datapath-windows/ovsext/IpFragment.h |  2 ++
 3 files changed, 32 insertions(+), 13 deletions(-)

Comments

Joe Stringer May 26, 2017, 6:19 p.m. UTC | #1
On 25 May 2017 at 16:21, Anand Kumar <kumaranand@vmware.com> wrote:
> - Minimum valid fragment size is 400 bytes, any fragment smaller
> is likely to be intentionally crafted.

This seems arbitrary and surprising. Is there some reference to argue
why this is a good idea?

Even if this makes sense I'd expect it to be configurable rather than
hard-coded.
Alin Serdean June 9, 2017, 3:22 p.m. UTC | #2
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of Anand Kumar
> Sent: Friday, May 26, 2017 2:21 AM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH] datapath-windows: Add validations in
> fragmentation module
> 
> - Minimum valid fragment size is 400 bytes, any fragment smaller is likely to
> be intentionally crafted.
> 
> - Validate maximum length of an Ip datagram
> 
> - Added counters to keep track of number of fragments for a given Ip
> datagram.
> 
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
Ben Pfaff June 9, 2017, 4:39 p.m. UTC | #3
On Fri, May 26, 2017 at 11:19:46AM -0700, Joe Stringer wrote:
> On 25 May 2017 at 16:21, Anand Kumar <kumaranand@vmware.com> wrote:
> > - Minimum valid fragment size is 400 bytes, any fragment smaller
> > is likely to be intentionally crafted.
> 
> This seems arbitrary and surprising. Is there some reference to argue
> why this is a good idea?
> 
> Even if this makes sense I'd expect it to be configurable rather than
> hard-coded.

Just to pile on: I'd think that, given that an IP packet is being
fragmented at all, the last fragment might be arbitrarily small.
Joe Stringer June 9, 2017, 6:11 p.m. UTC | #4
On 9 June 2017 at 09:39, Ben Pfaff <blp@ovn.org> wrote:
> On Fri, May 26, 2017 at 11:19:46AM -0700, Joe Stringer wrote:
>> On 25 May 2017 at 16:21, Anand Kumar <kumaranand@vmware.com> wrote:
>> > - Minimum valid fragment size is 400 bytes, any fragment smaller
>> > is likely to be intentionally crafted.
>>
>> This seems arbitrary and surprising. Is there some reference to argue
>> why this is a good idea?
>>
>> Even if this makes sense I'd expect it to be configurable rather than
>> hard-coded.
>
> Just to pile on: I'd think that, given that an IP packet is being
> fragmented at all, the last fragment might be arbitrarily small.

I think that this case is what the "if (flags..." part is trying to cover.
diff mbox

Patch

diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c
index c3f0362..4eeaab3 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -2030,7 +2030,7 @@  OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext,
             if (status != NDIS_STATUS_SUCCESS) {
                 /* Pending NBLs are consumed by Defragmentation. */
                 if (status != NDIS_STATUS_PENDING) {
-                    OVS_LOG_ERROR("CT Action failed");
+                    OVS_LOG_ERROR("CT Action failed status = %lu", status);
                     dropReason = L"OVS-conntrack action failed";
                 } else {
                     /* We added a new pending NBL to be consumed later.
diff --git a/datapath-windows/ovsext/IpFragment.c b/datapath-windows/ovsext/IpFragment.c
index 0874cb9..e601a15 100644
--- a/datapath-windows/ovsext/IpFragment.c
+++ b/datapath-windows/ovsext/IpFragment.c
@@ -25,6 +25,10 @@ 
 #undef OVS_DBG_MOD
 #endif
 #define OVS_DBG_MOD OVS_DBG_IPFRAG
+/* Based on MIN_FRAGMENT_SIZE.*/
+#define MAX_FRAGMENTS 164
+#define MIN_FRAGMENT_SIZE 400
+#define MAX_IPDATAGRAM_SIZE 65535
 
 /* Function declarations */
 static VOID OvsIpFragmentEntryCleaner(PVOID data);
@@ -146,8 +150,9 @@  OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext,
     IPHdr *ipHdr, *newIpHdr;
     CHAR *ethBuf[sizeof(EthHdr)];
     CHAR *packetBuf;
-    UINT16 ipHdrLen, packetLen, packetHeader;
+    UINT16 ipHdrLen, packetHeader;
     POVS_FRAGMENT_LIST head = NULL;
+    UINT32 packetLen;
 
     curNb = NET_BUFFER_LIST_FIRST_NB(*curNbl);
     ASSERT(NET_BUFFER_NEXT_NB(curNb) == NULL);
@@ -161,7 +166,10 @@  OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext,
     if (ipHdr == NULL) {
         return NDIS_STATUS_INVALID_PACKET;
     }
-    ipHdrLen = (UINT16)(ipHdr->ihl * 4);
+    ipHdrLen = ipHdr->ihl * 4;
+    if (ipHdrLen + entry->totalLen > MAX_IPDATAGRAM_SIZE) {
+        return NDIS_STATUS_INVALID_LENGTH;
+    }
     packetLen = ETH_HEADER_LENGTH + ipHdrLen + entry->totalLen;
     packetBuf = (CHAR*)OvsAllocateMemoryWithTag(packetLen,
                                                 OVS_IPFRAG_POOL_TAG);
@@ -185,7 +193,10 @@  OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext,
     packetHeader = ETH_HEADER_LENGTH + ipHdrLen;
     head = entry->head;
     while (head) {
-        ASSERT((packetHeader + head->offset) <= packetLen);
+        if ((UINT32)(packetHeader + head->offset) > packetLen) {
+            status = NDIS_STATUS_INVALID_DATA;
+            goto cleanup;
+        }
         NdisMoveMemory(packetBuf + packetHeader + head->offset,
                        head->pbuff, head->len);
         head = head->next;
@@ -197,10 +208,6 @@  OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext,
         status = NDIS_STATUS_RESOURCES;
     }
 
-    OvsFreeMemoryWithTag(packetBuf, OVS_IPFRAG_POOL_TAG);
-    /* Timeout the entry so that clean up thread deletes it .*/
-    entry->expiration -= IPFRAG_ENTRY_TIMEOUT;
-
     /* Complete the fragment NBL */
     ctx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(*curNbl);
     if (ctx->flags & OVS_BUFFER_NEED_COMPLETE) {
@@ -214,6 +221,9 @@  OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext,
     ctx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(*newNbl);
     ctx->mru = entry->mru;
     *curNbl = *newNbl;
+cleanup:
+    OvsFreeMemoryWithTag(packetBuf, OVS_IPFRAG_POOL_TAG);
+    entry->markedForDelete = TRUE;
     return status;
 }
 /*
@@ -259,12 +269,15 @@  OvsProcessIpv4Fragment(POVS_SWITCH_CONTEXT switchContext,
     if (ipHdr == NULL) {
         return NDIS_STATUS_INVALID_PACKET;
     }
-    ipHdrLen = (UINT16)(ipHdr->ihl * 4);
+    ipHdrLen = ipHdr->ihl * 4;
     payloadLen = ntohs(ipHdr->tot_len) - ipHdrLen;
     offset = ntohs(ipHdr->frag_off) & IP_OFFSET;
     offset <<= 3;
     flags = ntohs(ipHdr->frag_off) & IP_MF;
-
+    /* Only the last fragment can be of smaller size.*/
+    if (flags && ntohs(ipHdr->tot_len) < MIN_FRAGMENT_SIZE) {
+        return NDIS_STATUS_INVALID_LENGTH;
+    }
     /*Copy fragment specific fields. */
     fragKey.protocol = ipHdr->protocol;
     fragKey.id = ipHdr->id;
@@ -318,6 +331,7 @@  OvsProcessIpv4Fragment(POVS_SWITCH_CONTEXT switchContext,
         entry->mru = ETH_HEADER_LENGTH + ipHdrLen + payloadLen;
         entry->recvdLen += fragStorage->len;
         entry->head = entry->tail = fragStorage;
+        entry->numFragments = 1;
         if (!flags) {
             entry->totalLen = offset + payloadLen;
         }
@@ -337,8 +351,9 @@  OvsProcessIpv4Fragment(POVS_SWITCH_CONTEXT switchContext,
         /* Acquire the entry lock. */
         NdisAcquireSpinLock(&(entry->lockObj));
         NdisGetCurrentSystemTime((LARGE_INTEGER *)&currentTime);
-        if (currentTime > entry->expiration) {
-            /* Expired entry. */
+        if (currentTime > entry->expiration || entry->numFragments == MAX_FRAGMENTS) {
+            /* Mark the entry for delete. */
+            entry->markedForDelete = TRUE;
             goto fragment_error;
         }
         POVS_FRAGMENT_LIST next = entry->head;
@@ -393,6 +408,7 @@  found:
         /*Update Maximum recieved Unit */
         entry->mru = entry->mru > (ETH_HEADER_LENGTH + ipHdrLen + payloadLen) ?
             entry->mru : (ETH_HEADER_LENGTH + ipHdrLen + payloadLen);
+        entry->numFragments++;
         if (!flags) {
             entry->totalLen = offset + payloadLen;
         }
@@ -404,6 +420,7 @@  found:
         return status;
     }
 fragment_error:
+    status = NDIS_STATUS_INVALID_PACKET;
     /* Release the entry lock. */
     NdisReleaseSpinLock(&(entry->lockObj));
 payload_copy_error:
@@ -460,7 +477,7 @@  static VOID
 OvsIpFragmentEntryDelete(POVS_IPFRAG_ENTRY entry, BOOLEAN checkExpiry)
 {
     NdisAcquireSpinLock(&(entry->lockObj));
-    if (checkExpiry) {
+    if (!entry->markedForDelete && checkExpiry) {
         UINT64 currentTime;
         NdisGetCurrentSystemTime((LARGE_INTEGER *)&currentTime);
         if (entry->expiration > currentTime) {
diff --git a/datapath-windows/ovsext/IpFragment.h b/datapath-windows/ovsext/IpFragment.h
index e650399..cd5b960 100644
--- a/datapath-windows/ovsext/IpFragment.h
+++ b/datapath-windows/ovsext/IpFragment.h
@@ -37,6 +37,8 @@  typedef struct _OVS_IPFRAG_KEY {
 
 typedef struct _OVS_IPFRAG_ENTRY {
     NDIS_SPIN_LOCK lockObj;       /* To access the entry. */
+    BOOLEAN markedForDelete;
+    UINT8 numFragments;
     UINT16 totalLen;
     UINT16 recvdLen;
     UINT16 mru;