From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=jiaxin.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BECFE22280C23 for ; Wed, 27 Dec 2017 00:20:17 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Dec 2017 00:25:11 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,464,1508828400"; d="scan'208";a="187365954" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga005.jf.intel.com with ESMTP; 27 Dec 2017 00:25:11 -0800 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 27 Dec 2017 00:25:11 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 27 Dec 2017 00:25:10 -0800 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.213]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.152]) with mapi id 14.03.0319.002; Wed, 27 Dec 2017 16:25:09 +0800 From: "Wu, Jiaxin" To: "Fu, Siyuan" , "edk2-devel@lists.01.org" CC: "Ye, Ting" , "Wang, Fan" Thread-Topic: [Patch 3/3] NetworkPkg/TcpDxe: Check TCP payload for release version. Thread-Index: AQHTfhXPHyGEmuGdj06JtvRMjJbCW6NWgcEQgABZsTA= Date: Wed, 27 Dec 2017 08:25:08 +0000 Message-ID: <895558F6EA4E3B41AC93A00D163B7274163575E4@SHSMSX103.ccr.corp.intel.com> References: <1514271011-9780-1-git-send-email-jiaxin.wu@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNWQyMjM4ZmQtZmQwNC00ODJlLThlYWItZjhhOTFiNTk5NTNlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IjljdTI5U0JvVnRwK1RaQVRcL2ZDYldmWVlNdlUzODRvemVzZzFjS0lXTG9JPSJ9 x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [Patch 3/3] NetworkPkg/TcpDxe: Check TCP payload for release version. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Dec 2017 08:20:18 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable After talked with Siyuan, we agree to use the int 0-1 value instead of Bool= ean type so as to keep the same coding style in TcpInput.c. Thanks, Jiaxin=20 > -----Original Message----- > From: Fu, Siyuan > Sent: Wednesday, December 27, 2017 11:03 AM > To: Wu, Jiaxin ; edk2-devel@lists.01.org > Cc: Ye, Ting ; Wang, Fan > Subject: RE: [Patch 3/3] NetworkPkg/TcpDxe: Check TCP payload for release > version. >=20 > Hi, Jiaxin >=20 > I think it's better to return a Boolean type than int 0-1 value in > TcpTrimSegment(). > Other part of good to me. >=20 >=20 > Reviewed-by: Fu Siyuan >=20 >=20 >=20 > > -----Original Message----- > > From: Wu, Jiaxin > > Sent: Tuesday, December 26, 2017 2:50 PM > > To: edk2-devel@lists.01.org > > Cc: Ye, Ting ; Fu, Siyuan ; Wan= g, > > Fan ; Wu, Jiaxin > > 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) !=3D 0); > > > > This patch is to enable the check for release version. > > > > Cc: Ye Ting > > Cc: Fu Siyuan > > Cc: Wang Fan > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Wu Jiaxin > > --- > > 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 =3D Seg->End; > > NetbufTrim (Nbuf, Nbuf->TotalSize, NET_BUF_HEAD); > > - return; > > + return 1; > > } > > > > // > > // Adjust the buffer header > > // > > @@ -357,27 +360,30 @@ TcpTrimSegment ( > > if (Drop !=3D 0) { > > NetbufTrim (Nbuf, Drop, NET_BUF_TAIL); > > } > > } > > > > - ASSERT (TcpVerifySegment (Nbuf) !=3D 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 !=3D &Tcb->RcvQue) { > > Nbuf =3D NET_LIST_USER_STRUCT (Entry, NET_BUF, List); > > Seg =3D TCPSEG_NETBUF (Nbuf); > > > > - ASSERT (TcpVerifySegment (Nbuf) !=3D 0); > > + if (TcpVerifySegment (Nbuf) =3D=3D 0) { > > + DEBUG ( > > + (EFI_D_ERROR, > > + "TcpToSendData: discard a broken segment for TCB %p\n", > > + Tcb) > > + ); > > + NetbufFree (Nbuf); > > + return -1; > > + } > > + > > ASSERT (Nbuf->Tcp =3D=3D 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 =3D 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) > =3D=3D 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) > =3D=3D 0) > > { > > + RemoveEntryList (&Nbuf->List); > > + return 0; > > + } > > break; > > } > > > > Cur =3D 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) =3D=3D 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) =3D=3D 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 =3D=3D Tcb->Iss + 1); > > - TcpAdjustSndQue (Tcb, Tcb->SndNxt); > > > > - TcpTrimInWnd (Tcb, Nbuf); > > + if (TcpAdjustSndQue (Tcb, Tcb->SndNxt) =3D=3D 0 || TcpTrimInWn= d (Tcb, > > Nbuf) =3D=3D 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) =3D=3D 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) =3D=3D 0) { > > + DEBUG ( > > + (EFI_D_ERROR, > > + "TcpInput: discard a broken segment for TCB %p\n", > > + Tcb) > > + ); > > + > > + goto DISCARD; > > + } > > + > > Tcb->SndUna =3D 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) =3D=3D 0) { > > + DEBUG ( > > + (EFI_D_ERROR, > > + "TcpInput: discard a broken segment for TCB %p\n", > > + Tcb) > > + ); > > + > > + goto DISCARD; > > + } > > + > > if (TcpDeliverData (Tcb) =3D=3D -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 !=3D NULL) && (Nbuf->Tcp =3D=3D NULL) && (TcpVerifySeg= ment > > (Nbuf) !=3D 0)); > > + ASSERT ((Nbuf !=3D NULL) && (Nbuf->Tcp =3D=3D NULL)); > > + > > + if (TcpVerifySegment (Nbuf) =3D=3D 0) { > > + return -1; > > + } > > > > DataLen =3D Nbuf->TotalSize; > > > > Seg =3D TCPSEG_NETBUF (Nbuf); > > Syn =3D TCP_FLG_ON (Seg->Flag, TCP_FLG_SYN); > > @@ -632,11 +636,15 @@ TcpGetSegment ( > > } else { > > > > Nbuf =3D TcpGetSegmentSock (Tcb, Seq, Len); > > } > > > > - ASSERT (TcpVerifySegment (Nbuf) !=3D 0); > > + if (TcpVerifySegment (Nbuf) =3D=3D 0) { > > + NetbufFree (Nbuf); > > + return NULL; > > + } > > + > > return Nbuf; > > } > > > > /** > > Retransmit the segment from sequence Seq. > > @@ -699,11 +707,13 @@ TcpRetransmit ( > > Nbuf =3D TcpGetSegmentSndQue (Tcb, Seq, Len); > > if (Nbuf =3D=3D NULL) { > > return -1; > > } > > > > - ASSERT (TcpVerifySegment (Nbuf) !=3D 0); > > + if (TcpVerifySegment (Nbuf) =3D=3D 0) { > > + goto OnError; > > + } > > > > if (TcpTransmitSegment (Tcb, Nbuf) !=3D 0) { > > goto OnError; > > } > > > > @@ -884,12 +894,18 @@ TcpToSendData ( > > > > Seg->Seq =3D Seq; > > Seg->End =3D End; > > Seg->Flag =3D Flag; > > > > - ASSERT (TcpVerifySegment (Nbuf) !=3D 0); > > - ASSERT (TcpCheckSndQue (&Tcb->SndQue) !=3D 0); > > + if (TcpVerifySegment (Nbuf) =3D=3D 0 || TcpCheckSndQue (&Tcb->SndQ= ue) > =3D=3D > > 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 =3D=3D 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) !=3D 0) { > > NetbufTrim (Nbuf, (Nbuf->Tcp->HeadLen << 2), NET_BUF_HEAD); > > Nbuf->Tcp =3D NULL; > > -- > > 1.9.5.msysgit.1