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

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

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>

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.