Re: [Patch 3/3] NetworkPkg/TcpDxe: Check TCP payload for release version.

["Ye, Ting" <ting.ye@intel.com>]

Reviewed-by: Ye Ting <ting.ye@intel.com> 

-----Original Message-----
From: Wu, Jiaxin 
Sent: Tuesday, December 26, 2017 2:50 PM
To: edk2-devel@lists.01.org
Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wang, Fan <fan.wang@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: [Patch 3/3] NetworkPkg/TcpDxe: Check TCP payload for release version.

TCP payload check is implemented by TcpVerifySegment(), but all the function calls of TcpVerifySegment() are placed in ASSERT(), which is only valid for debug version:
  ASSERT (TcpVerifySegment (Nbuf) != 0);

This patch is to enable the check for release version.

Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Wang Fan <fan.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 NetworkPkg/TcpDxe/TcpInput.c  | 125 +++++++++++++++++++++++++++++++++---------
 NetworkPkg/TcpDxe/TcpOutput.c |  29 +++++++---
 2 files changed, 122 insertions(+), 32 deletions(-)

diff --git a/NetworkPkg/TcpDxe/TcpInput.c b/NetworkPkg/TcpDxe/TcpInput.c index f8845dc..92a0ab8 100644
--- a/NetworkPkg/TcpDxe/TcpInput.c
+++ b/NetworkPkg/TcpDxe/TcpInput.c
@@ -279,12 +279,15 @@ TcpComputeRtt (
 
   @param[in]  Nbuf     The buffer that contains a received TCP segment without an IP header.
   @param[in]  Left     The sequence number of the window's left edge.
   @param[in]  Right    The sequence number of the window's right edge.
 
+  @retval     0        The segment is broken.
+  @retval     1        The segment is in good shape.
+
 **/
-VOID
+INTN
 TcpTrimSegment (
   IN NET_BUF   *Nbuf,
   IN TCP_SEQNO Left,
   IN TCP_SEQNO Right
   )
@@ -304,11 +307,11 @@ TcpTrimSegment (
     TCP_CLEAR_FLG (Seg->Flag, TCP_FLG_SYN);
     TCP_CLEAR_FLG (Seg->Flag, TCP_FLG_FIN);
 
     Seg->Seq = Seg->End;
     NetbufTrim (Nbuf, Nbuf->TotalSize, NET_BUF_HEAD);
-    return;
+    return 1;
   }
 
   //
   // Adjust the buffer header
   //
@@ -357,27 +360,30 @@ TcpTrimSegment (
     if (Drop != 0) {
       NetbufTrim (Nbuf, Drop, NET_BUF_TAIL);
     }
   }
 
-  ASSERT (TcpVerifySegment (Nbuf) != 0);
+  return TcpVerifySegment (Nbuf);
 }
 
 /**
   Trim off the data outside the tcb's receive window.
 
   @param[in]  Tcb      Pointer to the TCP_CB of this TCP instance.
   @param[in]  Nbuf     Pointer to the NET_BUF containing the received tcp segment.
 
+  @retval     0        The segment is broken.
+  @retval     1        The segment is in good shape.
+
 **/
