* [Patch 0/2] Replace ASSERT with error return code in PXE and HTTP boot.
@ 2016-12-16 8:18 Fu Siyuan
2016-12-16 8:18 ` [Patch 1/2] MdeModulePkg: Replace ASSERT with error return code in PXE driver Fu Siyuan
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Fu Siyuan @ 2016-12-16 8:18 UTC (permalink / raw)
To: edk2-devel
This patch remove the ASSERT when receive a DHCP packet large than the maximum
cache buffer size.
Fu Siyuan (2):
MdeModulePkg: Replace ASSERT with error return code in PXE driver.
NetworkPkg: Replace ASSERT with error return code in PXE and HTTP boot
driver.
.../Universal/Network/UefiPxeBcDxe/PxeBcDhcp.c | 99 +++++++++++------
.../Universal/Network/UefiPxeBcDxe/PxeBcDhcp.h | 5 +-
NetworkPkg/HttpBootDxe/HttpBootDhcp4.c | 32 +++++-
NetworkPkg/HttpBootDxe/HttpBootDhcp6.c | 29 +++--
NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c | 119 +++++++++++++++------
NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c | 67 ++++++++----
6 files changed, 249 insertions(+), 102 deletions(-)
--
2.7.4.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [Patch 1/2] MdeModulePkg: Replace ASSERT with error return code in PXE driver. 2016-12-16 8:18 [Patch 0/2] Replace ASSERT with error return code in PXE and HTTP boot Fu Siyuan @ 2016-12-16 8:18 ` Fu Siyuan 2016-12-16 8:59 ` Wu, Jiaxin 2016-12-16 8:18 ` [Patch 2/2] NetworkPkg: Replace ASSERT with error return code in PXE and HTTP boot driver Fu Siyuan 2016-12-16 8:45 ` [Patch 0/2] Replace ASSERT with error return code in PXE and HTTP boot Ye, Ting 2 siblings, 1 reply; 6+ messages in thread From: Fu Siyuan @ 2016-12-16 8:18 UTC (permalink / raw) To: edk2-devel; +Cc: Yao Jiewen, Ye Ting, Wu Jiaxin This patch remove the ASSERT when receive a DHCP packet large than the maximum cache buffer size. Cc: Yao Jiewen <jiewen.yao@intel.com> Cc: Ye Ting <ting.ye@intel.com> Cc: Wu Jiaxin <jiaxin.wu@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Fu Siyuan <siyuan.fu@intel.com> --- .../Universal/Network/UefiPxeBcDxe/PxeBcDhcp.c | 99 ++++++++++++++-------- .../Universal/Network/UefiPxeBcDxe/PxeBcDhcp.h | 5 +- 2 files changed, 68 insertions(+), 36 deletions(-) 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. - @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. **/ -VOID +EFI_STATUS PxeBcCopyEfiDhcp4Packet ( IN EFI_DHCP4_PACKET *Dst, IN EFI_DHCP4_PACKET *Src ) { - ASSERT (Dst->Size >= Src->Length); + if (Dst->Size < Src->Length) { + return EFI_BUFFER_TOO_SMALL; + } CopyMem (&Dst->Dhcp4, &Src->Dhcp4, Src->Length); Dst->Length = Src->Length; + return EFI_SUCCESS; } @@ -93,8 +99,11 @@ PxeBcCopyEfiDhcp4Packet ( @param OfferIndex Index of cached packets as complements of pxe mode data, the index is maximum offer number. + @retval EFI_SUCCESS Cache and parse the packet successfully. + @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; ASSERT (OfferIndex < Private->NumOffers); ASSERT (OfferIndex < PXEBC_MAX_OFFER_NUM); @@ -109,11 +119,15 @@ PxeBcCopyProxyOffer ( Mode = Private->PxeBc.Mode; Offer = &Private->Dhcp4Offers[OfferIndex].Packet.Offer; - PxeBcCopyEfiDhcp4Packet (&Private->ProxyOffer.Packet.Offer, Offer); + Status = PxeBcCopyEfiDhcp4Packet (&Private->ProxyOffer.Packet.Offer, Offer); + if (EFI_ERROR(Status)) { + return Status; + } CopyMem (&Mode->ProxyOffer, &Offer->Dhcp4, Offer->Length); Mode->ProxyOfferReceived = TRUE; PxeBcParseCachedDhcpPacket (&Private->ProxyOffer); + return EFI_SUCCESS; } @@ -411,8 +425,9 @@ PxeBcTryBinlProxy ( @param Private Pointer to PxeBc private data. - @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 base code mode. **/ EFI_STATUS @@ -515,7 +530,7 @@ PxeBcCheckSelectedOffer ( // // Copy the proxy offer to Mode and set the flag // - PxeBcCopyProxyOffer (Private, ProxyOfferIndex); + Status = PxeBcCopyProxyOffer (Private, ProxyOfferIndex); } } else { // @@ -543,7 +558,7 @@ PxeBcCheckSelectedOffer ( // Other type of ACK is already cached. Bootp is special that we should // use the bootp reply as the ACK and put it into the DHCP_ONLY buffer. // - PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, Offer); + Status = PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, Offer); } 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. + @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; CachedOffer = &Private->Dhcp4Offers[Private->NumOffers]; Offer = &CachedOffer->Packet.Offer; @@ -583,18 +602,21 @@ PxeBcCacheDhcpOffer ( // // Cache the orignal dhcp packet // - PxeBcCopyEfiDhcp4Packet (Offer, RcvdOffer); + Status = PxeBcCopyEfiDhcp4Packet (Offer, RcvdOffer); + if (EFI_ERROR(Status)) { + return Status; + } // // Parse and validate the options (including dhcp option and vendor option) // if (!PxeBcParseCachedDhcpPacket (CachedOffer)) { - return ; + return EFI_ABORTED; } OfferType = CachedOffer->OfferType; if (OfferType >= DHCP4_PACKET_TYPE_MAX) { - return ; + return EFI_ABORTED; } if (OfferType == 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 others. // - return ; + return EFI_ABORTED; } else { // // Record index of the proxy dhcp offer with type other than binl. @@ -649,6 +671,8 @@ PxeBcCacheDhcpOffer ( // Count the accepted offers. // Private->NumOffers++; + + return EFI_SUCCESS; } /** @@ -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; case Dhcp4RcvdAck: - if (Packet->Length > PXEBC_DHCP4_MAX_PACKET_SIZE) { - // - // Abort the DHCP if the ACK packet exceeds the maximum length. - // - Status = EFI_ABORTED; - break; - } - // // Cache Ack // ASSERT (Private->SelectedOffer != 0); - PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, Packet); + Status = PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, Packet); + if (EFI_ERROR (Status)) { + return EFI_ABORTED; + } break; default: @@ -1340,6 +1360,12 @@ PxeBcDiscvBootService ( Response = Token.ResponseList; while (RepIndex < Token.ResponseCount) { + if (Response->Length > PXEBC_DHCP4_MAX_PACKET_SIZE) { + SrvIndex = 0; + RepIndex++; + Response = (EFI_DHCP4_PACKET *) ((UINT8 *) Response + Response->Size); + continue; + } while (SrvIndex < IpCount) { @@ -1360,14 +1386,16 @@ PxeBcDiscvBootService ( SrvIndex = 0; RepIndex++; - Response = (EFI_DHCP4_PACKET *) ((UINT8 *) Response + Response->Size); } if (RepIndex < Token.ResponseCount) { if (Reply != NULL) { - PxeBcCopyEfiDhcp4Packet (Reply, Response); + Status = PxeBcCopyEfiDhcp4Packet (Reply, Response); + if (EFI_ERROR(Status)) { + goto ON_EXIT; + } } if (IsDiscv) { @@ -1380,18 +1408,21 @@ PxeBcDiscvBootService ( } else { Status = EFI_NOT_FOUND; } + } - // - // free the responselist - // - if (Token.ResponseList != NULL) { - FreePool (Token.ResponseList); - } +ON_EXIT: + // + // free the responselist + // + if (Token.ResponseList != NULL) { + FreePool (Token.ResponseList); } // // Free the dhcp packet // - FreePool (Token.Packet); + if (Token.Packet != NULL) { + FreePool (Token.Packet); + } 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 ( @param Private Pointer to PxeBc private data. - @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 base code mode. **/ EFI_STATUS -- 2.7.4.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch 1/2] MdeModulePkg: Replace ASSERT with error return code in PXE driver. 2016-12-16 8:18 ` [Patch 1/2] MdeModulePkg: Replace ASSERT with error return code in PXE driver Fu Siyuan @ 2016-12-16 8:59 ` Wu, Jiaxin 0 siblings, 0 replies; 6+ messages in thread From: Wu, Jiaxin @ 2016-12-16 8:59 UTC (permalink / raw) To: Fu, Siyuan, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Ye, Ting Reviewed-by: Wu Jiaxin <jiaxin.wu@intel.com> Thanks, Jiaxin > -----Original Message----- > From: Fu, Siyuan > Sent: Friday, December 16, 2016 4:19 PM > To: edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ye, Ting <ting.ye@intel.com>; Wu, > Jiaxin <jiaxin.wu@intel.com> > Subject: [Patch 1/2] MdeModulePkg: Replace ASSERT with error return code > in PXE driver. > > This patch remove the ASSERT when receive a DHCP packet large than the > maximum cache buffer size. > > Cc: Yao Jiewen <jiewen.yao@intel.com> > Cc: Ye Ting <ting.ye@intel.com> > Cc: Wu Jiaxin <jiaxin.wu@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Fu Siyuan <siyuan.fu@intel.com> > --- > .../Universal/Network/UefiPxeBcDxe/PxeBcDhcp.c | 99 > ++++++++++++++-------- > .../Universal/Network/UefiPxeBcDxe/PxeBcDhcp.h | 5 +- > 2 files changed, 68 insertions(+), 36 deletions(-) > > 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. > > - @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. > > **/ > -VOID > +EFI_STATUS > PxeBcCopyEfiDhcp4Packet ( > IN EFI_DHCP4_PACKET *Dst, > IN EFI_DHCP4_PACKET *Src > ) > { > - ASSERT (Dst->Size >= Src->Length); > + if (Dst->Size < Src->Length) { > + return EFI_BUFFER_TOO_SMALL; > + } > > CopyMem (&Dst->Dhcp4, &Src->Dhcp4, Src->Length); > Dst->Length = Src->Length; > + return EFI_SUCCESS; > } > > > @@ -93,8 +99,11 @@ PxeBcCopyEfiDhcp4Packet ( > @param OfferIndex Index of cached packets as complements of pxe > mode data, > the index is maximum offer number. > > + @retval EFI_SUCCESS Cache and parse the packet successfully. > + @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; > > ASSERT (OfferIndex < Private->NumOffers); > ASSERT (OfferIndex < PXEBC_MAX_OFFER_NUM); @@ -109,11 +119,15 > @@ PxeBcCopyProxyOffer ( > Mode = Private->PxeBc.Mode; > Offer = &Private->Dhcp4Offers[OfferIndex].Packet.Offer; > > - PxeBcCopyEfiDhcp4Packet (&Private->ProxyOffer.Packet.Offer, Offer); > + Status = PxeBcCopyEfiDhcp4Packet (&Private->ProxyOffer.Packet.Offer, > + Offer); if (EFI_ERROR(Status)) { > + return Status; > + } > CopyMem (&Mode->ProxyOffer, &Offer->Dhcp4, Offer->Length); > Mode->ProxyOfferReceived = TRUE; > > PxeBcParseCachedDhcpPacket (&Private->ProxyOffer); > + return EFI_SUCCESS; > } > > > @@ -411,8 +425,9 @@ PxeBcTryBinlProxy ( > > @param Private Pointer to PxeBc private data. > > - @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 base > code mode. > > **/ > EFI_STATUS > @@ -515,7 +530,7 @@ PxeBcCheckSelectedOffer ( > // > // Copy the proxy offer to Mode and set the flag > // > - PxeBcCopyProxyOffer (Private, ProxyOfferIndex); > + Status = PxeBcCopyProxyOffer (Private, ProxyOfferIndex); > } > } else { > // > @@ -543,7 +558,7 @@ PxeBcCheckSelectedOffer ( > // Other type of ACK is already cached. Bootp is special that we should > // use the bootp reply as the ACK and put it into the DHCP_ONLY buffer. > // > - PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, Offer); > + Status = PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, > + Offer); > } > > 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. > > + @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; > > CachedOffer = &Private->Dhcp4Offers[Private->NumOffers]; > Offer = &CachedOffer->Packet.Offer; > @@ -583,18 +602,21 @@ PxeBcCacheDhcpOffer ( > // > // Cache the orignal dhcp packet > // > - PxeBcCopyEfiDhcp4Packet (Offer, RcvdOffer); > + Status = PxeBcCopyEfiDhcp4Packet (Offer, RcvdOffer); if > + (EFI_ERROR(Status)) { > + return Status; > + } > > // > // Parse and validate the options (including dhcp option and vendor option) > // > if (!PxeBcParseCachedDhcpPacket (CachedOffer)) { > - return ; > + return EFI_ABORTED; > } > > OfferType = CachedOffer->OfferType; > if (OfferType >= DHCP4_PACKET_TYPE_MAX) { > - return ; > + return EFI_ABORTED; > } > > if (OfferType == 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 others. > // > - return ; > + return EFI_ABORTED; > } else { > // > // Record index of the proxy dhcp offer with type other than binl. > @@ -649,6 +671,8 @@ PxeBcCacheDhcpOffer ( > // Count the accepted offers. > // > Private->NumOffers++; > + > + return EFI_SUCCESS; > } > > /** > @@ -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; > > case Dhcp4RcvdAck: > - if (Packet->Length > PXEBC_DHCP4_MAX_PACKET_SIZE) { > - // > - // Abort the DHCP if the ACK packet exceeds the maximum length. > - // > - Status = EFI_ABORTED; > - break; > - } > - > // > // Cache Ack > // > ASSERT (Private->SelectedOffer != 0); > > - PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, Packet); > + Status = PxeBcCopyEfiDhcp4Packet (&Private->Dhcp4Ack.Packet.Ack, > Packet); > + if (EFI_ERROR (Status)) { > + return EFI_ABORTED; > + } > break; > > default: > @@ -1340,6 +1360,12 @@ PxeBcDiscvBootService ( > Response = Token.ResponseList; > > while (RepIndex < Token.ResponseCount) { > + if (Response->Length > PXEBC_DHCP4_MAX_PACKET_SIZE) { > + SrvIndex = 0; > + RepIndex++; > + Response = (EFI_DHCP4_PACKET *) ((UINT8 *) Response + Response- > >Size); > + continue; > + } > > while (SrvIndex < IpCount) { > > @@ -1360,14 +1386,16 @@ PxeBcDiscvBootService ( > > SrvIndex = 0; > RepIndex++; > - > Response = (EFI_DHCP4_PACKET *) ((UINT8 *) Response + Response- > >Size); > } > > if (RepIndex < Token.ResponseCount) { > > if (Reply != NULL) { > - PxeBcCopyEfiDhcp4Packet (Reply, Response); > + Status = PxeBcCopyEfiDhcp4Packet (Reply, Response); > + if (EFI_ERROR(Status)) { > + goto ON_EXIT; > + } > } > > if (IsDiscv) { > @@ -1380,18 +1408,21 @@ PxeBcDiscvBootService ( > } else { > Status = EFI_NOT_FOUND; > } > + } > > - // > - // free the responselist > - // > - if (Token.ResponseList != NULL) { > - FreePool (Token.ResponseList); > - } > +ON_EXIT: > + // > + // free the responselist > + // > + if (Token.ResponseList != NULL) { > + FreePool (Token.ResponseList); > } > // > // Free the dhcp packet > // > - FreePool (Token.Packet); > + if (Token.Packet != NULL) { > + FreePool (Token.Packet); > + } > > 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 ( > > @param Private Pointer to PxeBc private data. > > - @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 base > code mode. > > **/ > EFI_STATUS > -- > 2.7.4.windows.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Patch 2/2] NetworkPkg: Replace ASSERT with error return code in PXE and HTTP boot driver. 2016-12-16 8:18 [Patch 0/2] Replace ASSERT with error return code in PXE and HTTP boot Fu Siyuan 2016-12-16 8:18 ` [Patch 1/2] MdeModulePkg: Replace ASSERT with error return code in PXE driver Fu Siyuan @ 2016-12-16 8:18 ` Fu Siyuan 2016-12-16 8:57 ` Wu, Jiaxin 2016-12-16 8:45 ` [Patch 0/2] Replace ASSERT with error return code in PXE and HTTP boot Ye, Ting 2 siblings, 1 reply; 6+ messages in thread From: Fu Siyuan @ 2016-12-16 8:18 UTC (permalink / raw) To: edk2-devel; +Cc: Yao Jiewen, Ye Ting, Wu Jiaxin This patch remove the ASSERT when receive a DHCP packet large than the maximum cache buffer size. Cc: Yao Jiewen <jiewen.yao@intel.com> Cc: Ye Ting <ting.ye@intel.com> Cc: Wu Jiaxin <jiaxin.wu@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Fu Siyuan <siyuan.fu@intel.com> --- NetworkPkg/HttpBootDxe/HttpBootDhcp4.c | 32 +++++++-- NetworkPkg/HttpBootDxe/HttpBootDhcp6.c | 29 ++++++-- NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c | 119 +++++++++++++++++++++++---------- NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c | 67 +++++++++++++------ 4 files changed, 181 insertions(+), 66 deletions(-) diff --git a/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c b/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c index a47a8f4..fcea916 100644 --- a/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c +++ b/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c @@ -220,17 +220,24 @@ HttpBootParseDhcp4Options ( @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. + **/ -VOID +EFI_STATUS HttpBootCacheDhcp4Packet ( IN EFI_DHCP4_PACKET *Dst, IN EFI_DHCP4_PACKET *Src ) { - ASSERT (Dst->Size >= Src->Length); + if (Dst->Size < Src->Length) { + return EFI_BUFFER_TOO_SMALL; + } CopyMem (&Dst->Dhcp4, &Src->Dhcp4, Src->Length); Dst->Length = Src->Length; + + return EFI_SUCCESS; } /** @@ -429,8 +436,10 @@ HttpBootParseDhcp4Packet ( @param[in] Private Pointer to HTTP boot driver private data. @param[in] RcvdOffer Pointer to the received offer packet. + @retval EFI_SUCCESS Cache and parse the packet successfully. + @retval Others Operation failed. **/ -VOID +EFI_STATUS HttpBootCacheDhcp4Offer ( IN HTTP_BOOT_PRIVATE_DATA *Private, IN EFI_DHCP4_PACKET *RcvdOffer @@ -439,6 +448,7 @@ HttpBootCacheDhcp4Offer ( HTTP_BOOT_DHCP4_PACKET_CACHE *Cache4; EFI_DHCP4_PACKET *Offer; HTTP_BOOT_OFFER_TYPE OfferType; + EFI_STATUS Status; ASSERT (Private->OfferNum < HTTP_BOOT_OFFER_MAX_NUM); Cache4 = &Private->OfferBuffer[Private->OfferNum].Dhcp4; @@ -447,13 +457,16 @@ HttpBootCacheDhcp4Offer ( // // Cache the content of DHCPv4 packet firstly. // - HttpBootCacheDhcp4Packet (Offer, RcvdOffer); + Status = HttpBootCacheDhcp4Packet (Offer, RcvdOffer); + if (EFI_ERROR (Status)) { + return Status; + } // // Validate the DHCPv4 packet, and parse the options and offer type. // if (EFI_ERROR (HttpBootParseDhcp4Packet (Cache4))) { - return; + return EFI_ABORTED; } // @@ -465,6 +478,8 @@ HttpBootCacheDhcp4Offer ( Private->OfferIndex[OfferType][Private->OfferCount[OfferType]] = Private->OfferNum; Private->OfferCount[OfferType]++; Private->OfferNum++; + + return EFI_SUCCESS; } /** @@ -618,10 +633,17 @@ HttpBootDhcp4CallBack ( switch (Dhcp4Event) { case Dhcp4RcvdOffer: Status = EFI_NOT_READY; + if (Packet->Length > HTTP_BOOT_DHCP4_PACKET_MAX_SIZE) { + // + // Ignore the incoming packets which exceed the maximum length. + // + break; + } if (Private->OfferNum < HTTP_BOOT_OFFER_MAX_NUM) { // // Cache the DHCPv4 offers to OfferBuffer[] for select later, and record // the OfferIndex and OfferCount. + // If error happens, just ignore this packet and continue to wait more offer. // HttpBootCacheDhcp4Offer (Private, Packet); } diff --git a/NetworkPkg/HttpBootDxe/HttpBootDhcp6.c b/NetworkPkg/HttpBootDxe/HttpBootDhcp6.c index ca84f2a..f2b8195 100644 --- a/NetworkPkg/HttpBootDxe/HttpBootDhcp6.c +++ b/NetworkPkg/HttpBootDxe/HttpBootDhcp6.c @@ -329,17 +329,24 @@ HttpBootParseDhcp6Packet ( @param[in] Dst The pointer to the cache buffer for DHCPv6 packet. @param[in] Src The pointer to the DHCPv6 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. + **/ -VOID +EFI_STATUS HttpBootCacheDhcp6Packet ( IN EFI_DHCP6_PACKET *Dst, IN EFI_DHCP6_PACKET *Src ) { - ASSERT (Dst->Size >= Src->Length); + if (Dst->Size < Src->Length) { + return EFI_BUFFER_TOO_SMALL; + } CopyMem (&Dst->Dhcp6, &Src->Dhcp6, Src->Length); Dst->Length = Src->Length; + + return EFI_SUCCESS; } /** @@ -348,8 +355,11 @@ HttpBootCacheDhcp6Packet ( @param[in] Private The pointer to HTTP_BOOT_PRIVATE_DATA. @param[in] RcvdOffer The pointer to the received offer packet. + @retval EFI_SUCCESS Cache and parse the packet successfully. + @retval Others Operation failed. + **/ -VOID +EFI_STATUS HttpBootCacheDhcp6Offer ( IN HTTP_BOOT_PRIVATE_DATA *Private, IN EFI_DHCP6_PACKET *RcvdOffer @@ -358,6 +368,7 @@ HttpBootCacheDhcp6Offer ( HTTP_BOOT_DHCP6_PACKET_CACHE *Cache6; EFI_DHCP6_PACKET *Offer; HTTP_BOOT_OFFER_TYPE OfferType; + EFI_STATUS Status; Cache6 = &Private->OfferBuffer[Private->OfferNum].Dhcp6; Offer = &Cache6->Packet.Offer; @@ -365,13 +376,16 @@ HttpBootCacheDhcp6Offer ( // // Cache the content of DHCPv6 packet firstly. // - HttpBootCacheDhcp6Packet(Offer, RcvdOffer); + Status = HttpBootCacheDhcp6Packet(Offer, RcvdOffer); + if (EFI_ERROR (Status)) { + return Status; + } // // Validate the DHCPv6 packet, and parse the options and offer type. // if (EFI_ERROR (HttpBootParseDhcp6Packet (Cache6))) { - return ; + return EFI_ABORTED; } // @@ -382,7 +396,9 @@ HttpBootCacheDhcp6Offer ( ASSERT (Private->OfferCount[OfferType] < HTTP_BOOT_OFFER_MAX_NUM); Private->OfferIndex[OfferType][Private->OfferCount[OfferType]] = Private->OfferNum; Private->OfferCount[OfferType]++; - Private->OfferNum++; + Private->OfferNum++; + + return EFI_SUCCESS; } /** @@ -437,6 +453,7 @@ HttpBootDhcp6CallBack ( // // Cache the dhcp offers to OfferBuffer[] for select later, and record // the OfferIndex and OfferCount. + // If error happens, just ignore this packet and continue to wait more offer. // HttpBootCacheDhcp6Offer (Private, Packet); } diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c index 44b0714..5497390 100644 --- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c @@ -424,17 +424,24 @@ PxeBcSeedDhcp4Packet ( @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. + **/ -VOID +EFI_STATUS PxeBcCacheDhcp4Packet ( IN EFI_DHCP4_PACKET *Dst, IN EFI_DHCP4_PACKET *Src ) { - ASSERT (Dst->Size >= Src->Length); - + if (Dst->Size < Src->Length) { + return EFI_BUFFER_TOO_SMALL; + } + CopyMem (&Dst->Dhcp4, &Src->Dhcp4, Src->Length); Dst->Length = Src->Length; + + return EFI_SUCCESS; } @@ -620,8 +627,11 @@ PxeBcParseDhcp4Packet ( @param[in] Ack Pointer to the DHCPv4 ack packet. @param[in] Verified If TRUE, parse the ACK packet and store info into mode data. + @retval EFI_SUCCESS Cache and parse the packet successfully. + @retval EFI_BUFFER_TOO_SMALL Cache buffer is not big enough to hold the packet. + **/ -VOID +EFI_STATUS PxeBcCopyDhcp4Ack ( IN PXEBC_PRIVATE_DATA *Private, IN EFI_DHCP4_PACKET *Ack, @@ -629,10 +639,14 @@ PxeBcCopyDhcp4Ack ( ) { EFI_PXE_BASE_CODE_MODE *Mode; + EFI_STATUS Status; Mode = Private->PxeBc.Mode; - PxeBcCacheDhcp4Packet (&Private->DhcpAck.Dhcp4.Packet.Ack, Ack); + Status = PxeBcCacheDhcp4Packet (&Private->DhcpAck.Dhcp4.Packet.Ack, Ack); + if (EFI_ERROR (Status)) { + return Status; + } if (Verified) { // @@ -642,6 +656,8 @@ PxeBcCopyDhcp4Ack ( CopyMem (&Mode->DhcpAck.Dhcpv4, &Ack->Dhcp4, Ack->Length); Mode->DhcpAckReceived = TRUE; } + + return EFI_SUCCESS; } @@ -651,8 +667,11 @@ PxeBcCopyDhcp4Ack ( @param[in] Private Pointer to PxeBc private data. @param[in] OfferIndex The received order of offer packets. + @retval EFI_SUCCESS Cache and parse the packet successfully. + @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 @@ -660,6 +679,7 @@ PxeBcCopyProxyOffer ( { EFI_PXE_BASE_CODE_MODE *Mode; EFI_DHCP4_PACKET *Offer; + EFI_STATUS Status; ASSERT (OfferIndex < Private->OfferNum); ASSERT (OfferIndex < PXEBC_OFFER_MAX_NUM); @@ -670,7 +690,11 @@ PxeBcCopyProxyOffer ( // // Cache the proxy offer packet and parse it. // - PxeBcCacheDhcp4Packet (&Private->ProxyOffer.Dhcp4.Packet.Offer, Offer); + Status = PxeBcCacheDhcp4Packet (&Private->ProxyOffer.Dhcp4.Packet.Offer, Offer); + if (EFI_ERROR(Status)) { + return Status; + } + PxeBcParseDhcp4Packet (&Private->ProxyOffer.Dhcp4); // @@ -678,6 +702,8 @@ PxeBcCopyProxyOffer ( // CopyMem (&Mode->ProxyOffer.Dhcpv4, &Offer->Dhcp4, Offer->Length); Mode->ProxyOfferReceived = TRUE; + + return EFI_SUCCESS; } @@ -780,8 +806,11 @@ PxeBcRetryBinlOffer ( @param[in] Private Pointer to PxeBc private data. @param[in] RcvdOffer Pointer to the received offer packet. + @retval EFI_SUCCESS Cache and parse the packet successfully. + @retval Others Operation failed. + **/ -VOID +EFI_STATUS PxeBcCacheDhcp4Offer ( IN PXEBC_PRIVATE_DATA *Private, IN EFI_DHCP4_PACKET *RcvdOffer @@ -790,6 +819,7 @@ PxeBcCacheDhcp4Offer ( PXEBC_DHCP4_PACKET_CACHE *Cache4; EFI_DHCP4_PACKET *Offer; PXEBC_OFFER_TYPE OfferType; + EFI_STATUS Status; ASSERT (Private->OfferNum < PXEBC_OFFER_MAX_NUM); Cache4 = &Private->OfferBuffer[Private->OfferNum].Dhcp4; @@ -798,13 +828,16 @@ PxeBcCacheDhcp4Offer ( // // Cache the content of DHCPv4 packet firstly. // - PxeBcCacheDhcp4Packet (Offer, RcvdOffer); + Status = PxeBcCacheDhcp4Packet (Offer, RcvdOffer); + if (EFI_ERROR(Status)) { + return Status; + } // // Validate the DHCPv4 packet, and parse the options and offer type. // if (EFI_ERROR (PxeBcParseDhcp4Packet (Cache4))) { - return; + return EFI_ABORTED; } // @@ -821,7 +854,7 @@ PxeBcCacheDhcp4Offer ( Private->OfferIndex[OfferType][0] = Private->OfferNum; Private->OfferCount[OfferType] = 1; } else { - return; + return EFI_ABORTED; } } else { ASSERT (Private->OfferCount[OfferType] < PXEBC_OFFER_MAX_NUM); @@ -844,7 +877,7 @@ PxeBcCacheDhcp4Offer ( Private->OfferIndex[OfferType][0] = Private->OfferNum; Private->OfferCount[OfferType] = 1; } else { - return ; + return EFI_ABORTED; } } else { // @@ -856,6 +889,8 @@ PxeBcCacheDhcp4Offer ( } Private->OfferNum++; + + return EFI_SUCCESS; } @@ -980,11 +1015,12 @@ PxeBcSelectDhcp4Offer ( /** Handle the DHCPv4 offer packet. - @param[in] Private Pointer to PxeBc private data. + @param[in] Private Pointer to PxeBc private data. - @retval EFI_SUCCESS Handled the DHCPv4 offer packet successfully. - @retval EFI_NO_RESPONSE No response to the following request packet. - @retval EFI_NOT_FOUND No boot filename received. + @retval EFI_SUCCESS Handled the DHCPv4 offer packet successfully. + @retval EFI_NO_RESPONSE No response to the following request packet. + @retval EFI_NOT_FOUND No boot filename received. + @retval EFI_BUFFER_TOO_SMALL Can't cache the offer pacet. **/ EFI_STATUS @@ -1089,7 +1125,7 @@ PxeBcHandleDhcp4Offer ( // // Success to try to request by a ProxyPxe10 or ProxyWfm11a offer, copy and parse it. // - PxeBcCopyProxyOffer (Private, ProxyIndex); + Status = PxeBcCopyProxyOffer (Private, ProxyIndex); } } else { // @@ -1116,7 +1152,10 @@ PxeBcHandleDhcp4Offer ( Ack = Offer; } - PxeBcCopyDhcp4Ack (Private, Ack, TRUE); + Status = PxeBcCopyDhcp4Ack (Private, Ack, TRUE); + if (EFI_ERROR (Status)) { + return Status; + } Mode->DhcpDiscoverValid = TRUE; } @@ -1258,6 +1297,7 @@ PxeBcDhcp4CallBack ( // // Cache the DHCPv4 offers to OfferBuffer[] for select later, and record // the OfferIndex and OfferCount. + // If error happens, just ignore this packet and continue to wait more offer. // PxeBcCacheDhcp4Offer (Private, Packet); } @@ -1278,21 +1318,16 @@ PxeBcDhcp4CallBack ( break; case Dhcp4RcvdAck: - if (Packet->Length > PXEBC_DHCP4_PACKET_MAX_SIZE) { - // - // Abort the DHCP if the ACK packet exceeds the maximum length. - // - Status = EFI_ABORTED; - break; - } - // // Cache the DHCPv4 ack to Private->Dhcp4Ack, but it's not the final ack in mode data // without verification. // ASSERT (Private->SelectIndex != 0); - PxeBcCopyDhcp4Ack (Private, Packet, FALSE); + Status = PxeBcCopyDhcp4Ack (Private, Packet, FALSE); + if (EFI_ERROR (Status)) { + Status = EFI_ABORTED; + } break; default: @@ -1491,6 +1526,12 @@ PxeBcDhcp4Discover ( // Find the right PXE Reply according to server address. // while (RepIndex < Token.ResponseCount) { + if (Response->Length > PXEBC_DHCP4_PACKET_MAX_SIZE) { + SrvIndex = 0; + RepIndex++; + Response = (EFI_DHCP4_PACKET *) ((UINT8 *) Response + Response->Size); + continue; + } while (SrvIndex < IpCount) { if (SrvList[SrvIndex].AcceptAnyResponse) { @@ -1509,7 +1550,6 @@ PxeBcDhcp4Discover ( SrvIndex = 0; RepIndex++; - Response = (EFI_DHCP4_PACKET *) ((UINT8 *) Response + Response->Size); } @@ -1519,10 +1559,16 @@ PxeBcDhcp4Discover ( // Especially for PXE discover packet, store it into mode data here. // if (Private->IsDoDiscover) { - PxeBcCacheDhcp4Packet (&Private->PxeReply.Dhcp4.Packet.Ack, Response); + Status = PxeBcCacheDhcp4Packet (&Private->PxeReply.Dhcp4.Packet.Ack, Response); + if (EFI_ERROR(Status)) { + goto ON_EXIT; + } CopyMem (&Mode->PxeDiscover, &Token.Packet->Dhcp4, Token.Packet->Length); } else { - PxeBcCacheDhcp4Packet (&Private->ProxyOffer.Dhcp4.Packet.Offer, Response); + Status = PxeBcCacheDhcp4Packet (&Private->ProxyOffer.Dhcp4.Packet.Offer, Response); + if (EFI_ERROR(Status)) { + goto ON_EXIT; + } } } else { // @@ -1530,12 +1576,15 @@ PxeBcDhcp4Discover ( // Status = EFI_NOT_FOUND; } - if (Token.ResponseList != NULL) { - FreePool (Token.ResponseList); - } } - - FreePool (Token.Packet); +ON_EXIT: + + if (Token.ResponseList != NULL) { + FreePool (Token.ResponseList); + } + if (Token.Packet != NULL) { + FreePool (Token.Packet); + } return Status; } diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c index 6a08e9a..3672f13 100644 --- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c @@ -181,17 +181,24 @@ PxeBcBuildDhcp6Options ( @param[in] Dst The pointer to the cache buffer for DHCPv6 packet. @param[in] Src The pointer to the DHCPv6 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. + **/ -VOID +EFI_STATUS PxeBcCacheDhcp6Packet ( IN EFI_DHCP6_PACKET *Dst, IN EFI_DHCP6_PACKET *Src ) { - ASSERT (Dst->Size >= Src->Length); + if (Dst->Size < Src->Length) { + return EFI_BUFFER_TOO_SMALL; + } CopyMem (&Dst->Dhcp6, &Src->Dhcp6, Src->Length); Dst->Length = Src->Length; + + return EFI_SUCCESS; } @@ -749,8 +756,11 @@ PxeBcParseDhcp6Packet ( @param[in] Ack The pointer to the DHCPv6 ack packet. @param[in] Verified If TRUE, parse the ACK packet and store info into mode data. + @retval EFI_SUCCESS Cache and parse the packet successfully. + @retval EFI_BUFFER_TOO_SMALL Cache buffer is not big enough to hold the packet. + **/ -VOID +EFI_STATUS PxeBcCopyDhcp6Ack ( IN PXEBC_PRIVATE_DATA *Private, IN EFI_DHCP6_PACKET *Ack, @@ -758,10 +768,14 @@ PxeBcCopyDhcp6Ack ( ) { EFI_PXE_BASE_CODE_MODE *Mode; + EFI_STATUS Status; Mode = Private->PxeBc.Mode; - PxeBcCacheDhcp6Packet (&Private->DhcpAck.Dhcp6.Packet.Ack, Ack); + Status = PxeBcCacheDhcp6Packet (&Private->DhcpAck.Dhcp6.Packet.Ack, Ack); + if (EFI_ERROR (Status)) { + return Status; + } if (Verified) { // @@ -771,6 +785,8 @@ PxeBcCopyDhcp6Ack ( CopyMem (&Mode->DhcpAck.Dhcpv6, &Ack->Dhcp6, Ack->Length); Mode->DhcpAckReceived = TRUE; } + + return EFI_SUCCESS; } @@ -780,8 +796,11 @@ PxeBcCopyDhcp6Ack ( @param[in] Private The pointer to PxeBc private data. @param[in] OfferIndex The received order of offer packets. + @retval EFI_SUCCESS Cache and parse the packet successfully. + @retval EFI_BUFFER_TOO_SMALL Cache buffer is not big enough to hold the packet. + **/ -VOID +EFI_STATUS PxeBcCopyDhcp6Proxy ( IN PXEBC_PRIVATE_DATA *Private, IN UINT32 OfferIndex @@ -789,6 +808,7 @@ PxeBcCopyDhcp6Proxy ( { EFI_PXE_BASE_CODE_MODE *Mode; EFI_DHCP6_PACKET *Offer; + EFI_STATUS Status; ASSERT (OfferIndex < Private->OfferNum); ASSERT (OfferIndex < PXEBC_OFFER_MAX_NUM); @@ -799,7 +819,10 @@ PxeBcCopyDhcp6Proxy ( // // Cache the proxy offer packet and parse it. // - PxeBcCacheDhcp6Packet (&Private->ProxyOffer.Dhcp6.Packet.Offer, Offer); + Status = PxeBcCacheDhcp6Packet (&Private->ProxyOffer.Dhcp6.Packet.Offer, Offer); + if (EFI_ERROR(Status)) { + return Status; + } PxeBcParseDhcp6Packet (&Private->ProxyOffer.Dhcp6); // @@ -807,6 +830,8 @@ PxeBcCopyDhcp6Proxy ( // CopyMem (&Mode->ProxyOffer.Dhcpv6, &Offer->Dhcp6, Offer->Length); Mode->ProxyOfferReceived = TRUE; + + return EFI_SUCCESS; } /** @@ -1121,8 +1146,10 @@ PxeBcRetryDhcp6Binl ( @param[in] Private The pointer to PXEBC_PRIVATE_DATA. @param[in] RcvdOffer The pointer to the received offer packet. + @retval EFI_SUCCESS Cache and parse the packet successfully. + @retval Others Operation failed. **/ -VOID +EFI_STATUS PxeBcCacheDhcp6Offer ( IN PXEBC_PRIVATE_DATA *Private, IN EFI_DHCP6_PACKET *RcvdOffer @@ -1131,6 +1158,7 @@ PxeBcCacheDhcp6Offer ( PXEBC_DHCP6_PACKET_CACHE *Cache6; EFI_DHCP6_PACKET *Offer; PXEBC_OFFER_TYPE OfferType; + EFI_STATUS Status; Cache6 = &Private->OfferBuffer[Private->OfferNum].Dhcp6; Offer = &Cache6->Packet.Offer; @@ -1138,13 +1166,13 @@ PxeBcCacheDhcp6Offer ( // // Cache the content of DHCPv6 packet firstly. // - PxeBcCacheDhcp6Packet (Offer, RcvdOffer); + Status = PxeBcCacheDhcp6Packet (Offer, RcvdOffer); // // Validate the DHCPv6 packet, and parse the options and offer type. // if (EFI_ERROR (PxeBcParseDhcp6Packet (Cache6))) { - return ; + return EFI_ABORTED; } // @@ -1173,7 +1201,7 @@ PxeBcCacheDhcp6Offer ( Private->OfferIndex[OfferType][0] = Private->OfferNum; Private->OfferCount[OfferType] = 1; } else { - return; + return EFI_ABORTED; } } else { // @@ -1184,6 +1212,8 @@ PxeBcCacheDhcp6Offer ( } Private->OfferNum++; + + return EFI_SUCCESS; } @@ -1301,6 +1331,7 @@ PxeBcSelectDhcp6Offer ( @retval EFI_SUCCESS Handled the DHCPv6 offer packet successfully. @retval EFI_NO_RESPONSE No response to the following request packet. @retval EFI_OUT_OF_RESOURCES Failed to allocate resources. + @retval EFI_BUFFER_TOO_SMALL Can't cache the offer pacet. **/ EFI_STATUS @@ -1410,7 +1441,7 @@ PxeBcHandleDhcp6Offer ( // // Success to try to request by a ProxyPxe10 or ProxyWfm11a offer, copy and parse it. // - PxeBcCopyDhcp6Proxy (Private, ProxyIndex); + Status = PxeBcCopyDhcp6Proxy (Private, ProxyIndex); } } else { // @@ -1424,7 +1455,7 @@ PxeBcHandleDhcp6Offer ( // // All PXE boot information is ready by now. // - PxeBcCopyDhcp6Ack (Private, &Private->DhcpAck.Dhcp6.Packet.Ack, TRUE); + Status = PxeBcCopyDhcp6Ack (Private, &Private->DhcpAck.Dhcp6.Packet.Ack, TRUE); Private->PxeBc.Mode->DhcpDiscoverValid = TRUE; } @@ -1997,19 +2028,15 @@ PxeBcDhcp6CallBack ( break; case Dhcp6RcvdReply: - if (Packet->Length > PXEBC_DHCP6_PACKET_MAX_SIZE) { - // - // Abort the DHCP if the Peply packet exceeds the maximum length. - // - Status = EFI_ABORTED; - break; - } // // Cache the dhcp ack to Private->Dhcp6Ack, but it's not the final ack in mode data // without verification. // ASSERT (Private->SelectIndex != 0); - PxeBcCopyDhcp6Ack (Private, Packet, FALSE); + Status = PxeBcCopyDhcp6Ack (Private, Packet, FALSE); + if (EFI_ERROR (Status)) { + Status = EFI_ABORTED; + } break; default: -- 2.7.4.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch 2/2] NetworkPkg: Replace ASSERT with error return code in PXE and HTTP boot driver. 2016-12-16 8:18 ` [Patch 2/2] NetworkPkg: Replace ASSERT with error return code in PXE and HTTP boot driver Fu Siyuan @ 2016-12-16 8:57 ` Wu, Jiaxin 0 siblings, 0 replies; 6+ messages in thread From: Wu, Jiaxin @ 2016-12-16 8:57 UTC (permalink / raw) To: Fu, Siyuan, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Ye, Ting Hi siyuan, In PxeBcCacheDhcp6Offer() function, 'Status' is set but not used, this may break the GCC build. Please double check that. Others good to me. Reviewed-by: Wu Jiaxin <jiaxin.wu@intel.com> Thanks, Jiaxin > -----Original Message----- > From: Fu, Siyuan > Sent: Friday, December 16, 2016 4:19 PM > To: edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ye, Ting <ting.ye@intel.com>; Wu, > Jiaxin <jiaxin.wu@intel.com> > Subject: [Patch 2/2] NetworkPkg: Replace ASSERT with error return code in > PXE and HTTP boot driver. > > This patch remove the ASSERT when receive a DHCP packet large than the > maximum cache buffer size. > > Cc: Yao Jiewen <jiewen.yao@intel.com> > Cc: Ye Ting <ting.ye@intel.com> > Cc: Wu Jiaxin <jiaxin.wu@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Fu Siyuan <siyuan.fu@intel.com> > --- > NetworkPkg/HttpBootDxe/HttpBootDhcp4.c | 32 +++++++-- > NetworkPkg/HttpBootDxe/HttpBootDhcp6.c | 29 ++++++-- > NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c | 119 > +++++++++++++++++++++++---------- > NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c | 67 +++++++++++++------ > 4 files changed, 181 insertions(+), 66 deletions(-) > > diff --git a/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c > b/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c > index a47a8f4..fcea916 100644 > --- a/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c > +++ b/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c > @@ -220,17 +220,24 @@ HttpBootParseDhcp4Options ( > @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. > + > **/ > -VOID > +EFI_STATUS > HttpBootCacheDhcp4Packet ( > IN EFI_DHCP4_PACKET *Dst, > IN EFI_DHCP4_PACKET *Src > ) > { > - ASSERT (Dst->Size >= Src->Length); > + if (Dst->Size < Src->Length) { > + return EFI_BUFFER_TOO_SMALL; > + } > > CopyMem (&Dst->Dhcp4, &Src->Dhcp4, Src->Length); > Dst->Length = Src->Length; > + > + return EFI_SUCCESS; > } > > /** > @@ -429,8 +436,10 @@ HttpBootParseDhcp4Packet ( > @param[in] Private Pointer to HTTP boot driver private data. > @param[in] RcvdOffer Pointer to the received offer packet. > > + @retval EFI_SUCCESS Cache and parse the packet successfully. > + @retval Others Operation failed. > **/ > -VOID > +EFI_STATUS > HttpBootCacheDhcp4Offer ( > IN HTTP_BOOT_PRIVATE_DATA *Private, > IN EFI_DHCP4_PACKET *RcvdOffer > @@ -439,6 +448,7 @@ HttpBootCacheDhcp4Offer ( > HTTP_BOOT_DHCP4_PACKET_CACHE *Cache4; > EFI_DHCP4_PACKET *Offer; > HTTP_BOOT_OFFER_TYPE OfferType; > + EFI_STATUS Status; > > ASSERT (Private->OfferNum < HTTP_BOOT_OFFER_MAX_NUM); > Cache4 = &Private->OfferBuffer[Private->OfferNum].Dhcp4; > @@ -447,13 +457,16 @@ HttpBootCacheDhcp4Offer ( > // > // Cache the content of DHCPv4 packet firstly. > // > - HttpBootCacheDhcp4Packet (Offer, RcvdOffer); > + Status = HttpBootCacheDhcp4Packet (Offer, RcvdOffer); if (EFI_ERROR > + (Status)) { > + return Status; > + } > > // > // Validate the DHCPv4 packet, and parse the options and offer type. > // > if (EFI_ERROR (HttpBootParseDhcp4Packet (Cache4))) { > - return; > + return EFI_ABORTED; > } > > // > @@ -465,6 +478,8 @@ HttpBootCacheDhcp4Offer ( > Private->OfferIndex[OfferType][Private->OfferCount[OfferType]] = > Private->OfferNum; > Private->OfferCount[OfferType]++; > Private->OfferNum++; > + > + return EFI_SUCCESS; > } > > /** > @@ -618,10 +633,17 @@ HttpBootDhcp4CallBack ( > switch (Dhcp4Event) { > case Dhcp4RcvdOffer: > Status = EFI_NOT_READY; > + if (Packet->Length > HTTP_BOOT_DHCP4_PACKET_MAX_SIZE) { > + // > + // Ignore the incoming packets which exceed the maximum length. > + // > + break; > + } > if (Private->OfferNum < HTTP_BOOT_OFFER_MAX_NUM) { > // > // Cache the DHCPv4 offers to OfferBuffer[] for select later, and record > // the OfferIndex and OfferCount. > + // If error happens, just ignore this packet and continue to wait more > offer. > // > HttpBootCacheDhcp4Offer (Private, Packet); > } > diff --git a/NetworkPkg/HttpBootDxe/HttpBootDhcp6.c > b/NetworkPkg/HttpBootDxe/HttpBootDhcp6.c > index ca84f2a..f2b8195 100644 > --- a/NetworkPkg/HttpBootDxe/HttpBootDhcp6.c > +++ b/NetworkPkg/HttpBootDxe/HttpBootDhcp6.c > @@ -329,17 +329,24 @@ HttpBootParseDhcp6Packet ( > @param[in] Dst The pointer to the cache buffer for DHCPv6 packet. > @param[in] Src The pointer to the DHCPv6 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. > + > **/ > -VOID > +EFI_STATUS > HttpBootCacheDhcp6Packet ( > IN EFI_DHCP6_PACKET *Dst, > IN EFI_DHCP6_PACKET *Src > ) > { > - ASSERT (Dst->Size >= Src->Length); > + if (Dst->Size < Src->Length) { > + return EFI_BUFFER_TOO_SMALL; > + } > > CopyMem (&Dst->Dhcp6, &Src->Dhcp6, Src->Length); > Dst->Length = Src->Length; > + > + return EFI_SUCCESS; > } > > /** > @@ -348,8 +355,11 @@ HttpBootCacheDhcp6Packet ( > @param[in] Private The pointer to HTTP_BOOT_PRIVATE_DATA. > @param[in] RcvdOffer The pointer to the received offer packet. > > + @retval EFI_SUCCESS Cache and parse the packet successfully. > + @retval Others Operation failed. > + > **/ > -VOID > +EFI_STATUS > HttpBootCacheDhcp6Offer ( > IN HTTP_BOOT_PRIVATE_DATA *Private, > IN EFI_DHCP6_PACKET *RcvdOffer > @@ -358,6 +368,7 @@ HttpBootCacheDhcp6Offer ( > HTTP_BOOT_DHCP6_PACKET_CACHE *Cache6; > EFI_DHCP6_PACKET *Offer; > HTTP_BOOT_OFFER_TYPE OfferType; > + EFI_STATUS Status; > > Cache6 = &Private->OfferBuffer[Private->OfferNum].Dhcp6; > Offer = &Cache6->Packet.Offer; > @@ -365,13 +376,16 @@ HttpBootCacheDhcp6Offer ( > // > // Cache the content of DHCPv6 packet firstly. > // > - HttpBootCacheDhcp6Packet(Offer, RcvdOffer); > + Status = HttpBootCacheDhcp6Packet(Offer, RcvdOffer); if (EFI_ERROR > + (Status)) { > + return Status; > + } > > // > // Validate the DHCPv6 packet, and parse the options and offer type. > // > if (EFI_ERROR (HttpBootParseDhcp6Packet (Cache6))) { > - return ; > + return EFI_ABORTED; > } > > // > @@ -382,7 +396,9 @@ HttpBootCacheDhcp6Offer ( > ASSERT (Private->OfferCount[OfferType] < > HTTP_BOOT_OFFER_MAX_NUM); > Private->OfferIndex[OfferType][Private->OfferCount[OfferType]] = > Private->OfferNum; > Private->OfferCount[OfferType]++; > - Private->OfferNum++; > + Private->OfferNum++; > + > + return EFI_SUCCESS; > } > > /** > @@ -437,6 +453,7 @@ HttpBootDhcp6CallBack ( > // > // Cache the dhcp offers to OfferBuffer[] for select later, and record > // the OfferIndex and OfferCount. > + // If error happens, just ignore this packet and continue to wait more > offer. > // > HttpBootCacheDhcp6Offer (Private, Packet); > } > diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c > b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c > index 44b0714..5497390 100644 > --- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c > +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c > @@ -424,17 +424,24 @@ PxeBcSeedDhcp4Packet ( > @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. > + > **/ > -VOID > +EFI_STATUS > PxeBcCacheDhcp4Packet ( > IN EFI_DHCP4_PACKET *Dst, > IN EFI_DHCP4_PACKET *Src > ) > { > - ASSERT (Dst->Size >= Src->Length); > - > + if (Dst->Size < Src->Length) { > + return EFI_BUFFER_TOO_SMALL; > + } > + > CopyMem (&Dst->Dhcp4, &Src->Dhcp4, Src->Length); > Dst->Length = Src->Length; > + > + return EFI_SUCCESS; > } > > > @@ -620,8 +627,11 @@ PxeBcParseDhcp4Packet ( > @param[in] Ack Pointer to the DHCPv4 ack packet. > @param[in] Verified If TRUE, parse the ACK packet and store info into > mode data. > > + @retval EFI_SUCCESS Cache and parse the packet successfully. > + @retval EFI_BUFFER_TOO_SMALL Cache buffer is not big enough to > hold the packet. > + > **/ > -VOID > +EFI_STATUS > PxeBcCopyDhcp4Ack ( > IN PXEBC_PRIVATE_DATA *Private, > IN EFI_DHCP4_PACKET *Ack, > @@ -629,10 +639,14 @@ PxeBcCopyDhcp4Ack ( > ) > { > EFI_PXE_BASE_CODE_MODE *Mode; > + EFI_STATUS Status; > > Mode = Private->PxeBc.Mode; > > - PxeBcCacheDhcp4Packet (&Private->DhcpAck.Dhcp4.Packet.Ack, Ack); > + Status = PxeBcCacheDhcp4Packet (&Private->DhcpAck.Dhcp4.Packet.Ack, > + Ack); if (EFI_ERROR (Status)) { > + return Status; > + } > > if (Verified) { > // > @@ -642,6 +656,8 @@ PxeBcCopyDhcp4Ack ( > CopyMem (&Mode->DhcpAck.Dhcpv4, &Ack->Dhcp4, Ack->Length); > Mode->DhcpAckReceived = TRUE; > } > + > + return EFI_SUCCESS; > } > > > @@ -651,8 +667,11 @@ PxeBcCopyDhcp4Ack ( > @param[in] Private Pointer to PxeBc private data. > @param[in] OfferIndex The received order of offer packets. > > + @retval EFI_SUCCESS Cache and parse the packet successfully. > + @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 > @@ -660,6 +679,7 @@ PxeBcCopyProxyOffer ( { > EFI_PXE_BASE_CODE_MODE *Mode; > EFI_DHCP4_PACKET *Offer; > + EFI_STATUS Status; > > ASSERT (OfferIndex < Private->OfferNum); > ASSERT (OfferIndex < PXEBC_OFFER_MAX_NUM); @@ -670,7 +690,11 @@ > PxeBcCopyProxyOffer ( > // > // Cache the proxy offer packet and parse it. > // > - PxeBcCacheDhcp4Packet (&Private->ProxyOffer.Dhcp4.Packet.Offer, > Offer); > + Status = PxeBcCacheDhcp4Packet > + (&Private->ProxyOffer.Dhcp4.Packet.Offer, Offer); if (EFI_ERROR(Status)) > { > + return Status; > + } > + > PxeBcParseDhcp4Packet (&Private->ProxyOffer.Dhcp4); > > // > @@ -678,6 +702,8 @@ PxeBcCopyProxyOffer ( > // > CopyMem (&Mode->ProxyOffer.Dhcpv4, &Offer->Dhcp4, Offer->Length); > Mode->ProxyOfferReceived = TRUE; > + > + return EFI_SUCCESS; > } > > > @@ -780,8 +806,11 @@ PxeBcRetryBinlOffer ( > @param[in] Private Pointer to PxeBc private data. > @param[in] RcvdOffer Pointer to the received offer packet. > > + @retval EFI_SUCCESS Cache and parse the packet successfully. > + @retval Others Operation failed. > + > **/ > -VOID > +EFI_STATUS > PxeBcCacheDhcp4Offer ( > IN PXEBC_PRIVATE_DATA *Private, > IN EFI_DHCP4_PACKET *RcvdOffer > @@ -790,6 +819,7 @@ PxeBcCacheDhcp4Offer ( > PXEBC_DHCP4_PACKET_CACHE *Cache4; > EFI_DHCP4_PACKET *Offer; > PXEBC_OFFER_TYPE OfferType; > + EFI_STATUS Status; > > ASSERT (Private->OfferNum < PXEBC_OFFER_MAX_NUM); > Cache4 = &Private->OfferBuffer[Private->OfferNum].Dhcp4; > @@ -798,13 +828,16 @@ PxeBcCacheDhcp4Offer ( > // > // Cache the content of DHCPv4 packet firstly. > // > - PxeBcCacheDhcp4Packet (Offer, RcvdOffer); > + Status = PxeBcCacheDhcp4Packet (Offer, RcvdOffer); if > + (EFI_ERROR(Status)) { > + return Status; > + } > > // > // Validate the DHCPv4 packet, and parse the options and offer type. > // > if (EFI_ERROR (PxeBcParseDhcp4Packet (Cache4))) { > - return; > + return EFI_ABORTED; > } > > // > @@ -821,7 +854,7 @@ PxeBcCacheDhcp4Offer ( > Private->OfferIndex[OfferType][0] = Private->OfferNum; > Private->OfferCount[OfferType] = 1; > } else { > - return; > + return EFI_ABORTED; > } > } else { > ASSERT (Private->OfferCount[OfferType] < PXEBC_OFFER_MAX_NUM); > @@ -844,7 +877,7 @@ PxeBcCacheDhcp4Offer ( > Private->OfferIndex[OfferType][0] = Private->OfferNum; > Private->OfferCount[OfferType] = 1; > } else { > - return ; > + return EFI_ABORTED; > } > } else { > // > @@ -856,6 +889,8 @@ PxeBcCacheDhcp4Offer ( > } > > Private->OfferNum++; > + > + return EFI_SUCCESS; > } > > > @@ -980,11 +1015,12 @@ PxeBcSelectDhcp4Offer ( > /** > Handle the DHCPv4 offer packet. > > - @param[in] Private Pointer to PxeBc private data. > + @param[in] Private Pointer to PxeBc private data. > > - @retval EFI_SUCCESS Handled the DHCPv4 offer packet successfully. > - @retval EFI_NO_RESPONSE No response to the following request > packet. > - @retval EFI_NOT_FOUND No boot filename received. > + @retval EFI_SUCCESS Handled the DHCPv4 offer packet successfully. > + @retval EFI_NO_RESPONSE No response to the following request > packet. > + @retval EFI_NOT_FOUND No boot filename received. > + @retval EFI_BUFFER_TOO_SMALL Can't cache the offer pacet. > > **/ > EFI_STATUS > @@ -1089,7 +1125,7 @@ PxeBcHandleDhcp4Offer ( > // > // Success to try to request by a ProxyPxe10 or ProxyWfm11a offer, copy > and parse it. > // > - PxeBcCopyProxyOffer (Private, ProxyIndex); > + Status = PxeBcCopyProxyOffer (Private, ProxyIndex); > } > } else { > // > @@ -1116,7 +1152,10 @@ PxeBcHandleDhcp4Offer ( > Ack = Offer; > } > > - PxeBcCopyDhcp4Ack (Private, Ack, TRUE); > + Status = PxeBcCopyDhcp4Ack (Private, Ack, TRUE); > + if (EFI_ERROR (Status)) { > + return Status; > + } > Mode->DhcpDiscoverValid = TRUE; > } > > @@ -1258,6 +1297,7 @@ PxeBcDhcp4CallBack ( > // > // Cache the DHCPv4 offers to OfferBuffer[] for select later, and record > // the OfferIndex and OfferCount. > + // If error happens, just ignore this packet and continue to wait more > offer. > // > PxeBcCacheDhcp4Offer (Private, Packet); > } > @@ -1278,21 +1318,16 @@ PxeBcDhcp4CallBack ( > break; > > case Dhcp4RcvdAck: > - if (Packet->Length > PXEBC_DHCP4_PACKET_MAX_SIZE) { > - // > - // Abort the DHCP if the ACK packet exceeds the maximum length. > - // > - Status = EFI_ABORTED; > - break; > - } > - > // > // Cache the DHCPv4 ack to Private->Dhcp4Ack, but it's not the final ack in > mode data > // without verification. > // > ASSERT (Private->SelectIndex != 0); > > - PxeBcCopyDhcp4Ack (Private, Packet, FALSE); > + Status = PxeBcCopyDhcp4Ack (Private, Packet, FALSE); > + if (EFI_ERROR (Status)) { > + Status = EFI_ABORTED; > + } > break; > > default: > @@ -1491,6 +1526,12 @@ PxeBcDhcp4Discover ( > // Find the right PXE Reply according to server address. > // > while (RepIndex < Token.ResponseCount) { > + if (Response->Length > PXEBC_DHCP4_PACKET_MAX_SIZE) { > + SrvIndex = 0; > + RepIndex++; > + Response = (EFI_DHCP4_PACKET *) ((UINT8 *) Response + Response- > >Size); > + continue; > + } > > while (SrvIndex < IpCount) { > if (SrvList[SrvIndex].AcceptAnyResponse) { @@ -1509,7 +1550,6 @@ > PxeBcDhcp4Discover ( > > SrvIndex = 0; > RepIndex++; > - > Response = (EFI_DHCP4_PACKET *) ((UINT8 *) Response + Response- > >Size); > } > > @@ -1519,10 +1559,16 @@ PxeBcDhcp4Discover ( > // Especially for PXE discover packet, store it into mode data here. > // > if (Private->IsDoDiscover) { > - PxeBcCacheDhcp4Packet (&Private->PxeReply.Dhcp4.Packet.Ack, > Response); > + Status = PxeBcCacheDhcp4Packet (&Private- > >PxeReply.Dhcp4.Packet.Ack, Response); > + if (EFI_ERROR(Status)) { > + goto ON_EXIT; > + } > CopyMem (&Mode->PxeDiscover, &Token.Packet->Dhcp4, > Token.Packet->Length); > } else { > - PxeBcCacheDhcp4Packet (&Private->ProxyOffer.Dhcp4.Packet.Offer, > Response); > + Status = PxeBcCacheDhcp4Packet (&Private- > >ProxyOffer.Dhcp4.Packet.Offer, Response); > + if (EFI_ERROR(Status)) { > + goto ON_EXIT; > + } > } > } else { > // > @@ -1530,12 +1576,15 @@ PxeBcDhcp4Discover ( > // > Status = EFI_NOT_FOUND; > } > - if (Token.ResponseList != NULL) { > - FreePool (Token.ResponseList); > - } > } > - > - FreePool (Token.Packet); > +ON_EXIT: > + > + if (Token.ResponseList != NULL) { > + FreePool (Token.ResponseList); > + } > + if (Token.Packet != NULL) { > + FreePool (Token.Packet); > + } > return Status; > } > > diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c > b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c > index 6a08e9a..3672f13 100644 > --- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c > +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c > @@ -181,17 +181,24 @@ PxeBcBuildDhcp6Options ( > @param[in] Dst The pointer to the cache buffer for DHCPv6 packet. > @param[in] Src The pointer to the DHCPv6 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. > + > **/ > -VOID > +EFI_STATUS > PxeBcCacheDhcp6Packet ( > IN EFI_DHCP6_PACKET *Dst, > IN EFI_DHCP6_PACKET *Src > ) > { > - ASSERT (Dst->Size >= Src->Length); > + if (Dst->Size < Src->Length) { > + return EFI_BUFFER_TOO_SMALL; > + } > > CopyMem (&Dst->Dhcp6, &Src->Dhcp6, Src->Length); > Dst->Length = Src->Length; > + > + return EFI_SUCCESS; > } > > > @@ -749,8 +756,11 @@ PxeBcParseDhcp6Packet ( > @param[in] Ack The pointer to the DHCPv6 ack packet. > @param[in] Verified If TRUE, parse the ACK packet and store info into > mode data. > > + @retval EFI_SUCCESS Cache and parse the packet successfully. > + @retval EFI_BUFFER_TOO_SMALL Cache buffer is not big enough to > hold the packet. > + > **/ > -VOID > +EFI_STATUS > PxeBcCopyDhcp6Ack ( > IN PXEBC_PRIVATE_DATA *Private, > IN EFI_DHCP6_PACKET *Ack, > @@ -758,10 +768,14 @@ PxeBcCopyDhcp6Ack ( > ) > { > EFI_PXE_BASE_CODE_MODE *Mode; > + EFI_STATUS Status; > > Mode = Private->PxeBc.Mode; > > - PxeBcCacheDhcp6Packet (&Private->DhcpAck.Dhcp6.Packet.Ack, Ack); > + Status = PxeBcCacheDhcp6Packet (&Private->DhcpAck.Dhcp6.Packet.Ack, > + Ack); if (EFI_ERROR (Status)) { > + return Status; > + } > > if (Verified) { > // > @@ -771,6 +785,8 @@ PxeBcCopyDhcp6Ack ( > CopyMem (&Mode->DhcpAck.Dhcpv6, &Ack->Dhcp6, Ack->Length); > Mode->DhcpAckReceived = TRUE; > } > + > + return EFI_SUCCESS; > } > > > @@ -780,8 +796,11 @@ PxeBcCopyDhcp6Ack ( > @param[in] Private The pointer to PxeBc private data. > @param[in] OfferIndex The received order of offer packets. > > + @retval EFI_SUCCESS Cache and parse the packet successfully. > + @retval EFI_BUFFER_TOO_SMALL Cache buffer is not big enough to > hold the packet. > + > **/ > -VOID > +EFI_STATUS > PxeBcCopyDhcp6Proxy ( > IN PXEBC_PRIVATE_DATA *Private, > IN UINT32 OfferIndex > @@ -789,6 +808,7 @@ PxeBcCopyDhcp6Proxy ( { > EFI_PXE_BASE_CODE_MODE *Mode; > EFI_DHCP6_PACKET *Offer; > + EFI_STATUS Status; > > ASSERT (OfferIndex < Private->OfferNum); > ASSERT (OfferIndex < PXEBC_OFFER_MAX_NUM); @@ -799,7 +819,10 @@ > PxeBcCopyDhcp6Proxy ( > // > // Cache the proxy offer packet and parse it. > // > - PxeBcCacheDhcp6Packet (&Private->ProxyOffer.Dhcp6.Packet.Offer, > Offer); > + Status = PxeBcCacheDhcp6Packet > + (&Private->ProxyOffer.Dhcp6.Packet.Offer, Offer); if (EFI_ERROR(Status)) > { > + return Status; > + } > PxeBcParseDhcp6Packet (&Private->ProxyOffer.Dhcp6); > > // > @@ -807,6 +830,8 @@ PxeBcCopyDhcp6Proxy ( > // > CopyMem (&Mode->ProxyOffer.Dhcpv6, &Offer->Dhcp6, Offer->Length); > Mode->ProxyOfferReceived = TRUE; > + > + return EFI_SUCCESS; > } > > /** > @@ -1121,8 +1146,10 @@ PxeBcRetryDhcp6Binl ( > @param[in] Private The pointer to PXEBC_PRIVATE_DATA. > @param[in] RcvdOffer The pointer to the received offer packet. > > + @retval EFI_SUCCESS Cache and parse the packet successfully. > + @retval Others Operation failed. > **/ > -VOID > +EFI_STATUS > PxeBcCacheDhcp6Offer ( > IN PXEBC_PRIVATE_DATA *Private, > IN EFI_DHCP6_PACKET *RcvdOffer > @@ -1131,6 +1158,7 @@ PxeBcCacheDhcp6Offer ( > PXEBC_DHCP6_PACKET_CACHE *Cache6; > EFI_DHCP6_PACKET *Offer; > PXEBC_OFFER_TYPE OfferType; > + EFI_STATUS Status; > > Cache6 = &Private->OfferBuffer[Private->OfferNum].Dhcp6; > Offer = &Cache6->Packet.Offer; > @@ -1138,13 +1166,13 @@ PxeBcCacheDhcp6Offer ( > // > // Cache the content of DHCPv6 packet firstly. > // > - PxeBcCacheDhcp6Packet (Offer, RcvdOffer); > + Status = PxeBcCacheDhcp6Packet (Offer, RcvdOffer); > > // > // Validate the DHCPv6 packet, and parse the options and offer type. > // > if (EFI_ERROR (PxeBcParseDhcp6Packet (Cache6))) { > - return ; > + return EFI_ABORTED; > } > > // > @@ -1173,7 +1201,7 @@ PxeBcCacheDhcp6Offer ( > Private->OfferIndex[OfferType][0] = Private->OfferNum; > Private->OfferCount[OfferType] = 1; > } else { > - return; > + return EFI_ABORTED; > } > } else { > // > @@ -1184,6 +1212,8 @@ PxeBcCacheDhcp6Offer ( > } > > Private->OfferNum++; > + > + return EFI_SUCCESS; > } > > > @@ -1301,6 +1331,7 @@ PxeBcSelectDhcp6Offer ( > @retval EFI_SUCCESS Handled the DHCPv6 offer packet successfully. > @retval EFI_NO_RESPONSE No response to the following request > packet. > @retval EFI_OUT_OF_RESOURCES Failed to allocate resources. > + @retval EFI_BUFFER_TOO_SMALL Can't cache the offer pacet. > > **/ > EFI_STATUS > @@ -1410,7 +1441,7 @@ PxeBcHandleDhcp6Offer ( > // > // Success to try to request by a ProxyPxe10 or ProxyWfm11a offer, copy > and parse it. > // > - PxeBcCopyDhcp6Proxy (Private, ProxyIndex); > + Status = PxeBcCopyDhcp6Proxy (Private, ProxyIndex); > } > } else { > // > @@ -1424,7 +1455,7 @@ PxeBcHandleDhcp6Offer ( > // > // All PXE boot information is ready by now. > // > - PxeBcCopyDhcp6Ack (Private, &Private->DhcpAck.Dhcp6.Packet.Ack, > TRUE); > + Status = PxeBcCopyDhcp6Ack (Private, > + &Private->DhcpAck.Dhcp6.Packet.Ack, TRUE); > Private->PxeBc.Mode->DhcpDiscoverValid = TRUE; > } > > @@ -1997,19 +2028,15 @@ PxeBcDhcp6CallBack ( > break; > > case Dhcp6RcvdReply: > - if (Packet->Length > PXEBC_DHCP6_PACKET_MAX_SIZE) { > - // > - // Abort the DHCP if the Peply packet exceeds the maximum length. > - // > - Status = EFI_ABORTED; > - break; > - } > // > // Cache the dhcp ack to Private->Dhcp6Ack, but it's not the final ack in > mode data > // without verification. > // > ASSERT (Private->SelectIndex != 0); > - PxeBcCopyDhcp6Ack (Private, Packet, FALSE); > + Status = PxeBcCopyDhcp6Ack (Private, Packet, FALSE); > + if (EFI_ERROR (Status)) { > + Status = EFI_ABORTED; > + } > break; > > default: > -- > 2.7.4.windows.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch 0/2] Replace ASSERT with error return code in PXE and HTTP boot. 2016-12-16 8:18 [Patch 0/2] Replace ASSERT with error return code in PXE and HTTP boot Fu Siyuan 2016-12-16 8:18 ` [Patch 1/2] MdeModulePkg: Replace ASSERT with error return code in PXE driver Fu Siyuan 2016-12-16 8:18 ` [Patch 2/2] NetworkPkg: Replace ASSERT with error return code in PXE and HTTP boot driver Fu Siyuan @ 2016-12-16 8:45 ` Ye, Ting 2 siblings, 0 replies; 6+ messages in thread From: Ye, Ting @ 2016-12-16 8:45 UTC (permalink / raw) To: Fu, Siyuan, edk2-devel@lists.01.org Looks good to me. Series Reviewed-by: Ye Ting <ting.ye@intel.com> -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fu Siyuan Sent: Friday, December 16, 2016 4:19 PM To: edk2-devel@lists.01.org Subject: [edk2] [Patch 0/2] Replace ASSERT with error return code in PXE and HTTP boot. This patch remove the ASSERT when receive a DHCP packet large than the maximum cache buffer size. Fu Siyuan (2): MdeModulePkg: Replace ASSERT with error return code in PXE driver. NetworkPkg: Replace ASSERT with error return code in PXE and HTTP boot driver. .../Universal/Network/UefiPxeBcDxe/PxeBcDhcp.c | 99 +++++++++++------ .../Universal/Network/UefiPxeBcDxe/PxeBcDhcp.h | 5 +- NetworkPkg/HttpBootDxe/HttpBootDhcp4.c | 32 +++++- NetworkPkg/HttpBootDxe/HttpBootDhcp6.c | 29 +++-- NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c | 119 +++++++++++++++------ NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c | 67 ++++++++---- 6 files changed, 249 insertions(+), 102 deletions(-) -- 2.7.4.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-16 8:59 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-16 8:18 [Patch 0/2] Replace ASSERT with error return code in PXE and HTTP boot Fu Siyuan 2016-12-16 8:18 ` [Patch 1/2] MdeModulePkg: Replace ASSERT with error return code in PXE driver Fu Siyuan 2016-12-16 8:59 ` Wu, Jiaxin 2016-12-16 8:18 ` [Patch 2/2] NetworkPkg: Replace ASSERT with error return code in PXE and HTTP boot driver Fu Siyuan 2016-12-16 8:57 ` Wu, Jiaxin 2016-12-16 8:45 ` [Patch 0/2] Replace ASSERT with error return code in PXE and HTTP boot Ye, Ting
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox