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

["Siyuan, Fu" <siyuan.fu@intel.com>]

Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: 2020年3月31日 8:48
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Maciej Rabeda
> <maciej.rabeda@linux.intel.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Fu, Siyuan <siyuan.fu@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.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.
> --
> 2.19.1.3.g30247aa5d201