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 <[email protected]> > -----Original Message----- > From: Wu, Jiaxin > Sent: Tuesday, December 26, 2017 2:50 PM > To: [email protected] > Cc: Ye, Ting <[email protected]>; Fu, Siyuan <[email protected]>; Wang, > Fan <[email protected]>; Wu, Jiaxin <[email protected]> > 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 <[email protected]> > Cc: Fu Siyuan <[email protected]> > Cc: Wang Fan <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Wu Jiaxin <[email protected]> > --- > 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
