[PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Maciej Rabeda]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2876

According to PXE 2.1 spec, DHCP option 43, tag 6 (PXE_DISCOVERY_CONTROL),
bit 3 specifies whether PXE client should use/accept TFTP servers defined
within PXE_BOOT_SERVERS list (tag 8). This bit was not being taken into
account when choosing boot server IP in PxeBcDhcp4BootInfo().

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
 NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
index d062a526077b..ed9bca0f7de3 100644
--- a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
+++ b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
@@ -499,12 +499,16 @@ PxeBcDhcp4BootInfo (
 
   //
   // Parse the boot server address.
-  // If prompt/discover is disabled, get the first boot server from the boot servers list.
-  // Otherwise, parse the boot server Ipv4 address from next server address field in DHCP header.
+  // If prompt/discover is disabled, server list should be used and is present (DHCP option 43),
+  // get the first boot server from the boot servers list.
+  // Otherwise, parse the boot server IPv4 address from next server address field in DHCP header.
   // If all these fields are not available, use option 54 instead.
   //
   VendorOpt = &Cache4->VendorOpt;
-  if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) && IS_VALID_BOOT_SERVERS (VendorOpt->BitMap)) {
+  if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) &&
+      IS_VALID_BOOT_SERVERS (VendorOpt->BitMap) &&
+      IS_ENABLE_USE_SERVER_LIST (VendorOpt->DiscoverCtrl))
+  {
     Entry = VendorOpt->BootSvr;
     if (VendorOpt->BootSvrLen >= sizeof (PXEBC_BOOT_SVR_ENTRY) && Entry->IpCnt > 0) {
       CopyMem (
-- 
2.24.0.windows.2

Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Laszlo Ersek]

On 08/19/20 18:53, Maciej Rabeda wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2876
> 
> According to PXE 2.1 spec, DHCP option 43, tag 6 (PXE_DISCOVERY_CONTROL),
> bit 3 specifies whether PXE client should use/accept TFTP servers defined
> within PXE_BOOT_SERVERS list (tag 8). This bit was not being taken into
> account when choosing boot server IP in PxeBcDhcp4BootInfo().
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> ---
>  NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
> index d062a526077b..ed9bca0f7de3 100644
> --- a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
> +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
> @@ -499,12 +499,16 @@ PxeBcDhcp4BootInfo (
>  
>    //
>    // Parse the boot server address.
> -  // If prompt/discover is disabled, get the first boot server from the boot servers list.
> -  // Otherwise, parse the boot server Ipv4 address from next server address field in DHCP header.
> +  // If prompt/discover is disabled, server list should be used and is present (DHCP option 43),
> +  // get the first boot server from the boot servers list.
> +  // Otherwise, parse the boot server IPv4 address from next server address field in DHCP header.
>    // If all these fields are not available, use option 54 instead.
>    //
>    VendorOpt = &Cache4->VendorOpt;
> -  if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) && IS_VALID_BOOT_SERVERS (VendorOpt->BitMap)) {
> +  if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) &&
> +      IS_VALID_BOOT_SERVERS (VendorOpt->BitMap) &&
> +      IS_ENABLE_USE_SERVER_LIST (VendorOpt->DiscoverCtrl))
> +  {
>      Entry = VendorOpt->BootSvr;
>      if (VendorOpt->BootSvrLen >= sizeof (PXEBC_BOOT_SVR_ENTRY) && Entry->IpCnt > 0) {
>        CopyMem (
> 

I'm still undecided whether option#43 / tag#6 / bit#3 being clear means
we should *ignore* PXE_BOOT_SERVERS (tag#8), but I'm willing to defer to
you on that. So, I can give a cautious

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

for this patch.

Regarding the feature freeze -- in theory, this is a bugfix. However,
before we merge it, it would be really nice to get feedback from the
original reporter (CC'd).

I also intend to regression-test this patch, I'll report back.

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Michael Brown]

On 19/08/2020 19:13, Laszlo Ersek wrote:
> I'm still undecided whether option#43 / tag#6 / bit#3 being clear means
> we should *ignore* PXE_BOOT_SERVERS (tag#8), but I'm willing to defer to
> you on that. So, I can give a cautious
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> for this patch.

FWIW, iPXE's equivalent logic (based on a combination of what the PXE 
spec says and what the Intel reference PXE implementation actually does, 
which is not necessarily the same thing) is to *ignore* PXE_BOOT_SERVERS 
if a DHCP filename is available and option 43 tag 6 bit 3 is *set*.

Michael

Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Laszlo Ersek]

On 08/19/20 20:46, Michael Brown wrote:
> On 19/08/2020 19:13, Laszlo Ersek wrote:
>> I'm still undecided whether option#43 / tag#6 / bit#3 being clear means
>> we should *ignore* PXE_BOOT_SERVERS (tag#8), but I'm willing to defer to
>> you on that. So, I can give a cautious
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> for this patch.
> 
> FWIW, iPXE's equivalent logic (based on a combination of what the PXE
> spec says and what the Intel reference PXE implementation actually does,
> which is not necessarily the same thing) is to *ignore* PXE_BOOT_SERVERS
> if a DHCP filename is available and option 43 tag 6 bit 3 is *set*.

Sorry, I expressed my concern incorrectly (I think I was tripped up by
the typo in Maciej's commit message).

It's about bit#2 in PXE_DISCOVERY_CONTROL.

The question is whether bit#2 is *equivalent* to adhering to
PXE_BOOT_SERVERS ("if, and only if").

When bit#2 is set, the spec says we must. OK.

When bit#2 is clear, the spec doesn't say anything. So two
interpretations are possible, "we still may consider PXE_BOOT_SERVERS",
and "we must not consider PXE_BOOT_SERVERS".

Anyway -- I'm sending this just to explain my earlier email. My main
point remains:

http://mid.mail-archive.com/11640b08-6f42-e4d8-356d-91d4bdf86c2c@redhat.com

(alt link: https://edk2.groups.io/g/devel/message/64530)

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Michael Brown]

On 21/08/2020 12:19, Laszlo Ersek wrote:
> On 08/19/20 20:46, Michael Brown wrote:
>> FWIW, iPXE's equivalent logic (based on a combination of what the PXE
>> spec says and what the Intel reference PXE implementation actually does,
>> which is not necessarily the same thing) is to *ignore* PXE_BOOT_SERVERS
>> if a DHCP filename is available and option 43 tag 6 bit 3 is *set*.
> 
> Sorry, I expressed my concern incorrectly (I think I was tripped up by
> the typo in Maciej's commit message).

My fault; I should have read more closely.

> It's about bit#2 in PXE_DISCOVERY_CONTROL.
> 
> The question is whether bit#2 is *equivalent* to adhering to
> PXE_BOOT_SERVERS ("if, and only if").
> 
> When bit#2 is set, the spec says we must. OK.
> 
> When bit#2 is clear, the spec doesn't say anything. So two
> interpretations are possible, "we still may consider PXE_BOOT_SERVERS",
> and "we must not consider PXE_BOOT_SERVERS".

iPXE's interpretation is:

- if bit 2 is set then the subsequent boot server discovery will ignore 
replies from any servers not mentioned (for the selected boot server 
type) in PXE_BOOT_SERVERS

- if bit 2 is clear then the subsequent boot server discovery will 
accept replies from any server

This seems consistent with the spec paragraph stating:

   "If PXE_DISCOVERY_CONTROL bit 2 is set, the client may still use
    multicast and broadcast discovery (if it is permitted by bits 0
    and 1); but the client may only accept replies from servers that
    are identified in the PXE_BOOT_SERVERS option."

> Anyway -- I'm sending this just to explain my earlier email. My main
> point remains:
> 
> http://mid.mail-archive.com/11640b08-6f42-e4d8-356d-91d4bdf86c2c@redhat.com
> 
> (alt link: https://edk2.groups.io/g/devel/message/64530)

Resolving as INVALID seems appropriate to me.

Thanks,

Michael

Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Laszlo Ersek]

On 08/19/20 20:13, Laszlo Ersek wrote:
> On 08/19/20 18:53, Maciej Rabeda wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2876
>>
>> According to PXE 2.1 spec, DHCP option 43, tag 6 (PXE_DISCOVERY_CONTROL),
>> bit 3 specifies whether PXE client should use/accept TFTP servers defined
>> within PXE_BOOT_SERVERS list (tag 8). This bit was not being taken into
>> account when choosing boot server IP in PxeBcDhcp4BootInfo().
>>
>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
>> ---
>>  NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
>> index d062a526077b..ed9bca0f7de3 100644
>> --- a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
>> +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
>> @@ -499,12 +499,16 @@ PxeBcDhcp4BootInfo (
>>  
>>    //
>>    // Parse the boot server address.
>> -  // If prompt/discover is disabled, get the first boot server from the boot servers list.
>> -  // Otherwise, parse the boot server Ipv4 address from next server address field in DHCP header.
>> +  // If prompt/discover is disabled, server list should be used and is present (DHCP option 43),
>> +  // get the first boot server from the boot servers list.
>> +  // Otherwise, parse the boot server IPv4 address from next server address field in DHCP header.
>>    // If all these fields are not available, use option 54 instead.
>>    //
>>    VendorOpt = &Cache4->VendorOpt;
>> -  if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) && IS_VALID_BOOT_SERVERS (VendorOpt->BitMap)) {
>> +  if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) &&
>> +      IS_VALID_BOOT_SERVERS (VendorOpt->BitMap) &&
>> +      IS_ENABLE_USE_SERVER_LIST (VendorOpt->DiscoverCtrl))
>> +  {
>>      Entry = VendorOpt->BootSvr;
>>      if (VendorOpt->BootSvrLen >= sizeof (PXEBC_BOOT_SVR_ENTRY) && Entry->IpCnt > 0) {
>>        CopyMem (
>>
> 
> I'm still undecided whether option#43 / tag#6 / bit#3 being clear means
> we should *ignore* PXE_BOOT_SERVERS (tag#8), but I'm willing to defer to
> you on that. So, I can give a cautious
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> for this patch.
> 
> Regarding the feature freeze -- in theory, this is a bugfix. However,
> before we merge it, it would be really nice to get feedback from the
> original reporter (CC'd).
> 
> I also intend to regression-test this patch, I'll report back.

I've repeated my usual PXEv4 boot tests (using OVMF IA32X64 and
ArmVirtQemu AARCH64 guests, covering shim + grub + vmlinuz + initrd);
nothing seems to have regressed.

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Maciej Rabeda]

@Laszlo
Thanks for regression test!

I need to correct the typo in commit msg, (bit 3 -> bit 2).
Code logic is not affected.

@Michael
I am now wondering whether bit 3 is actually relevant to server choice.

Bit 3:
== 0 -> prompt user to choose a boot file. Which means to me: show 
minimal menu with prompt (tag 10 - PXE_MENU_PROMPT) and options (tag 9 - 
PXE_BOOT_MENU).
== 1 -> do not prompt user. If boot file name is present (option 67), 
download that boot file.

Bit 3 does not seem to specify/regulate which server to use.

Choice of server IP might look like:

if (option 43 is present, tag 6 is present, tag_6.bit_2 is set and tag 8 
is present and valid)
         take server IP from tag 8 (PXE_BOOT_SERVERS)

else if (option 66 is present)
         take server IP from option 66 (TFTP server name)

else if (option 54 is present)
         take server IP from option 54 (Server Identifier)

else
         failure

Looking forward to discussion.

Thanks,
Maciej

On 19-Aug-20 21:20, Laszlo Ersek wrote:
> On 08/19/20 20:13, Laszlo Ersek wrote:
>> On 08/19/20 18:53, Maciej Rabeda wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2876
>>>
>>> According to PXE 2.1 spec, DHCP option 43, tag 6 (PXE_DISCOVERY_CONTROL),
>>> bit 3 specifies whether PXE client should use/accept TFTP servers defined
>>> within PXE_BOOT_SERVERS list (tag 8). This bit was not being taken into
>>> account when choosing boot server IP in PxeBcDhcp4BootInfo().
>>>
>>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
>>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>>> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
>>> ---
>>>   NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c | 10 +++++++---
>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
>>> index d062a526077b..ed9bca0f7de3 100644
>>> --- a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
>>> +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
>>> @@ -499,12 +499,16 @@ PxeBcDhcp4BootInfo (
>>>   
>>>     //
>>>     // Parse the boot server address.
>>> -  // If prompt/discover is disabled, get the first boot server from the boot servers list.
>>> -  // Otherwise, parse the boot server Ipv4 address from next server address field in DHCP header.
>>> +  // If prompt/discover is disabled, server list should be used and is present (DHCP option 43),
>>> +  // get the first boot server from the boot servers list.
>>> +  // Otherwise, parse the boot server IPv4 address from next server address field in DHCP header.
>>>     // If all these fields are not available, use option 54 instead.
>>>     //
>>>     VendorOpt = &Cache4->VendorOpt;
>>> -  if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) && IS_VALID_BOOT_SERVERS (VendorOpt->BitMap)) {
>>> +  if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) &&
>>> +      IS_VALID_BOOT_SERVERS (VendorOpt->BitMap) &&
>>> +      IS_ENABLE_USE_SERVER_LIST (VendorOpt->DiscoverCtrl))
>>> +  {
>>>       Entry = VendorOpt->BootSvr;
>>>       if (VendorOpt->BootSvrLen >= sizeof (PXEBC_BOOT_SVR_ENTRY) && Entry->IpCnt > 0) {
>>>         CopyMem (
>>>
>> I'm still undecided whether option#43 / tag#6 / bit#3 being clear means
>> we should *ignore* PXE_BOOT_SERVERS (tag#8), but I'm willing to defer to
>> you on that. So, I can give a cautious
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> for this patch.
>>
>> Regarding the feature freeze -- in theory, this is a bugfix. However,
>> before we merge it, it would be really nice to get feedback from the
>> original reporter (CC'd).
>>
>> I also intend to regression-test this patch, I'll report back.
> I've repeated my usual PXEv4 boot tests (using OVMF IA32X64 and
> ArmVirtQemu AARCH64 guests, covering shim + grub + vmlinuz + initrd);
> nothing seems to have regressed.
>
> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo
>
>
> 
>

Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Michael Brown]

On 20/08/2020 11:44, Maciej Rabeda wrote:
> @Michael
> I am now wondering whether bit 3 is actually relevant to server choice.
> 
> Bit 3:
> == 0 -> prompt user to choose a boot file. Which means to me: show 
> minimal menu with prompt (tag 10 - PXE_MENU_PROMPT) and options (tag 9 - 
> PXE_BOOT_MENU).
> == 1 -> do not prompt user. If boot file name is present (option 67), 
> download that boot file.
> 
> Bit 3 does not seem to specify/regulate which server to use.
> 
> Choice of server IP might look like:
> 
> if (option 43 is present, tag 6 is present, tag_6.bit_2 is set and tag 8 
> is present and valid)
>          take server IP from tag 8 (PXE_BOOT_SERVERS)
> 
> else if (option 66 is present)
>          take server IP from option 66 (TFTP server name)
> 
> else if (option 54 is present)
>          take server IP from option 54 (Server Identifier)
> 
> else
>          failure

RFC 2132 defines option 66 as a hostname (not an IP address): it is the 
equivalent of the non-option "sname" field.

RFC 2132 defines option 54 as the DHCP server identifier, which is 
unrelated to the TFTP server.

In the simple case (with no PXE menus involved), the TFTP server IP is 
provided by the non-option "siaddr" field.

If option 60 is set to "PXEClient" and option 43 tag 9 is present and 
option 43 tag 6 bit 3 is clear then this initiates a convoluted process 
in which the user is first presented with an interactive menu 
(constructed from the contents of option 43 tag 9) in order to select a 
"boot server type", after which a second convoluted process is performed 
to query the network using a protocol that is almost, but not quite, 
entirely unlike DHCP.  The TFTP server IP and boot filename are 
eventually taken from the selected response packet in this final 
almost-DHCP exchange.

Michael

Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Laszlo Ersek]

On 08/20/20 15:41, Michael Brown wrote:
> On 20/08/2020 11:44, Maciej Rabeda wrote:
>> @Michael
>> I am now wondering whether bit 3 is actually relevant to server choice.
>>
>> Bit 3:
>> == 0 -> prompt user to choose a boot file. Which means to me: show
>> minimal menu with prompt (tag 10 - PXE_MENU_PROMPT) and options (tag 9
>> - PXE_BOOT_MENU).
>> == 1 -> do not prompt user. If boot file name is present (option 67),
>> download that boot file.
>>
>> Bit 3 does not seem to specify/regulate which server to use.
>>
>> Choice of server IP might look like:
>>
>> if (option 43 is present, tag 6 is present, tag_6.bit_2 is set and tag
>> 8 is present and valid)
>>          take server IP from tag 8 (PXE_BOOT_SERVERS)
>>
>> else if (option 66 is present)
>>          take server IP from option 66 (TFTP server name)
>>
>> else if (option 54 is present)
>>          take server IP from option 54 (Server Identifier)
>>
>> else
>>          failure
> 
> RFC 2132 defines option 66 as a hostname (not an IP address): it is the
> equivalent of the non-option "sname" field.
> 
> RFC 2132 defines option 54 as the DHCP server identifier, which is
> unrelated to the TFTP server.
> 
> In the simple case (with no PXE menus involved), the TFTP server IP is
> provided by the non-option "siaddr" field.
> 
> If option 60 is set to "PXEClient" and option 43 tag 9 is present and
> option 43 tag 6 bit 3 is clear then this initiates a convoluted process
> in which the user is first presented with an interactive menu
> (constructed from the contents of option 43 tag 9) in order to select a
> "boot server type", after which a second convoluted process is performed
> to query the network using a protocol that is almost, but not quite,
> entirely unlike DHCP.  The TFTP server IP and boot filename are
> eventually taken from the selected response packet in this final
> almost-DHCP exchange.

*shudder*

I'll 100% defer to you and Maciej on this -- this is very complicated.

To begin with, I'm not fully clear what the purpose of edk2 git commit
ecec42044078 ("Update PXE driver to support PXE forced mode.",
2014-01-06) was.

What on Earth is "PXE forced mode"?

Siyuan, can you please explain?

And then I don't know whether the bug report at

  https://bugzilla.tianocore.org/show_bug.cgi?id=2876

really has merit.

In the words of the reporter, the presently discussed patch would still
qualify as a "work-around", for making the PXE client ignore
PXE_BOOT_SERVERS, via clearing option#43 tag#6 bit#2 in the DHCP server
response. But IMO the more important question is whether it is valid for
the DHCP server (config) at their site to (a) populate PXE_BOOT_SERVERS,
(b) put (apparently!) the ProxyDHCP IP address in PXE_BOOT_SERVERS.

Like, I'd like to be convinced that the server config at the reporter's
site is not *invalid* in the first place. If it's invalid, then we
shouldn't be complicating the edk2 client code with a workaround. Even
if we adopted the workaround, the reporter would still have to
*activate* it, by manually clearing the bit in question (see at the very
end of <https://bugzilla.tianocore.org/show_bug.cgi?id=2876#c4>).

For me one big difficulty is that the PXE config options are scattered
about a forest of specs. Last time I spent more than an hour cursing and
hunting for them.

At Red Hat, over the last few years I've received an immense amount of
bug reports related to PXEv4/PXEv6 booting with edk2. In almost every
case, it was a bug in the reporter's server configuration. Yes,
anecdotal evidence. It makes me very reluctant to change the edk2 code,
especially that the reporter of TianoCore#2876 has seemingly stopped
communications.

Note how the bug report makes references to various attachments, such as
RAR files and one "Serva32.exe", regarding a reproducer. But until now,
with the latest comment being #9, those files have *not* been attached.
So it's not like we can set up some virtual machines on a virtual
network and fire up wireshark or tcpdump, to see the actual traffic.

I'm happy to pull out of this review session, as I trust you Michael and
Maciej to do the right here. I'm happy to offer some level of regression
testing, if you got new patches. I'd also be OK to simply close
TianoCore#2876 as INVALID (due to insufficient data).

Thanks
Laszlo

回复: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Ding, Seven]

[-- Attachment #1.1.1: Type: text/plain, Size: 6776 bytes --]

Thank you for your attention to this issue,  current status we already fix our customer problem, but we don’t know the root cause  whether related to server  configuration or EDK2 code.

I have no concerns with close TianoCore#2876 as INVALID

Thank you all.





Attached package is server configuration files,   PSW: 123

How to reproduce:

1. Prepare three machines for server and a switch with at least 4 LAN ports (you can also use a router instead). The machine will use Windows 10 system

2. Connect all three machines to the switch with a network cable. The switch does not need to be connected to an external network.

3. Turn off the firewalls on each machine

4. Set the static Address of the first machine to 192.168.20.2, extract the TFTP.rAR package and copy it to the root directory of Disk C, click Serva32. exe and wait for 7 seconds, then click "Thanks, not today..." The button

5. Set the second machine static IP 192.168.20.100, extract dhCp.rar and copy it to the root directory of Disk C, and click Serva32.exe

6. Set the third machine static IP 192.168.20.10, extract copy of proxydhcp.rar and proxydhcpworking. Rar to the root directory of Disk C, just click serva32.exe in the proxydhCP folder

7. Ping the machines to make sure they are connected

8. To prepare a fourth machine, connected to the switch.

9. Set UEFI only Mode in the BIOS (the fourth machine ) , Restart the machine press F12 to enter the boot page and select PXE Boot.

you will see the fourth machine connect to proxydhcp-> problem





problem:

[cid:image005.jpg@01D677EC.DD9D2960]



Expection:





[cid:image006.jpg@01D677EC.DD9D2960]









-----邮件原件-----
发件人: Laszlo Ersek <lersek@redhat.com>
发送时间: Friday, August 21, 2020 5:11 PM
收件人: Michael Brown <mcb30@ipxe.org>; devel@edk2.groups.io; maciej.rabeda@linux.intel.com
抄送: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Ding, Seven <Seven.ding@lcfuturecenter.com>
主题: Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow



On 08/20/20 15:41, Michael Brown wrote:

> On 20/08/2020 11:44, Maciej Rabeda wrote:

>> @Michael

>> I am now wondering whether bit 3 is actually relevant to server choice.

>>

>> Bit 3:

>> == 0 -> prompt user to choose a boot file. Which means to me: show

>> minimal menu with prompt (tag 10 - PXE_MENU_PROMPT) and options (tag 9

>> - PXE_BOOT_MENU).

>> == 1 -> do not prompt user. If boot file name is present (option 67),

>> download that boot file.

>>

>> Bit 3 does not seem to specify/regulate which server to use.

>>

>> Choice of server IP might look like:

>>

>> if (option 43 is present, tag 6 is present, tag_6.bit_2 is set and tag

>> 8 is present and valid)

>>          take server IP from tag 8 (PXE_BOOT_SERVERS)

>>

>> else if (option 66 is present)

>>          take server IP from option 66 (TFTP server name)

>>

>> else if (option 54 is present)

>>          take server IP from option 54 (Server Identifier)

>>

>> else

>>          failure

>

> RFC 2132 defines option 66 as a hostname (not an IP address): it is the

> equivalent of the non-option "sname" field.

>

> RFC 2132 defines option 54 as the DHCP server identifier, which is

> unrelated to the TFTP server.

>

> In the simple case (with no PXE menus involved), the TFTP server IP is

> provided by the non-option "siaddr" field.

>

> If option 60 is set to "PXEClient" and option 43 tag 9 is present and

> option 43 tag 6 bit 3 is clear then this initiates a convoluted process

> in which the user is first presented with an interactive menu

> (constructed from the contents of option 43 tag 9) in order to select a

> "boot server type", after which a second convoluted process is performed

> to query the network using a protocol that is almost, but not quite,

> entirely unlike DHCP.  The TFTP server IP and boot filename are

> eventually taken from the selected response packet in this final

> almost-DHCP exchange.



*shudder*



I'll 100% defer to you and Maciej on this -- this is very complicated.



To begin with, I'm not fully clear what the purpose of edk2 git commit

ecec42044078 ("Update PXE driver to support PXE forced mode.",

2014-01-06) was.



What on Earth is "PXE forced mode"?



Siyuan, can you please explain?



And then I don't know whether the bug report at



  https://bugzilla.tianocore.org/show_bug.cgi?id=2876



really has merit.



In the words of the reporter, the presently discussed patch would still

qualify as a "work-around", for making the PXE client ignore

PXE_BOOT_SERVERS, via clearing option#43 tag#6 bit#2 in the DHCP server

response. But IMO the more important question is whether it is valid for

the DHCP server (config) at their site to (a) populate PXE_BOOT_SERVERS,

(b) put (apparently!) the ProxyDHCP IP address in PXE_BOOT_SERVERS.



Like, I'd like to be convinced that the server config at the reporter's

site is not *invalid* in the first place. If it's invalid, then we

shouldn't be complicating the edk2 client code with a workaround. Even

if we adopted the workaround, the reporter would still have to

*activate* it, by manually clearing the bit in question (see at the very

end of <https://bugzilla.tianocore.org/show_bug.cgi?id=2876#c4>).



For me one big difficulty is that the PXE config options are scattered

about a forest of specs. Last time I spent more than an hour cursing and

hunting for them.



At Red Hat, over the last few years I've received an immense amount of

bug reports related to PXEv4/PXEv6 booting with edk2. In almost every

case, it was a bug in the reporter's server configuration. Yes,

anecdotal evidence. It makes me very reluctant to change the edk2 code,

especially that the reporter of TianoCore#2876 has seemingly stopped

communications.



Note how the bug report makes references to various attachments, such as

RAR files and one "Serva32.exe", regarding a reproducer. But until now,

with the latest comment being #9, those files have *not* been attached.

So it's not like we can set up some virtual machines on a virtual

network and fire up wireshark or tcpdump, to see the actual traffic.



I'm happy to pull out of this review session, as I trust you Michael and

Maciej to do the right here. I'm happy to offer some level of regression

testing, if you got new patches. I'd also be OK to simply close

TianoCore#2876 as INVALID (due to insufficient data).



Thanks

Laszlo



[-- Attachment #1.1.2: Type: text/html, Size: 15204 bytes --]

[-- Attachment #1.2: image005.jpg --]
[-- Type: image/jpeg, Size: 38051 bytes --]

[-- Attachment #1.3: image006.jpg --]
[-- Type: image/jpeg, Size: 42104 bytes --]

[-- Attachment #2: TOOL_DHCP.7z --]
[-- Type: application/octet-stream, Size: 2426121 bytes --]

Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Maciej Rabeda]

@Michael
I stand corrected on option 66. I have confused myself with "next 
server" comment in EFI code.

Bugzilla 2876
----------------
The more I get into the problem with a fresh mind and a cup of tea, the 
more I realize that it is indeed a problem in reporters configuration.
Reporter stated:
- Failing scenario: tag 6 == 0x7
- Working scenario: tag 6 == 0x3

In the failing scenario, user sets bit 2 (so PXE_BOOT_SERVERS are being 
used) and PXE_BOOT_SERVERS contains ProxyDHCP IP instead of TFTP server.
It should work fine if ProxyDHCP and TFTP sits on the same machine (with 
the same IP).
Case closed, patch to be dropped, BZ to be rejected.

Philosophy / PXE Forced Mode
----------------
All I could find about 'PXE Forced Mode' is some non-spec 
tooling/documentation.
https://knowledge.broadcom.com/external/article?legacyId=HOWTO7071
https://knowledge.broadcom.com/external/article/180499/pxe-forced-mode-utility.html

PDF from the first link's page sets up tag 6 to value 11 (bit 3, 1, 0 
are set) and setting up tag 8 with some values.
Now I am sharing the shudder. Why is PXE_BOOT_SERVERS being used when 
tag 6, bit 3 is set?
All I can blame is PXE spec being very vague on the matter or some kind 
of ancient knowledge that I am not aware of.

Thanks,
Maciej.

On 21-Aug-20 11:11, Laszlo Ersek wrote:
> On 08/20/20 15:41, Michael Brown wrote:
>> On 20/08/2020 11:44, Maciej Rabeda wrote:
>>> @Michael
>>> I am now wondering whether bit 3 is actually relevant to server choice.
>>>
>>> Bit 3:
>>> == 0 -> prompt user to choose a boot file. Which means to me: show
>>> minimal menu with prompt (tag 10 - PXE_MENU_PROMPT) and options (tag 9
>>> - PXE_BOOT_MENU).
>>> == 1 -> do not prompt user. If boot file name is present (option 67),
>>> download that boot file.
>>>
>>> Bit 3 does not seem to specify/regulate which server to use.
>>>
>>> Choice of server IP might look like:
>>>
>>> if (option 43 is present, tag 6 is present, tag_6.bit_2 is set and tag
>>> 8 is present and valid)
>>>           take server IP from tag 8 (PXE_BOOT_SERVERS)
>>>
>>> else if (option 66 is present)
>>>           take server IP from option 66 (TFTP server name)
>>>
>>> else if (option 54 is present)
>>>           take server IP from option 54 (Server Identifier)
>>>
>>> else
>>>           failure
>> RFC 2132 defines option 66 as a hostname (not an IP address): it is the
>> equivalent of the non-option "sname" field.
>>
>> RFC 2132 defines option 54 as the DHCP server identifier, which is
>> unrelated to the TFTP server.
>>
>> In the simple case (with no PXE menus involved), the TFTP server IP is
>> provided by the non-option "siaddr" field.
>>
>> If option 60 is set to "PXEClient" and option 43 tag 9 is present and
>> option 43 tag 6 bit 3 is clear then this initiates a convoluted process
>> in which the user is first presented with an interactive menu
>> (constructed from the contents of option 43 tag 9) in order to select a
>> "boot server type", after which a second convoluted process is performed
>> to query the network using a protocol that is almost, but not quite,
>> entirely unlike DHCP.  The TFTP server IP and boot filename are
>> eventually taken from the selected response packet in this final
>> almost-DHCP exchange.
> *shudder*
>
> I'll 100% defer to you and Maciej on this -- this is very complicated.
>
> To begin with, I'm not fully clear what the purpose of edk2 git commit
> ecec42044078 ("Update PXE driver to support PXE forced mode.",
> 2014-01-06) was.
>
> What on Earth is "PXE forced mode"?
>
> Siyuan, can you please explain?
>
> And then I don't know whether the bug report at
>
>    https://bugzilla.tianocore.org/show_bug.cgi?id=2876
>
> really has merit.
>
> In the words of the reporter, the presently discussed patch would still
> qualify as a "work-around", for making the PXE client ignore
> PXE_BOOT_SERVERS, via clearing option#43 tag#6 bit#2 in the DHCP server
> response. But IMO the more important question is whether it is valid for
> the DHCP server (config) at their site to (a) populate PXE_BOOT_SERVERS,
> (b) put (apparently!) the ProxyDHCP IP address in PXE_BOOT_SERVERS.
>
> Like, I'd like to be convinced that the server config at the reporter's
> site is not *invalid* in the first place. If it's invalid, then we
> shouldn't be complicating the edk2 client code with a workaround. Even
> if we adopted the workaround, the reporter would still have to
> *activate* it, by manually clearing the bit in question (see at the very
> end of <https://bugzilla.tianocore.org/show_bug.cgi?id=2876#c4>).
>
> For me one big difficulty is that the PXE config options are scattered
> about a forest of specs. Last time I spent more than an hour cursing and
> hunting for them.
>
> At Red Hat, over the last few years I've received an immense amount of
> bug reports related to PXEv4/PXEv6 booting with edk2. In almost every
> case, it was a bug in the reporter's server configuration. Yes,
> anecdotal evidence. It makes me very reluctant to change the edk2 code,
> especially that the reporter of TianoCore#2876 has seemingly stopped
> communications.
>
> Note how the bug report makes references to various attachments, such as
> RAR files and one "Serva32.exe", regarding a reproducer. But until now,
> with the latest comment being #9, those files have *not* been attached.
> So it's not like we can set up some virtual machines on a virtual
> network and fire up wireshark or tcpdump, to see the actual traffic.
>
> I'm happy to pull out of this review session, as I trust you Michael and
> Maciej to do the right here. I'm happy to offer some level of regression
> testing, if you got new patches. I'd also be OK to simply close
> TianoCore#2876 as INVALID (due to insufficient data).
>
> Thanks
> Laszlo
>

Re: [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

[Siyuan, Fu]

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

> -----Original Message-----
> From: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Sent: 2020年8月20日 0:54
> To: devel@edk2.groups.io
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage
> in boot info parse flow
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2876
> 
> According to PXE 2.1 spec, DHCP option 43, tag 6 (PXE_DISCOVERY_CONTROL),
> bit 3 specifies whether PXE client should use/accept TFTP servers defined
> within PXE_BOOT_SERVERS list (tag 8). This bit was not being taken into
> account when choosing boot server IP in PxeBcDhcp4BootInfo().
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> ---
>  NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
> b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
> index d062a526077b..ed9bca0f7de3 100644
> --- a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
> +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c
> @@ -499,12 +499,16 @@ PxeBcDhcp4BootInfo (
> 
> 
>    //
> 
>    // Parse the boot server address.
> 
> -  // If prompt/discover is disabled, get the first boot server from the boot
> servers list.
> 
> -  // Otherwise, parse the boot server Ipv4 address from next server address field
> in DHCP header.
> 
> +  // If prompt/discover is disabled, server list should be used and is present
> (DHCP option 43),
> 
> +  // get the first boot server from the boot servers list.
> 
> +  // Otherwise, parse the boot server IPv4 address from next server address
> field in DHCP header.
> 
>    // If all these fields are not available, use option 54 instead.
> 
>    //
> 
>    VendorOpt = &Cache4->VendorOpt;
> 
> -  if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) &&
> IS_VALID_BOOT_SERVERS (VendorOpt->BitMap)) {
> 
> +  if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) &&
> 
> +      IS_VALID_BOOT_SERVERS (VendorOpt->BitMap) &&
> 
> +      IS_ENABLE_USE_SERVER_LIST (VendorOpt->DiscoverCtrl))
> 
> +  {
> 
>      Entry = VendorOpt->BootSvr;
> 
>      if (VendorOpt->BootSvrLen >= sizeof (PXEBC_BOOT_SVR_ENTRY) && Entry-
> >IpCnt > 0) {
> 
>        CopyMem (
> 
> --
> 2.24.0.windows.2