Re: [PATCH] efinet: get bootstrap info from proxy offer packet

[Andrei Borzenkov]

On Thu, Apr 21, 2016 at 11:36 AM, chen zhihui <address@hidden> wrote:
> From: chenzhihui <address@hidden>
>
> Bootstrap server ip address and boot file name maybe come from
> a separate proxy DHCP server, check the proxy_offer packet if
> failed with dhcp_ack packet.
>

Yes, this came up before. Thank you for a patch. This is likely
post-2.02 material though.

> Signed-off-by: chenzhihui <address@hidden>
> Tested-by: Jerome Forissier <address@hidden>
> ---
>  grub-core/net/bootp.c              | 170 
> ++++++++++++++++++++++++++++++++++++-
>  grub-core/net/drivers/efi/efinet.c |  23 ++++-
>  include/grub/net.h                 |  10 +++
>  3 files changed, 200 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index 4fdeac3..52f4051 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -186,7 +186,7 @@ grub_net_configure_by_dhcp_ack (const char *name,
>      }
>  #endif
>
> -  if (size > OFFSET_OF (boot_file, bp))
> +  if (size > OFFSET_OF (boot_file, bp) && bp->boot_file[0])
>      grub_env_set_net_property (name, "boot_file", bp->boot_file,
>                                 sizeof (bp->boot_file));
>    if (is_def)
> @@ -233,7 +233,7 @@ grub_net_configure_by_dhcp_ack (const char *name,
>         }
>      }
>
> -  if (size > OFFSET_OF (boot_file, bp) && path)
> +  if (size > OFFSET_OF (boot_file, bp) && bp->boot_file[0] && path)
>      {
>        *path = grub_strndup (bp->boot_file, sizeof (bp->boot_file));
>        grub_print_error ();
> @@ -263,6 +263,172 @@ grub_net_configure_by_dhcp_ack (const char *name,
>    return inter;
>  }
>
> +struct dhcp4_packet_option {
> +       grub_uint8_t code;
> +       grub_uint8_t length;
> +       grub_uint8_t data[0];
> +};
> +
> +/*
> + * Get specified option from DHCP extension data
> + *
> + * from PxeBcDhcp.c of UEFI
> + *
> + */
> +static struct dhcp4_packet_option *
> +dhcp_proxy_extension_option (const grub_uint8_t *buf,
> +               grub_size_t size,
> +               grub_uint8_t code)
> +{
> +       struct dhcp4_packet_option *option = (struct dhcp4_packet_option 
> *)buf;
> +       grub_size_t offset = 0;
> +
> +       while (offset < size && option->code != GRUB_NET_BOOTP_END) {

OK, could you please rebase it on top of
http://lists.gnu.org/archive/html/grub-devel/2016-03/msg00280.html. Of
course comments to this patch are welcome. Without comments it will
land here at some point and it implements DHCP options processing
already. If you find it easier, I can create branch with this patch.

...
> +void
> +grub_net_configure_by_proxy_offer (const struct grub_net_bootp_packet *bp,
> +                               grub_size_t size,
> +                               char **device,
> +                               char **path)
> +{
> +       const grub_uint8_t *buf = bp->vendor + sizeof (grub_uint32_t);
> +       grub_uint32_t option_size =
> +               size - OFFSET_OF(vendor, bp) - sizeof (grub_uint32_t);
> +       struct dhcp4_packet_option *option;
> +
> +       if (device == NULL)
> +               return;
> +
> +       if (!proxy_offer_is_valid(bp, size))
> +               return;
> +
> +       if (!*device && bp->server_ip)
> +       {
> +               *device = grub_xasprintf ("tftp,%d.%d.%d.%d",
> +                               ((grub_uint8_t *) &bp->server_ip)[0],
> +                               ((grub_uint8_t *) &bp->server_ip)[1],
> +                               ((grub_uint8_t *) &bp->server_ip)[2],
> +                               ((grub_uint8_t *) &bp->server_ip)[3]);
> +               grub_print_error ();
> +       }
> +
> +       option = dhcp_proxy_extension_option(buf,
> +                       option_size, GRUB_NET_DHCP_OVERLOAD);
> +
> +       if ((option == NULL || option->data[0] == 1) && !*device && 
> bp->server_name[0])
> +       {
> +               *device = grub_xasprintf ("tftp,%s", bp->server_name);
> +               grub_print_error ();
> +       }
> +
> +       if (!*device)
> +       {
> +               option = dhcp_proxy_extension_option(buf,
> +                               option_size, GRUB_NET_DHCP_SERVER_ID);
> +
> +               if (option) {
> +                       *device = grub_xasprintf("tftp,%d.%d.%d.%d",
> +                                       option->data[0],
> +                                       option->data[1],
> +                                       option->data[2],
> +                                       option->data[3]);

DHCP_SERVER_ID (option 54) is defined for DHCP client/server
communication only. There is no implied usage of this option as next
server (i.e. boot server) so I do not think this is correct.

...
> diff --git a/grub-core/net/drivers/efi/efinet.c 
> b/grub-core/net/drivers/efi/efinet.c
> index 5388f95..ef0ccd9 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -338,6 +338,7 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char 
> **device,
>    FOR_NET_CARDS (card)
>    {
>      grub_efi_device_path_t *cdp;
> +       struct grub_net_network_level_interface *inter;
>      struct grub_efi_pxe *pxe;
>      struct grub_efi_pxe_mode *pxe_mode;
>      if (card->driver != &efidriver)
> @@ -378,11 +379,31 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char 
> **device,
>      if (! pxe)
>        continue;
>      pxe_mode = pxe->mode;
> -    grub_net_configure_by_dhcp_ack (card->name, card, 0,
> +    inter = grub_net_configure_by_dhcp_ack (card->name, card, 0,
>                                     (struct grub_net_bootp_packet *)
>                                     &pxe_mode->dhcp_ack,
>                                     sizeof (pxe_mode->dhcp_ack),
>                                     1, device, path);
> +
> +       /*
> +        * Bootstrap server ip address and file name maybe
> +        * come from a separate proxy DHCP server,
> +        * so check the proxy_offer DHCP packet
> +        *
> +        */
> +       if (inter && *path == NULL) {
> +               if (*device) {
> +                       grub_free(*device);
> +                       *device = NULL;
> +               }
> +
> +               grub_net_configure_by_proxy_offer(
> +                               (struct grub_net_bootp_packet 
> *)&pxe_mode->proxy_offer,

Please check ProxyOfferReceived as required by UEFI. I know we do not
do it currently but let's do it right at least in new code.

> +                               sizeof (pxe_mode->proxy_offer),
> +                               device,
> +                               path);
> +       }
> +

Well, this opens up the question about precedence of data from
different packets. With my patch (that abstracts away options
processing) we can actually simply pass both packets at once and fetch
IP information from (original) DHCPACK and other information from
proxy DHCPOFFER as required (falling back to DHCPACK if proxy is
NULL).

>      return;
>    }
>  }
> diff --git a/include/grub/net.h b/include/grub/net.h
> index b62643a..f107a23 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -433,6 +433,10 @@ enum
>      GRUB_NET_BOOTP_DOMAIN = 0x0f,
>      GRUB_NET_BOOTP_ROOT_PATH = 0x11,
>      GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
> +       GRUB_NET_DHCP_OVERLOAD = 0x34,
> +       GRUB_NET_DHCP_SERVER_ID = 0x36,
> +       GRUB_NET_DHCP_CLASS_ID = 0x3c,
> +       GRUB_NET_DHCP_BOOTFILE = 0x43,

Please use decimal option numbers for any new option you add. They are
defined as decimal in RFC and it makes it easier to cross-reference
with RFC later.