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

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

[Laszlo Ersek]

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

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

[Siyuan, Fu]

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

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

[Maciej Rabeda]

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.

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

[Laszlo Ersek]

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.
> 
> 
>