On 03/31/20 19:49, Maciej Rabeda wrote: > Always better than not detecting such stuff at all (or by ASSERT in > debug builds). Thanks for the patch! > > Reviewed-by: Maciej Rabeda <maciej.rab...@linux.intel.com>
Thanks All for the reviews; pushed as commit 3f55418d5396629c4458061f283068b6c46895fc, via <https://github.com/tianocore/edk2/pull/491>. Laszlo > > On 31-Mar-20 02:47, Laszlo Ersek wrote: >> 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. > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56853): https://edk2.groups.io/g/devel/message/56853 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] -=-=-=-=-=-=-=-=-=-=-=-
