From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 E86B682066 for ; Fri, 16 Dec 2016 00:59:39 -0800 (PST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 16 Dec 2016 00:59:37 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,356,1477983600"; d="scan'208";a="18878712" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 16 Dec 2016 00:59:37 -0800 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 16 Dec 2016 00:59:37 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 16 Dec 2016 00:59:37 -0800 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.11]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.77]) with mapi id 14.03.0248.002; Fri, 16 Dec 2016 16:59:32 +0800 From: "Wu, Jiaxin" To: "Fu, Siyuan" , "edk2-devel@lists.01.org" CC: "Yao, Jiewen" , "Ye, Ting" Thread-Topic: [Patch 1/2] MdeModulePkg: Replace ASSERT with error return code in PXE driver. Thread-Index: AQHSV3UQsxZTm2V6EU64Ql1thWp5FaEKRmdA Date: Fri, 16 Dec 2016 08:59:32 +0000 Message-ID: <895558F6EA4E3B41AC93A00D163B727416280CC1@SHSMSX103.ccr.corp.intel.com> References: <1481876334-130492-1-git-send-email-siyuan.fu@intel.com> <1481876334-130492-2-git-send-email-siyuan.fu@intel.com> In-Reply-To: <1481876334-130492-2-git-send-email-siyuan.fu@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZWM2N2EyNjYtYTY2ZS00ZDU0LWFiZjQtMGM3ZmM2MzlkMDU2IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Ik9FWGJjeDArK24wcFllUVU2WmlMaElianpDeFdCaVlaN1U0akswWWRjbzg9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [Patch 1/2] MdeModulePkg: Replace ASSERT with error return code in PXE driver. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Dec 2016 08:59:40 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Wu Jiaxin Thanks, Jiaxin > -----Original Message----- > From: Fu, Siyuan > Sent: Friday, December 16, 2016 4:19 PM > To: edk2-devel@lists.01.org > Cc: Yao, Jiewen ; Ye, Ting ; Wu, > Jiaxin > Subject: [Patch 1/2] MdeModulePkg: Replace ASSERT with error return code > in PXE driver. >=20 > This patch remove the ASSERT when receive a DHCP packet large than the > maximum cache buffer size. >=20 > Cc: Yao Jiewen > Cc: Ye Ting > Cc: Wu Jiaxin > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Fu Siyuan > --- > .../Universal/Network/UefiPxeBcDxe/PxeBcDhcp.c | 99 > ++++++++++++++-------- > .../Universal/Network/UefiPxeBcDxe/PxeBcDhcp.h | 5 +- > 2 files changed, 68 insertions(+), 36 deletions(-) >=20 > diff --git a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcDhcp.c > b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcDhcp.c > index f03176b..f0720e5 100644 > --- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcDhcp.c > +++ b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcDhcp.c > @@ -69,20 +69,26 @@ PxeBcInitSeedPacket ( > /** > Copy the DCHP4 packet from srouce to destination. >=20 > - @param Dst Pointer to the EFI_DHCP4_PROTOCOL instance. > - @param Src Pointer to the EFI_DHCP4_PROTOCOL instance. > + @param[in] Dst Pointer to the cache buffer for DHCPv4 packet= . > + @param[in] Src Pointer to the DHCPv4 packet to be cached. > + > + @retval EFI_SUCCESS Packet is copied. > + @retval EFI_BUFFER_TOO_SMALL Cache buffer is not big enough = to > hold the packet. >=20 > **/ > -VOID > +EFI_STATUS > PxeBcCopyEfiDhcp4Packet ( > IN EFI_DHCP4_PACKET *Dst, > IN EFI_DHCP4_PACKET *Src > ) > { > - ASSERT (Dst->Size >=3D Src->Length); > + if (Dst->Size < Src->Length) { > + return EFI_BUFFER_TOO_SMALL; > + } >=20 > CopyMem (&Dst->Dhcp4, &Src->Dhcp4, Src->Length); > Dst->Length =3D Src->Length; > + return EFI_SUCCESS; > } >=20 >=20 > @@ -93,8 +99,11 @@ PxeBcCopyEfiDhcp4Packet ( > @param OfferIndex Index of cached packets as complements of pxe > mode data, > the index is maximum offer number. >=20 > + @retval EFI_SUCCESS Cache and parse the packet succ= essfully. > + @retval EFI_BUFFER_TOO_SMALL Cache buffer is not big enough = to > hold the packet. > + > **/ > -VOID > +EFI_STATUS > PxeBcCopyProxyOffer ( > IN PXEBC_PRIVATE_DATA *Private, > IN UINT32 OfferIndex > @@ -102,6 +111,7 @@ PxeBcCopyProxyOffer ( { > EFI_PXE_BASE_CODE_MODE *Mode; > EFI_DHCP4_PACKET *Offer; > + EFI_STATUS Status; >=20 > ASSERT (OfferIndex < Private->NumOffers); > ASSERT (OfferIndex < PXEBC_MAX_OFFER_NUM); @@ -109,11 +119,15 > @@ PxeBcCopyProxyOffer ( > Mode =3D Private->PxeBc.Mode; > Offer =3D &Private->Dhcp4Offers[OfferIndex].Packet.Offer; >=20 > - PxeBcCopyEfiDhcp4Packet (&Private->ProxyOffer.Packet.Offer, Offer); > + Status =3D PxeBcCopyEfiDhcp4Packet (&Private->ProxyOffer.Packet.Offer, > + Offer); if (EFI_ERROR(Status)) { > + return Status; > + } > CopyMem (&Mode->ProxyOffer, &Offer->Dhcp4, Offer->Length); > Mode->ProxyOfferReceived =3D TRUE; >=20 > PxeBcParseCachedDhcpPacket (&Private->ProxyOffer); > + return EFI_SUCCESS; > } >=20 >=20 > @@ -411,8 +425,9 @@ PxeBcTryBinlProxy ( >=20 > @param Private Pointer to PxeBc private data. >=20 > - @retval EFI_SUCCESS Operational successful. > - @retval EFI_NO_RESPONSE Offer dhcp service failed. > + @retval EFI_SUCCESS Operational successful. > + @retval EFI_NO_RESPONSE Offer dhcp service failed. > + @retval EFI_BUFFER_TOO_SMALL Failed to copy the packet to Pxe ba= se > code mode. >=20 > **/ > EFI_STATUS > @@ -515,7 +530,7 @@ PxeBcCheckSelectedOffer ( > // > // Copy the proxy offer to Mode and set the flag > // > - PxeBcCopyProxyOffer (Private, ProxyOfferIndex); > + Status =3D PxeBcCopyProxyOffer (Private, ProxyOfferIndex); > } > } else { > // > @@ -543,7 +558,7 @@ PxeBcCheckSelectedOffer ( > // Other type of ACK is already cached. Bootp is special that we s= hould > // use the bootp reply as the ACK and put it into the DHCP_ONLY bu= ffer. > // > - PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, Offer); > + Status =3D PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, > + Offer); > } >=20 > PxeBcParseCachedDhcpPacket (&Private->Dhcp4Ack); @@ -566,8 +581,11 > @@ PxeBcCheckSelectedOffer ( > @param Private Pointer to PxeBc private data. > @param RcvdOffer Pointer to the received Dhcp proxy offer packet. >=20 > + @retval EFI_SUCCESS Cache and parse the packet successfully. > + @retval Others Operation failed. > + > **/ > -VOID > +EFI_STATUS > PxeBcCacheDhcpOffer ( > IN PXEBC_PRIVATE_DATA *Private, > IN EFI_DHCP4_PACKET *RcvdOffer > @@ -576,6 +594,7 @@ PxeBcCacheDhcpOffer ( > PXEBC_CACHED_DHCP4_PACKET *CachedOffer; > EFI_DHCP4_PACKET *Offer; > UINT8 OfferType; > + EFI_STATUS Status; >=20 > CachedOffer =3D &Private->Dhcp4Offers[Private->NumOffers]; > Offer =3D &CachedOffer->Packet.Offer; > @@ -583,18 +602,21 @@ PxeBcCacheDhcpOffer ( > // > // Cache the orignal dhcp packet > // > - PxeBcCopyEfiDhcp4Packet (Offer, RcvdOffer); > + Status =3D PxeBcCopyEfiDhcp4Packet (Offer, RcvdOffer); if > + (EFI_ERROR(Status)) { > + return Status; > + } >=20 > // > // Parse and validate the options (including dhcp option and vendor op= tion) > // > if (!PxeBcParseCachedDhcpPacket (CachedOffer)) { > - return ; > + return EFI_ABORTED; > } >=20 > OfferType =3D CachedOffer->OfferType; > if (OfferType >=3D DHCP4_PACKET_TYPE_MAX) { > - return ; > + return EFI_ABORTED; > } >=20 > if (OfferType =3D=3D DHCP4_PACKET_TYPE_BOOTP) { @@ -603,7 +625,7 @@ > PxeBcCacheDhcpOffer ( > // > // Only cache the first bootp offer, discard others. > // > - return ; > + return EFI_ABORTED; > } else { > // > // Take as a dhcp only offer, but record index specifically. > @@ -628,7 +650,7 @@ PxeBcCacheDhcpOffer ( > // > // Only cache the first pxe10/wfm11a offers each, discard the ot= hers. > // > - return ; > + return EFI_ABORTED; > } else { > // > // Record index of the proxy dhcp offer with type other than bin= l. > @@ -649,6 +671,8 @@ PxeBcCacheDhcpOffer ( > // Count the accepted offers. > // > Private->NumOffers++; > + > + return EFI_SUCCESS; > } >=20 > /** > @@ -960,6 +984,7 @@ PxeBcDhcpCallBack ( > if (Private->NumOffers < PXEBC_MAX_OFFER_NUM) { > // > // Cache the dhcp offers in Private->Dhcp4Offers[] > + // If error happens, just ignore this packet and continue to wait = more > offer. > // > PxeBcCacheDhcpOffer (Private, Packet); > } > @@ -982,20 +1007,15 @@ PxeBcDhcpCallBack ( > break; >=20 > case Dhcp4RcvdAck: > - if (Packet->Length > PXEBC_DHCP4_MAX_PACKET_SIZE) { > - // > - // Abort the DHCP if the ACK packet exceeds the maximum length. > - // > - Status =3D EFI_ABORTED; > - break; > - } > - > // > // Cache Ack > // > ASSERT (Private->SelectedOffer !=3D 0); >=20 > - PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, Packet); > + Status =3D PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, > Packet); > + if (EFI_ERROR (Status)) { > + return EFI_ABORTED; > + } > break; >=20 > default: > @@ -1340,6 +1360,12 @@ PxeBcDiscvBootService ( > Response =3D Token.ResponseList; >=20 > while (RepIndex < Token.ResponseCount) { > + if (Response->Length > PXEBC_DHCP4_MAX_PACKET_SIZE) { > + SrvIndex =3D 0; > + RepIndex++; > + Response =3D (EFI_DHCP4_PACKET *) ((UINT8 *) Response + Response= - > >Size); > + continue; > + } >=20 > while (SrvIndex < IpCount) { >=20 > @@ -1360,14 +1386,16 @@ PxeBcDiscvBootService ( >=20 > SrvIndex =3D 0; > RepIndex++; > - > Response =3D (EFI_DHCP4_PACKET *) ((UINT8 *) Response + Response- > >Size); > } >=20 > if (RepIndex < Token.ResponseCount) { >=20 > if (Reply !=3D NULL) { > - PxeBcCopyEfiDhcp4Packet (Reply, Response); > + Status =3D PxeBcCopyEfiDhcp4Packet (Reply, Response); > + if (EFI_ERROR(Status)) { > + goto ON_EXIT; > + } > } >=20 > if (IsDiscv) { > @@ -1380,18 +1408,21 @@ PxeBcDiscvBootService ( > } else { > Status =3D EFI_NOT_FOUND; > } > + } >=20 > - // > - // free the responselist > - // > - if (Token.ResponseList !=3D NULL) { > - FreePool (Token.ResponseList); > - } > +ON_EXIT: > + // > + // free the responselist > + // > + if (Token.ResponseList !=3D NULL) { > + FreePool (Token.ResponseList); > } > // > // Free the dhcp packet > // > - FreePool (Token.Packet); > + if (Token.Packet !=3D NULL) { > + FreePool (Token.Packet); > + } >=20 > return Status; > } > diff --git a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcDhcp.h > b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcDhcp.h > index 614ea75..d19d2a3 100644 > --- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcDhcp.h > +++ b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcDhcp.h > @@ -291,8 +291,9 @@ PxeBcParseCachedDhcpPacket ( >=20 > @param Private Pointer to PxeBc private data. >=20 > - @retval EFI_SUCCESS Operational successful. > - @retval EFI_NO_RESPONSE Offer dhcp service failed. > + @retval EFI_SUCCESS Operational successful. > + @retval EFI_NO_RESPONSE Offer dhcp service failed. > + @retval EFI_BUFFER_TOO_SMALL Failed to copy the packet to Pxe ba= se > code mode. >=20 > **/ > EFI_STATUS > -- > 2.7.4.windows.1