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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

After talked with Siyuan, we agree to use the int 0-1 value instead of Boolean type so as to keep the same coding style in TcpInput.c.

Thanks,
Jiaxin 

> -----Original Message-----
> From: Fu, Siyuan
> Sent: Wednesday, December 27, 2017 11:03 AM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Wang, Fan <fan.wang@intel.com>
> Subject: RE: [Patch 3/3] NetworkPkg/TcpDxe: Check TCP payload for release
> version.
> 
> 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