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=ting.ye@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 2917E21A143F0 for ; Tue, 26 Dec 2017 19:06:14 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Dec 2017 19:11:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,462,1508828400"; d="scan'208";a="5262382" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga007.fm.intel.com with ESMTP; 26 Dec 2017 19:11:08 -0800 Received: from fmsmsx119.amr.corp.intel.com (10.18.124.207) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 26 Dec 2017 19:11:08 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX119.amr.corp.intel.com (10.18.124.207) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 26 Dec 2017 19:11:07 -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:11:06 +0800 From: "Ye, Ting" To: "Wu, Jiaxin" , "edk2-devel@lists.01.org" CC: "Fu, Siyuan" , "Wang, Fan" Thread-Topic: [Patch 3/3] NetworkPkg/TcpDxe: Check TCP payload for release version. Thread-Index: AQHTfhXPHyGEmuGdj06JtvRMjJbCW6NWhGYg Date: Wed, 27 Dec 2017 03:11:05 +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: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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 03:06:14 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Ye Ting =20 -----Original Message----- From: Wu, Jiaxin=20 Sent: Tuesday, December 26, 2017 2:50 PM To: edk2-devel@lists.01.org Cc: Ye, Ting ; Fu, Siyuan ; Wang, F= an ; Wu, Jiaxin Subject: [Patch 3/3] NetworkPkg/TcpDxe: Check TCP payload for release versi= on. TCP payload check is implemented by TcpVerifySegment(), but all the functio= n calls of TcpVerifySegment() are placed in ASSERT(), which is only valid f= or 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 in= dex 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 wit= hout 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 tcp = 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; + } + =20 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 q= ueued. =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 seg= ment. =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 (T= cb, 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; + } + =20 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; + } + =20 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) && (TcpVerifySegment= (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; + } + =20 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