-VOID
+INTN
 TcpTrimInWnd (
   IN TCP_CB  *Tcb,
   IN NET_BUF *Nbuf
   )
 {
-  TcpTrimSegment (Nbuf, Tcb->RcvNxt, Tcb->RcvWl2 + Tcb->RcvWnd);
+  return TcpTrimSegment (Nbuf, Tcb->RcvNxt, Tcb->RcvWl2 + Tcb->RcvWnd);
 }
 
 /**
   Process the data and FIN flag, and check whether to deliver
   data to the socket layer.
@@ -419,11 +425,20 @@ TcpDeliverData (
 
   while (Entry != &Tcb->RcvQue) {
     Nbuf  = NET_LIST_USER_STRUCT (Entry, NET_BUF, List);
     Seg   = TCPSEG_NETBUF (Nbuf);
 
-    ASSERT (TcpVerifySegment (Nbuf) != 0);
+    if (TcpVerifySegment (Nbuf) == 0) {
+      DEBUG (
+        (EFI_D_ERROR,
+        "TcpToSendData: discard a broken segment for TCB %p\n",
+        Tcb)
+        );
+      NetbufFree (Nbuf);
+      return -1;
+    }
+    
     ASSERT (Nbuf->Tcp == NULL);
 
     if (TCP_SEQ_GT (Seg->Seq, Seq)) {
       break;
     }
@@ -559,12 +574,15 @@ TcpDeliverData (
   Store the data into the reassemble queue.
 
   @param[in, out]  Tcb   Pointer to the TCP_CB of this TCP instance.
   @param[in]       Nbuf  Pointer to the buffer containing the data to be queued.
 
+  @retval          0     An error condition occurred.
+  @retval          1     No error occurred to queue data.
+
 **/
