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.20; helo=mga02.intel.com; envelope-from=siyuan.fu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 BC3F522280C22 for ; Tue, 26 Dec 2017 18:58:33 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Dec 2017 19:03:28 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,462,1508828400"; d="scan'208";a="190020111" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga005.fm.intel.com with ESMTP; 26 Dec 2017 19:03:28 -0800 Received: from fmsmsx157.amr.corp.intel.com (10.18.116.73) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 26 Dec 2017 19:03:27 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX157.amr.corp.intel.com (10.18.116.73) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 26 Dec 2017 19:03:27 -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 11:03:25 +0800 From: "Fu, Siyuan" To: "Wu, Jiaxin" , "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: AQHTfhXPHyGEmuGdj06JtvRMjJbCW6NWgcEQ Date: Wed, 27 Dec 2017 03:03:25 +0000 Message-ID: References: <1514271011-9780-1-git-send-email-jiaxin.wu@intel.com> In-Reply-To: <1514271011-9780-1-git-send-email-jiaxin.wu@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZGY4YTYyYzAtNDNlYi00NjBiLTkyNDctY2E2YzY4ZmE5YTUwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJZY3d5MlIyYlVaMWZNZlFDNHNxWkNPTDhSMFM4V0FSbzlETm9cL2dDTDJhWE1Ld21KZ3YyU2dvQkFCa1IxVkF2eiJ9 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 02:58:34 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi, Jiaxin I think it's better to return a Boolean type than int 0-1 value in TcpTrimS= egment(). Other part of good to me. Reviewed-by: Fu Siyuan > -----Original Message----- > From: Wu, Jiaxin > Sent: Tuesday, December 26, 2017 2:50 PM > To: edk2-devel@lists.01.org > Cc: Ye, Ting ; Fu, Siyuan ; Wang, > Fan ; Wu, Jiaxin > Subject: [Patch 3/3] NetworkPkg/TcpDxe: Check TCP payload for release > version. >=20 > 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); >=20 > This patch is to enable the check for release version. >=20 > 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(-) >=20 > 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 ( >=20 > @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. >=20 > + @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); >=20 > Seg->Seq =3D Seg->End; > NetbufTrim (Nbuf, Nbuf->TotalSize, NET_BUF_HEAD); > - return; > + return 1; > } >=20 > // > // Adjust the buffer header > // > @@ -357,27 +360,30 @@ TcpTrimSegment ( > if (Drop !=3D 0) { > NetbufTrim (Nbuf, Drop, NET_BUF_TAIL); > } > } >=20 > - ASSERT (TcpVerifySegment (Nbuf) !=3D 0); > + return TcpVerifySegment (Nbuf); > } >=20 > /** > Trim off the data outside the tcb's receive window. >=20 > @param[in] Tcb Pointer to the TCP_CB of this TCP instance. > @param[in] Nbuf Pointer to the NET_BUF containing the received tc= p > segment. >=20 > + @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); > } >=20 > /** > Process the data and FIN flag, and check whether to deliver > data to the socket layer. > @@ -419,11 +425,20 @@ TcpDeliverData ( >=20 > while (Entry !=3D &Tcb->RcvQue) { > Nbuf =3D NET_LIST_USER_STRUCT (Entry, NET_BUF, List); > Seg =3D TCPSEG_NETBUF (Nbuf); >=20 > - 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); >=20 > if (TCP_SEQ_GT (Seg->Seq, Seq)) { > break; > } > @@ -559,12 +574,15 @@ TcpDeliverData ( > Store the data into the reassemble queue. >=20 > @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. >=20 > + @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)) { >=20 > InsertTailList (Head, &Nbuf->List); > - return; > + return 1; > } >=20 > // > // Find the point to insert the buffer > // > @@ -613,16 +631,16 @@ TcpQueueData ( > Node =3D NET_LIST_USER_STRUCT (Prev, NET_BUF, List); >=20 > if (TCP_SEQ_LT (Seg->Seq, TCPSEG_NETBUF (Node)->End)) { >=20 > if (TCP_SEQ_LEQ (Seg->End, TCPSEG_NETBUF (Node)->End)) { > - > - NetbufFree (Nbuf); > - return; > + return 1; > } >=20 > - TcpTrimSegment (Nbuf, TCPSEG_NETBUF (Node)->End, Seg->End); > + if (TcpTrimSegment (Nbuf, TCPSEG_NETBUF (Node)->End, Seg->End) =3D= =3D 0) > { > + return 0; > + } > } > } >=20 > InsertHeadList (Prev, &Nbuf->List); >=20 > @@ -646,31 +664,38 @@ TcpQueueData ( > if (TCP_SEQ_LT (TCPSEG_NETBUF (Node)->Seq, Seg->End)) { >=20 > if (TCP_SEQ_LEQ (TCPSEG_NETBUF (Node)->Seq, Seg->Seq)) { >=20 > RemoveEntryList (&Nbuf->List); > - NetbufFree (Nbuf); > - return; > + return 1; > } >=20 > - 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; > } >=20 > Cur =3D Cur->ForwardLink; > } > + > + return 1; > } >=20 >=20 > /** > Adjust the send queue or the retransmit queue. >=20 > @param[in] Tcb Pointer to the TCP_CB of this TCP instance. > @param[in] Ack The acknowledge seuqence number of the received > segment. >=20 > + @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; > } >=20 > - TcpTrimSegment (Node, Ack, Seg->End); > - break; > + return TcpTrimSegment (Node, Ack, Seg->End); > } > + > + return 1; > } >=20 > /** > Process the received TCP segments. >=20 > @@ -891,11 +917,19 @@ TcpInput ( > TcpInitTcbLocal (Tcb); > TcpInitTcbPeer (Tcb, Seg, &Option); >=20 > 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; > + } >=20 > goto StepSix; > } >=20 > goto DISCARD; > @@ -973,11 +1007,19 @@ TcpInput ( >=20 > TcpComputeRtt (Tcb, Tcb->RttMeasure); > TCP_CLEAR_FLG (Tcb->CtrlFlag, TCP_CTRL_RTT_ON); > } >=20 > - 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; > + } >=20 > TCP_SET_FLG (Tcb->CtrlFlag, TCP_CTRL_ACK_NOW); >=20 > DEBUG ( > (EFI_D_NET, > @@ -991,13 +1033,20 @@ TcpInput ( > // Received a SYN segment without ACK, simultanous open. > // > TcpSetState (Tcb, TCP_SYN_RCVD); >=20 > ASSERT (Tcb->SndNxt =3D=3D Tcb->Iss + 1); > - TcpAdjustSndQue (Tcb, Tcb->SndNxt); >=20 > - TcpTrimInWnd (Tcb, Nbuf); > + if (TcpAdjustSndQue (Tcb, Tcb->SndNxt) =3D=3D 0 || TcpTrimInWnd = (Tcb, > Nbuf) =3D=3D 0) { > + DEBUG ( > + (EFI_D_ERROR, > + "TcpInput: discard a broken segment for TCB %p\n", > + Tcb) > + ); > + > + goto DISCARD; > + } >=20 > DEBUG ( > (EFI_D_WARN, > "TcpInput: simultaneous open for TCB %p in SYN_SENT\n", > Tcb) > @@ -1079,11 +1128,19 @@ TcpInput ( > } >=20 > // > // 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; > + } >=20 > // > // Third step: Check security and precedence, Ignored > // >=20 > @@ -1254,11 +1311,20 @@ TcpInput ( > TcpFastRecover (Tcb, Seg); > } >=20 > if (TCP_SEQ_GT (Seg->Ack, Tcb->SndUna)) { >=20 > - 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; >=20 > if (TCP_FLG_ON (Tcb->CtrlFlag, TCP_CTRL_SND_URG) && > TCP_SEQ_LT (Tcb->SndUp, Seg->Ack)) > { > @@ -1487,11 +1553,20 @@ StepSix: > ); >=20 > goto RESET_THEN_DROP; > } >=20 > - 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; > } >=20 > 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; >=20 > - ASSERT ((Nbuf !=3D NULL) && (Nbuf->Tcp =3D=3D NULL) && (TcpVerifySegme= nt > (Nbuf) !=3D 0)); > + ASSERT ((Nbuf !=3D NULL) && (Nbuf->Tcp =3D=3D NULL)); > + > + if (TcpVerifySegment (Nbuf) =3D=3D 0) { > + return -1; > + } >=20 > DataLen =3D Nbuf->TotalSize; >=20 > Seg =3D TCPSEG_NETBUF (Nbuf); > Syn =3D TCP_FLG_ON (Seg->Flag, TCP_FLG_SYN); > @@ -632,11 +636,15 @@ TcpGetSegment ( > } else { >=20 > Nbuf =3D TcpGetSegmentSock (Tcb, Seq, Len); > } >=20 > - ASSERT (TcpVerifySegment (Nbuf) !=3D 0); > + if (TcpVerifySegment (Nbuf) =3D=3D 0) { > + NetbufFree (Nbuf); > + return NULL; > + } > + > return Nbuf; > } >=20 > /** > Retransmit the segment from sequence Seq. > @@ -699,11 +707,13 @@ TcpRetransmit ( > Nbuf =3D TcpGetSegmentSndQue (Tcb, Seq, Len); > if (Nbuf =3D=3D NULL) { > return -1; > } >=20 > - ASSERT (TcpVerifySegment (Nbuf) !=3D 0); > + if (TcpVerifySegment (Nbuf) =3D=3D 0) { > + goto OnError; > + } >=20 > if (TcpTransmitSegment (Tcb, Nbuf) !=3D 0) { > goto OnError; > } >=20 > @@ -884,12 +894,18 @@ TcpToSendData ( >=20 > Seg->Seq =3D Seq; > Seg->End =3D End; > Seg->Flag =3D Flag; >=20 > - ASSERT (TcpVerifySegment (Nbuf) !=3D 0); > - ASSERT (TcpCheckSndQue (&Tcb->SndQue) !=3D 0); > + if (TcpVerifySegment (Nbuf) =3D=3D 0 || TcpCheckSndQue (&Tcb->SndQue= ) =3D=3D > 0) { > + DEBUG ( > + (EFI_D_ERROR, > + "TcpToSendData: discard a broken segment for TCB %p\n", > + Tcb) > + ); > + goto OnError; > + } >=20 > // > // 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) > ); >=20 > - NetbufFree (Nbuf); > - return Sent; > + goto OnError; > } >=20 > if (TcpTransmitSegment (Tcb, Nbuf) !=3D 0) { > NetbufTrim (Nbuf, (Nbuf->Tcp->HeadLen << 2), NET_BUF_HEAD); > Nbuf->Tcp =3D NULL; > -- > 1.9.5.msysgit.1