diff mbox

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

Message ID 20170610025445.468-1-kumaranand@vmware.com
State Accepted
Headers show

Commit Message

Anand Kumar June 10, 2017, 2:54 a.m. UTC 
- Minimum valid fragment size is 400 bytes, any fragment smaller
is likely to be intentionally crafted (CVE-2000-0305).

- 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>
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
---
v1->v2: Update commmit message to add (CVE-2000-0305) vulnerability
---
 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

Darrell Ball June 11, 2017, 5:51 p.m. UTC | #1 
On 6/9/17, 7:54 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote:

    - Minimum valid fragment size is 400 bytes, any fragment smaller
    is likely to be intentionally crafted (CVE-2000-0305).
    
    - 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>

    Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>

    ---
    v1->v2: Update commmit message to add (CVE-2000-0305) vulnerability
    ---
     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(-)
    
    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

I don’t plan a full review; just one comment.
It is almost certainly an exploit attempt and can be treated as such in all cases, but this
default is something like 20 years old, so it super conservatively small; using a 400 minimum
for non-last fragments will do most of what is needed though.

    +#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;
    -- 
    2.9.3.windows.1
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=0LGiq-mkmw2jTPk_LrWeOvPcXDuO92oANfpzdxui6gM&s=Fj9i3liY5YmbLmFVY63-uMv2Vjwj11rXFfn8cXSU2Yw&e=
Sairam Venugopal June 15, 2017, 9:25 p.m. UTC | #2 
Thanks for adding in the CVE.

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





On 6/9/17, 7:54 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote:

>- Minimum valid fragment size is 400 bytes, any fragment smaller
>is likely to be intentionally crafted (CVE-2000-0305).
>
>- 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>
>Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
>---
>v1->v2: Update commmit message to add (CVE-2000-0305) vulnerability
>---
> 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(-)
>
>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;
>-- 
>2.9.3.windows.1
>
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=Fj48Qs5cvjKoU6eV7J_szN5n4VTBzbIRnT73MY8sNNk&s=if3mZ38TqK_TYmpMtaFl8qGdV9_IWeG-omGU3q0X-Ho&e=
Ben Pfaff July 10, 2017, 6:15 p.m. UTC | #3 
On Fri, Jun 09, 2017 at 07:54:45PM -0700, Anand Kumar wrote:
> - Minimum valid fragment size is 400 bytes, any fragment smaller
> is likely to be intentionally crafted (CVE-2000-0305).
> 
> - 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>
> Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
> ---
> v1->v2: Update commmit message to add (CVE-2000-0305) vulnerability

Applied to master, thanks!

(Let me know if this should be backported.)
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;