-VOID
+INTN
 TcpQueueData (
   IN OUT TCP_CB  *Tcb,
   IN     NET_BUF *Nbuf
   )
 {
@@ -586,11 +604,11 @@ TcpQueueData (
   // no out-of-order segments are received.
   //
   if (IsListEmpty (Head)) {
 
     InsertTailList (Head, &Nbuf->List);
-    return;
+    return 1;
   }
 
   //
   // Find the point to insert the buffer
   //
@@ -613,16 +631,16 @@ TcpQueueData (
     Node = NET_LIST_USER_STRUCT (Prev, NET_BUF, List);
 
     if (TCP_SEQ_LT (Seg->Seq, TCPSEG_NETBUF (Node)->End)) {
 
       if (TCP_SEQ_LEQ (Seg->End, TCPSEG_NETBUF (Node)->End)) {
-
-        NetbufFree (Nbuf);
-        return;
+        return 1;
       }
 
-      TcpTrimSegment (Nbuf, TCPSEG_NETBUF (Node)->End, Seg->End);
+      if (TcpTrimSegment (Nbuf, TCPSEG_NETBUF (Node)->End, Seg->End) == 0) {
+        return 0;
+      }
     }
   }
 
   InsertHeadList (Prev, &Nbuf->List);
 
@@ -646,31 +664,38 @@ TcpQueueData (
     if (TCP_SEQ_LT (TCPSEG_NETBUF (Node)->Seq, Seg->End)) {
 
       if (TCP_SEQ_LEQ (TCPSEG_NETBUF (Node)->Seq, Seg->Seq)) {
 
         RemoveEntryList (&Nbuf->List);
-        NetbufFree (Nbuf);
-        return;
+        return 1;
       }
 
-      TcpTrimSegment (Nbuf, Seg->Seq, TCPSEG_NETBUF (Node)->Seq);
+      if (TcpTrimSegment (Nbuf, Seg->Seq, TCPSEG_NETBUF (Node)->Seq) == 0) {
+        RemoveEntryList (&Nbuf->List);
+        return 0;
+      }
       break;
     }
 
     Cur = Cur->ForwardLink;
   }
+
+  return 1;
 }
 
 
 /**
   Adjust the send queue or the retransmit queue.
 
   @param[in]  Tcb      Pointer to the TCP_CB of this TCP instance.
   @param[in]  Ack      The acknowledge seuqence number of the received segment.
 
+  @retval          0     An error condition occurred.
+  @retval          1     No error occurred.
+
 **/
-VOID
+INTN
 TcpAdjustSndQue (
   IN TCP_CB    *Tcb,
   IN TCP_SEQNO Ack
   )
 {
@@ -699,13 +724,14 @@ TcpAdjustSndQue (
       RemoveEntryList (&Node->List);
       NetbufFree (Node);
       continue;
     }
 
-    TcpTrimSegment (Node, Ack, Seg->End);
-    break;
+    return TcpTrimSegment (Node, Ack, Seg->End);
   }
+
+  return 1;
 }
 
 /**
   Process the received TCP segments.
 
@@ -891,11 +917,19 @@ TcpInput (
       TcpInitTcbLocal (Tcb);
       TcpInitTcbPeer (Tcb, Seg, &Option);
 
       TcpSetState (Tcb, TCP_SYN_RCVD);
       TcpSetTimer (Tcb, TCP_TIMER_CONNECT, Tcb->ConnectTimeout);
-      TcpTrimInWnd (Tcb, Nbuf);
+      if (TcpTrimInWnd (Tcb, Nbuf) == 0) {
+        DEBUG (
+          (EFI_D_ERROR,
+          "TcpInput: discard a broken segment for TCB %p\n",
+          Tcb)
+          );
+
+        goto DISCARD;
+      }
 
       goto StepSix;
     }
 
     goto DISCARD;
@@ -973,11 +1007,19 @@ TcpInput (
 
           TcpComputeRtt (Tcb, Tcb->RttMeasure);
           TCP_CLEAR_FLG (Tcb->CtrlFlag, TCP_CTRL_RTT_ON);
         }
 
-        TcpTrimInWnd (Tcb, Nbuf);
+        if (TcpTrimInWnd (Tcb, Nbuf) == 0) {
+          DEBUG (
+            (EFI_D_ERROR,
+            "TcpInput: discard a broken segment for TCB %p\n",
+            Tcb)
+            );
+
+          goto DISCARD;
+        }
 
         TCP_SET_FLG (Tcb->CtrlFlag, TCP_CTRL_ACK_NOW);
 
         DEBUG (
           (EFI_D_NET,
@@ -991,13 +1033,20 @@ TcpInput (
         // Received a SYN segment without ACK, simultanous open.
         //
         TcpSetState (Tcb, TCP_SYN_RCVD);
 
         ASSERT (Tcb->SndNxt == Tcb->Iss + 1);
-        TcpAdjustSndQue (Tcb, Tcb->SndNxt);
 
-        TcpTrimInWnd (Tcb, Nbuf);
+        if (TcpAdjustSndQue (Tcb, Tcb->SndNxt) == 0 || TcpTrimInWnd (Tcb, Nbuf) == 0) {
+          DEBUG (
+            (EFI_D_ERROR,
+            "TcpInput: discard a broken segment for TCB %p\n",
+            Tcb)
+            );
+
+          goto DISCARD;
+        }
 
         DEBUG (
           (EFI_D_WARN,
           "TcpInput: simultaneous open for TCB %p in SYN_SENT\n",
           Tcb)
@@ -1079,11 +1128,19 @@ TcpInput (
   }
 
   //
   // Trim the data and flags.
   //
-  TcpTrimInWnd (Tcb, Nbuf);
+  if (TcpTrimInWnd (Tcb, Nbuf) == 0) {
+    DEBUG (
+      (EFI_D_ERROR,
+      "TcpInput: discard a broken segment for TCB %p\n",
+      Tcb)
+      );
+
+    goto DISCARD;
+  }
 
   //
   // Third step: Check security and precedence, Ignored
   //
 
@@ -1254,11 +1311,20 @@ TcpInput (
     TcpFastRecover (Tcb, Seg);
   }
 
   if (TCP_SEQ_GT (Seg->Ack, Tcb->SndUna)) {
 
-    TcpAdjustSndQue (Tcb, Seg->Ack);
+    if (TcpAdjustSndQue (Tcb, Seg->Ack) == 0) {
+      DEBUG (
+        (EFI_D_ERROR,
+        "TcpInput: discard a broken segment for TCB %p\n",
+        Tcb)
+        );
+
+      goto DISCARD;
+    }
+    
     Tcb->SndUna = Seg->Ack;
 
     if (TCP_FLG_ON (Tcb->CtrlFlag, TCP_CTRL_SND_URG) &&
         TCP_SEQ_LT (Tcb->SndUp, Seg->Ack))
     {
@@ -1487,11 +1553,20 @@ StepSix:
         );
 
       goto RESET_THEN_DROP;
     }
 
-    TcpQueueData (Tcb, Nbuf);
+    if (TcpQueueData (Tcb, Nbuf) == 0) {
+      DEBUG (
+        (EFI_D_ERROR,
+        "TcpInput: discard a broken segment for TCB %p\n",
+        Tcb)
+        );
+
+      goto DISCARD;
+    }
+    
     if (TcpDeliverData (Tcb) == -1) {
       goto RESET_THEN_DROP;
     }
 
     if (!IsListEmpty (&Tcb->RcvQue)) {
diff --git a/NetworkPkg/TcpDxe/TcpOutput.c b/NetworkPkg/TcpDxe/TcpOutput.c index a7e59f0..1697514 100644
--- a/NetworkPkg/TcpDxe/TcpOutput.c
+++ b/NetworkPkg/TcpDxe/TcpOutput.c
@@ -290,11 +290,15 @@ TcpTransmitSegment (
   TCP_HEAD  *Head;
   TCP_SEG   *Seg;
   BOOLEAN   Syn;
   UINT32    DataLen;
 
-  ASSERT ((Nbuf != NULL) && (Nbuf->Tcp == NULL) && (TcpVerifySegment (Nbuf) != 0));
+  ASSERT ((Nbuf != NULL) && (Nbuf->Tcp == NULL));
+
+  if (TcpVerifySegment (Nbuf) == 0) {
+    return -1;
+  }
 
   DataLen = Nbuf->TotalSize;
 
   Seg     = TCPSEG_NETBUF (Nbuf);
   Syn     = TCP_FLG_ON (Seg->Flag, TCP_FLG_SYN);
@@ -632,11 +636,15 @@ TcpGetSegment (
   } else {
 
     Nbuf = TcpGetSegmentSock (Tcb, Seq, Len);
   }
 
-  ASSERT (TcpVerifySegment (Nbuf) != 0);
+  if (TcpVerifySegment (Nbuf) == 0) {
+    NetbufFree (Nbuf);
+    return NULL;
+  }
+  
   return Nbuf;
 }
 
 /**
   Retransmit the segment from sequence Seq.
@@ -699,11 +707,13 @@ TcpRetransmit (
   Nbuf = TcpGetSegmentSndQue (Tcb, Seq, Len);
   if (Nbuf == NULL) {
     return -1;
   }
 
-  ASSERT (TcpVerifySegment (Nbuf) != 0);
+  if (TcpVerifySegment (Nbuf) == 0) {
+    goto OnError;
+  }
 
   if (TcpTransmitSegment (Tcb, Nbuf) != 0) {
     goto OnError;
   }
 
@@ -884,12 +894,18 @@ TcpToSendData (
 
     Seg->Seq  = Seq;
     Seg->End  = End;
     Seg->Flag = Flag;
 
-    ASSERT (TcpVerifySegment (Nbuf) != 0);
-    ASSERT (TcpCheckSndQue (&Tcb->SndQue) != 0);
+    if (TcpVerifySegment (Nbuf) == 0 || TcpCheckSndQue (&Tcb->SndQue) == 0) {
+      DEBUG (
+        (EFI_D_ERROR,
+        "TcpToSendData: discard a broken segment for TCB %p\n",
+        Tcb)
+        );
+      goto OnError;
+    }
 
     //
     // Don't send an empty segment here.
     //
     if (Seg->End == Seg->Seq) {
@@ -897,12 +913,11 @@ TcpToSendData (
         (EFI_D_WARN,
         "TcpToSendData: created a empty segment for TCB %p, free it now\n",
         Tcb)
         );
 
-      NetbufFree (Nbuf);
-      return Sent;
+      goto OnError;
     }
 
     if (TcpTransmitSegment (Tcb, Nbuf) != 0) {
       NetbufTrim (Nbuf, (Nbuf->Tcp->HeadLen << 2), NET_BUF_HEAD);
       Nbuf->Tcp = NULL;
--
1.9.5.msysgit.1