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

["Fu, Siyuan" <siyuan.fu@intel.com>]

Hi, Jiaxin

I think it's better to return a Boolean type than int 0-1 value in TcpTrimSegment().
Other part of good to me.


Reviewed-by: Fu Siyuan <siyuan.fu@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