Reviewed-by: Siyuan Fu <siyuan...@intel.com> > -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: 2020年3月31日 8:48 > To: edk2-devel-groups-io <devel@edk2.groups.io> > Cc: Wu, Jiaxin <jiaxin...@intel.com>; Maciej Rabeda > <maciej.rab...@linux.intel.com>; Philippe Mathieu-Daudé > <phi...@redhat.com>; Fu, Siyuan <siyuan...@intel.com> > Subject: [PATCH] NetworkPkg/UefiPxeBcDxe: handle competing DHCP servers > (more) gracefully > > When DHCP is misconfigured on a network segment, such that two DHCP > servers attempt to reply to requests (and therefore race with each other), > the edk2 PXE client can confuse itself. > > In PxeBcDhcp4BootInfo() / PxeBcDhcp6BootInfo(), the client may refer to a > DHCP reply packet as an "earlier" packet from the "same" DHCP server, when > in reality both packets are unrelated, and arrive from different DHCP > servers. > > While the edk2 PXE client can do nothing to fix this, it should at least > not ASSERT() -- ASSERT() is for catching programming errors (violations of > invariants that are under the control of the programmer). ASSERT()s should > in particular not refer to external data (such as network packets). What's > more, in RELEASE builds, we get NULL pointer references. > > Check the problem conditions with actual "if"s, and return > EFI_PROTOCOL_ERROR. This will trickle out to PxeBcLoadBootFile(), and be > reported as "PXE-E99: Unexpected network error". > > Cc: Jiaxin Wu <jiaxin...@intel.com> > Cc: Maciej Rabeda <maciej.rab...@linux.intel.com> > Cc: Philippe Mathieu-Daudé <phi...@redhat.com> > Cc: Siyuan Fu <siyuan...@intel.com> > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > --- > > Notes: > Repo: https://pagure.io/lersek/edk2.git > Branch: dhcp_assert > > NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c | 30 ++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c > b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c > index 10bbb06f7593..d062a526077b 100644 > --- a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c > +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c > @@ -482,7 +482,20 @@ PxeBcDhcp4BootInfo ( > Cache4 = &Private->DhcpAck.Dhcp4; > } > > - ASSERT (Cache4->OptList[PXEBC_DHCP4_TAG_INDEX_BOOTFILE] != NULL); > + if (Cache4->OptList[PXEBC_DHCP4_TAG_INDEX_BOOTFILE] == NULL) { > + // > + // This should never happen in a correctly configured DHCP / PXE > + // environment. One misconfiguration that can cause it is two DHCP > servers > + // mistakenly running on the same network segment at the same time, > and > + // racing each other in answering DHCP requests. Thus, the DHCP packets > + // that the edk2 PXE client considers "belonging together" may actually > be > + // entirely independent, coming from two (competing) DHCP servers. > + // > + // Try to deal with this gracefully. Note that this check is not > + // comprehensive, as we don't try to identify all such errors. > + // > + return EFI_PROTOCOL_ERROR; > + } > > // > // Parse the boot server address. > @@ -612,7 +625,20 @@ PxeBcDhcp6BootInfo ( > Cache6 = &Private->DhcpAck.Dhcp6; > } > > - ASSERT (Cache6->OptList[PXEBC_DHCP6_IDX_BOOT_FILE_URL] != NULL); > + if (Cache6->OptList[PXEBC_DHCP6_IDX_BOOT_FILE_URL] == NULL) { > + // > + // This should never happen in a correctly configured DHCP / PXE > + // environment. One misconfiguration that can cause it is two DHCP > servers > + // mistakenly running on the same network segment at the same time, > and > + // racing each other in answering DHCP requests. Thus, the DHCP packets > + // that the edk2 PXE client considers "belonging together" may actually > be > + // entirely independent, coming from two (competing) DHCP servers. > + // > + // Try to deal with this gracefully. Note that this check is not > + // comprehensive, as we don't try to identify all such errors. > + // > + return EFI_PROTOCOL_ERROR; > + } > > // > // Set the station address to IP layer. > -- > 2.19.1.3.g30247aa5d201
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56753): https://edk2.groups.io/g/devel/message/56753 Mute This Topic: https://groups.io/mt/72667954/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
