Re: [edk2-devel] [PATCH] NetworkPkg/UefiPxeBcDxe: handle competing DHCP servers (more) gracefully

["Laszlo Ersek" <lersek@redhat.com>]

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.rabeda@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.wu@intel.com>
>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>> Signed-off-by: Laszlo Ersek <lersek@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.
> 
> 
>