Re: [PATCH] bootp: add native DHCPv4 support

[Daniel Kiper]

On Fri, Jan 11, 2019 at 03:59:34PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <address@hidden>
>
> This patch adds support for native DHCPv4 and removes requirement for
> BOOTP compatibility support in DHCP server.
>
> There is no provision for selecting preferred server. We take the first
> OFFER and try to REQUEST configuration from it. If NAK was received,
> transaction is restarted, but if we hit timeout, configuration fails.
>
> It also handles pure BOOTP reply (detected by lack of DHCP message type
> option) and proceeds with configuration immedately. I could not test it
> because I do not have pure BOOTP server.
>
> Because we need access to DHCP options in multiple places now, it also
> factors out DHCP option processing, adding support for option overload.
>
> Timeout handling is now per-interface, with independent timeouts for
> both DISCOVER and REQUEST. It should make it more responsive if answer
> was delayed initially. Total timeout remains the same as originally, as
> well as backoff algorithm, but we poll in fixed 200ms ticks so notice
> (successful) reply earlier.
>
> Failure to send packet to interface now does not terminate command (and
> leaks memory) but rather simply ignores this interface and continues with
> remaining ones if present.
>
> Finally it adds net_dhcp alias with intention to deprecate net_bootp
> completely (it would be possible to make net_bootp to actually send BOOTP
> packet if someone thinks it is required).
>
> Features not implements:
>
> - DHCP server selection. We take first DHCPOFFER or BOOTPREPLY. No plans
> to implement.
>
> - DHCP option concatenation (RFC3396). I do not expect to hit it in real
> life and it rather complicates implementation, so let's wait for actual
> use case.
>
> - client identifier (RFC6842). So far we expect valid MAC address, which
> should be enough to uniquely identify client. It is also not clear how to
> generate unique client identifier if we ever need one.
>
> v2: change find_dhcp_option to use subscripts to avoid signed/unsigned
>     comparison warning.
>
>     Should fix "may be used uninitialized" warning (although I could not
>     reproduce it).
>

I think that we should put Andrei's credits here. Probably we cannot add
SOB without his approval. However, I am happy with anything else which
does something in the similar fashion.

> Signed-off-by: Andre Przywara <address@hidden>
>
> ---
> Hi,
>
> this patch is a rebased version of Andrei's work from 2016 [1][2][3].
> We have still this issue here where our company DHCP server does not
> support the BOOTP protocol, so grub doesn't get a valid IP address.
>
> I took v2 from Andrei of the list, and fixed the minor conflicts during the
> rebase. I can confirm that this patch makes the difference between DHCP
> working and not:
> grub-master> net_bootp
> error: couldn't autoconfigure efinet0.
> ...
> grub-patched> net_bootp
> grub-patched> net_ls_addr
> efinet0:dhcp 00:11:22:33:44:55 10.1.23.42
>
> Also back in the days there were concerns about regressing BOOTP-only
> servers. So I took the CMU bootpd[4], disabled the DHCP extensions at
> compile time and still got IP addresses in both cases (patched/unpatched).
> dhclient on Linux on the other hand was not happy with that server and

It seems to me that two sentences are glued here...

> ignored the reply. I guess this is as close to a "BOOTP only server" as
> we can get in 2019.

I am OK with that.

> I would be very happy if we can get this merged now.
>
> Please let me know if you need more information or any help on this.

Could you merge your comment with the commit message above? Please just
refer to the current situation. If something historic should be referred
please say clearly that it does not apply for today.

Is it possible to split this patch to smaller pieces?

Some nitpicks below...

> Thanks!
> Andre.
>
> P.S. The original patch didn't have a Signed-off-by:, so I kept it that
> way. Added mine for legal reason, feel free to drop it.
>
> [1] http://lists.gnu.org/archive/html/grub-devel/2016-01/msg00035.html
> [2] http://lists.gnu.org/archive/html/grub-devel/2016-03/msg00276.html
> [3] http://lists.gnu.org/archive/html/grub-devel/2016-04/msg00066.html
> [4] https://packages.debian.org/jessie/bootp
>
>  grub-core/net/bootp.c | 759 ++++++++++++++++++++++++++++--------------
>  grub-core/net/ip.c    |   2 +-
>  include/grub/net.h    |  14 +-
>  3 files changed, 528 insertions(+), 247 deletions(-)
>
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index 9e2fdb795..890df715b 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c

If we add DHCP support here then I think we should change file name in
the following patch.

> @@ -25,25 +25,107 @@
>  #include <grub/net/udp.h>
>  #include <grub/datetime.h>
>
> -static void
> -parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
> +struct grub_dhcp_discover_options
> +  {

Drop spaces before "{".

> +  grub_uint8_t magic[4];
> +  struct
> +    {

Drop two spaces starting from above line...

> +      grub_uint8_t code;
> +      grub_uint8_t len;
> +      grub_uint8_t data;
> +    } GRUB_PACKED  message_type;
> +    grub_uint8_t end;
> +  } GRUB_PACKED;

...up to here. Same thing for structs below.

> +
> +struct grub_dhcp_request_options
> +  {
> +    grub_uint8_t magic[4];
> +    struct
> +      {
> +     grub_uint8_t code;
> +     grub_uint8_t len;
> +     grub_uint8_t data;
> +      } GRUB_PACKED  message_type;
> +    struct
> +      {
> +     grub_uint8_t type;
> +     grub_uint8_t len;
> +     grub_uint32_t data;
> +      } GRUB_PACKED  server_identifier;
> +    struct
> +      {
> +     grub_uint8_t type;
> +     grub_uint8_t len;
> +     grub_uint32_t data;
> +      } GRUB_PACKED  requested_ip;
> +    struct
> +      {
> +     grub_uint8_t type;
> +     grub_uint8_t len;
> +     grub_uint8_t data[7];
> +      } GRUB_PACKED  parameter_request;
> +    grub_uint8_t end;
> +  } GRUB_PACKED;
> +
> +enum
> +  {

Drop two spaces starting from above line...

> +    GRUB_DHCP_MESSAGE_UNKNOWN,
> +    GRUB_DHCP_MESSAGE_DISCOVER,
> +    GRUB_DHCP_MESSAGE_OFFER,
> +    GRUB_DHCP_MESSAGE_REQUEST,
> +    GRUB_DHCP_MESSAGE_DECLINE,
> +    GRUB_DHCP_MESSAGE_ACK,
> +    GRUB_DHCP_MESSAGE_NAK,
> +    GRUB_DHCP_MESSAGE_RELEASE,
> +    GRUB_DHCP_MESSAGE_INFORM,
> +  };

...up to here.

> +enum
> +  {
> +    GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
> +    GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
> +  };

Ditto.

> +
> +#define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
> +#define OFFSET_OF(x, y) ((grub_size_t)((grub_uint8_t *)((y)->x) - 
> (grub_uint8_t *)(y)))

Please define this macro behind GRUB_DHCP_MAX_PACKET_TIMEOUT.

> +
> +/* Max timeout when waiting for BOOTP/DHCP reply */
> +#define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
> +
> +static const void *
> +find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,

s/grub_net_bootp_packet/grub_net_dhcp_packet/?

And in general should not we change bootp with dhcp everywhere after this patch?

> +              grub_uint8_t opt_code, grub_uint8_t *opt_len)
>  {
> -  const grub_uint8_t *ptr, *ptr0;
> +  const grub_uint8_t *ptr;
> +  grub_uint8_t overload = 0;
> +  int end = 0;
> +  grub_size_t i;
> +
> +  if (opt_len)
> +    *opt_len = 0;
> +
> +  /* Magic */
> +  if (size < sizeof (*bp) + sizeof (grub_uint32_t))
> +    goto out;
>
> -  ptr = ptr0 = vend;
> +  ptr = (grub_uint8_t *) (bp + 1);

Please add one line comment where (bp + 1) points to.

>    if (ptr[0] != GRUB_NET_BOOTP_RFC1048_MAGIC_0
>        || ptr[1] != GRUB_NET_BOOTP_RFC1048_MAGIC_1
>        || ptr[2] != GRUB_NET_BOOTP_RFC1048_MAGIC_2
>        || ptr[3] != GRUB_NET_BOOTP_RFC1048_MAGIC_3)
> -    return;
> -  ptr = ptr + sizeof (grub_uint32_t);
> -  while (ptr - ptr0 < limit)
> +    goto out;
> +
> +  size -= sizeof (*bp);
> +  i = sizeof (grub_uint32_t);
> +
> +again:
> +  while (i < size)
>      {
>        grub_uint8_t tagtype;
>        grub_uint8_t taglength;
>
> -      tagtype = *ptr++;
> +      tagtype = ptr[i++];
>
>        /* Pad tag.  */
>        if (tagtype == GRUB_NET_BOOTP_PAD)
> @@ -51,84 +133,77 @@ parse_dhcp_vendor (const char *name, const void *vend, 
> int limit, int *mask)
>
>        /* End tag.  */
>        if (tagtype == GRUB_NET_BOOTP_END)
> -     return;
> +     {
> +       end = 1;
> +       break;
> +     }
> +
> +      if (i >= size)
> +     goto out;
>
> -      taglength = *ptr++;
> +      taglength = ptr[i++];
> +      if (i + taglength >= size)
> +     goto out;
>
> -      switch (tagtype)
> +      /* FIXME RFC 3396 options concatentation */
> +      if (tagtype == opt_code)
>       {
> -     case GRUB_NET_BOOTP_NETMASK:
> -       if (taglength == 4)
> -         {
> -           int i;
> -           for (i = 0; i < 32; i++)
> -             if (!(ptr[i / 8] & (1 << (7 - (i % 8)))))
> -               break;
> -           *mask = i;
> -         }
> -       break;
> +       if (opt_len)
> +         *opt_len = taglength;
> +       return &ptr[i];
> +     }
>
> -     case GRUB_NET_BOOTP_ROUTER:
> -       if (taglength == 4)
> -         {
> -           grub_net_network_level_netaddress_t target;
> -           grub_net_network_level_address_t gw;
> -           char *rname;
> -
> -           target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
> -           target.ipv4.base = 0;
> -           target.ipv4.masksize = 0;
> -           gw.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
> -           grub_memcpy (&gw.ipv4, ptr, sizeof (gw.ipv4));
> -           rname = grub_xasprintf ("%s:default", name);
> -           if (rname)
> -             grub_net_add_route_gw (rname, target, gw, NULL);
> -           grub_free (rname);
> -         }
> -       break;
> -     case GRUB_NET_BOOTP_DNS:
> +      if (tagtype == GRUB_NET_DHCP_OVERLOAD && taglength == 1)
> +     overload = ptr[i];
> +
> +      i += taglength;
> +    }
> +
> +    /* RFC2131, 4.1, 23ff:
> +       If the options in a DHCP message extend into the 'sname' and 'file'
> +       fields, the 'option overload' option MUST appear in the 'options'
> +       field, with value 1, 2 or 3, as specified in RFC 1533.  If the
> +       'option overload' option is present in the 'options' field, the
> +       options in the 'options' field MUST be terminated by an 'end' option,
> +       and MAY contain one or more 'pad' options to fill the options field.
> +       The options in the 'sname' and 'file' fields (if in use as indicated
> +       by the 'options overload' option) MUST begin with the first octet of
> +       the field, MUST be terminated by an 'end' option, and MUST be
> +       followed by 'pad' options to fill the remainder of the field.  Any
> +       individual option in the 'options', 'sname' and 'file' fields MUST be
> +       entirely contained in that field.  The options in the 'options' field
> +       MUST be interpreted first, so that any 'option overload' options may
> +       be interpreted.  The 'file' field MUST be interpreted next (if the
> +       'option overload' option indicates that the 'file' field contains
> +       DHCP options), followed by the 'sname' field.
> +
> +       FIXME: We do not explicitly check for trailing 'pad' options here.
> +     */

Could you convert all multiline comments to the following format?

/*
 * RFC2131, 4.1, 23ff:
 * If the options in a DHCP message extend into the 'sname' and 'file'
 * fields, the 'option overload' option MUST appear in the 'options'
 ...
 *
 * FIXME: We do not explicitly check for trailing 'pad' options here.
 */

> +    if (end)
> +      {
> +     end = 0;
> +     if (overload & GRUB_DHCP_OPT_OVERLOAD_FILE)
>         {
> -         int i;
> -         for (i = 0; i < taglength / 4; i++)
> -           {
> -             struct grub_net_network_level_address s;
> -             s.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
> -             s.ipv4 = grub_get_unaligned32 (ptr);
> -             s.option = DNS_OPTION_PREFER_IPV4;
> -             grub_net_add_dns_server (&s);
> -             ptr += 4;
> -           }
> +         overload &= ~GRUB_DHCP_OPT_OVERLOAD_FILE;
> +         ptr = (grub_uint8_t *) &bp->boot_file[0];
> +         size = sizeof (bp->boot_file);
> +         i = 0;
> +         goto again;
>         }
> -       continue;
> -     case GRUB_NET_BOOTP_HOSTNAME:
> -          grub_env_set_net_property (name, "hostname", (const char *) ptr,
> -                                     taglength);
> -          break;
> -
> -     case GRUB_NET_BOOTP_DOMAIN:
> -          grub_env_set_net_property (name, "domain", (const char *) ptr,
> -                                     taglength);
> -          break;
> -
> -     case GRUB_NET_BOOTP_ROOT_PATH:
> -          grub_env_set_net_property (name, "rootpath", (const char *) ptr,
> -                                     taglength);
> -          break;
> -
> -     case GRUB_NET_BOOTP_EXTENSIONS_PATH:
> -          grub_env_set_net_property (name, "extensionspath", (const char *) 
> ptr,
> -                                     taglength);
> -          break;
> -
> -       /* If you need any other options please contact GRUB
> -          development team.  */
> -     }
>
> -      ptr += taglength;
> -    }
> -}
> +     if (overload & GRUB_DHCP_OPT_OVERLOAD_SNAME)
> +       {
> +         overload &= ~GRUB_DHCP_OPT_OVERLOAD_SNAME;
> +         ptr = (grub_uint8_t *) &bp->server_name[0];
> +         size = sizeof (bp->server_name);
> +         i = 0;
> +         goto again;
> +       }
> +      }
>
> -#define OFFSET_OF(x, y) ((grub_size_t)((grub_uint8_t *)((y)->x) - 
> (grub_uint8_t *)(y)))

This is the move. I think that this should be in separate patch.

> +out:
> +  return 0;
> +}

[...]

> +static grub_err_t
> +send_dhcp_packet (struct grub_net_network_level_interface *iface)
> +{
> +  grub_err_t err;
> +  struct grub_net_bootp_packet *pack;
> +  struct grub_datetime date;
> +  grub_int32_t t = 0;
> +  struct grub_net_buff *nb;
> +  struct udphdr *udph;
> +  grub_net_network_level_address_t target;
> +  grub_net_link_level_address_t ll_target;
> +
> +  static struct grub_dhcp_discover_options discover_options =
> +    {
> +      {
> +     GRUB_NET_BOOTP_RFC1048_MAGIC_0,
> +     GRUB_NET_BOOTP_RFC1048_MAGIC_1,
> +     GRUB_NET_BOOTP_RFC1048_MAGIC_2,
> +     GRUB_NET_BOOTP_RFC1048_MAGIC_3,
> +      },
> +      {
> +     GRUB_NET_DHCP_MESSAGE_TYPE,
> +     sizeof (discover_options.message_type.data),
> +     GRUB_DHCP_MESSAGE_DISCOVER,
> +      },
> +      GRUB_NET_BOOTP_END,
> +    };
> +
> +  static struct grub_dhcp_request_options request_options =
> +    {
> +      {
> +     GRUB_NET_BOOTP_RFC1048_MAGIC_0,
> +     GRUB_NET_BOOTP_RFC1048_MAGIC_1,
> +     GRUB_NET_BOOTP_RFC1048_MAGIC_2,
> +     GRUB_NET_BOOTP_RFC1048_MAGIC_3,
> +      },
> +      {
> +     GRUB_NET_DHCP_MESSAGE_TYPE,
> +     sizeof (request_options.message_type.data),
> +     GRUB_DHCP_MESSAGE_REQUEST,
> +      },
> +      {
> +     GRUB_NET_DHCP_SERVER_IDENTIFIER,
> +     sizeof (request_options.server_identifier.data),
> +     0,
> +      },
> +      {
> +     GRUB_NET_DHCP_REQUESTED_IP_ADDRESS,
> +     sizeof (request_options.requested_ip.data),
> +     0,
> +      },
> +      {
> +     GRUB_NET_DHCP_PARAMETER_REQUEST_LIST,
> +     sizeof (request_options.parameter_request.data),
> +     {
> +       GRUB_NET_BOOTP_NETMASK,
> +       GRUB_NET_BOOTP_ROUTER,
> +       GRUB_NET_BOOTP_DNS,
> +       GRUB_NET_BOOTP_DOMAIN,
> +       GRUB_NET_BOOTP_HOSTNAME,
> +       GRUB_NET_BOOTP_ROOT_PATH,
> +       GRUB_NET_BOOTP_EXTENSIONS_PATH,
> +     },
> +      },
> +      GRUB_NET_BOOTP_END,
> +    };
> +
> +  COMPILE_TIME_ASSERT (sizeof (discover_options) <= 
> GRUB_BOOTP_MAX_OPTIONS_SIZE);
> +  COMPILE_TIME_ASSERT (sizeof (request_options) <= 
> GRUB_BOOTP_MAX_OPTIONS_SIZE);
> +
> +  nb = grub_netbuff_alloc (sizeof (*pack) + GRUB_BOOTP_MAX_OPTIONS_SIZE + 
> 128);

Please define 128 as a constant.

> +  if (!nb)
> +    return grub_errno;
> +
> +  err = grub_netbuff_reserve (nb, sizeof (*pack) + 
> GRUB_BOOTP_MAX_OPTIONS_SIZE + 128);
> +  if (err)
> +    goto out;
> +
> +  err = grub_netbuff_push (nb, GRUB_BOOTP_MAX_OPTIONS_SIZE);
> +  if (err)
> +    goto out;
> +
> +  grub_memset (nb->data, 0, GRUB_BOOTP_MAX_OPTIONS_SIZE);
> +  if (!iface->srv_id)
> +    {
> +      grub_memcpy (nb->data, &discover_options, sizeof (discover_options));
> +    }
> +  else
> +    {
> +      struct grub_dhcp_request_options *ro = (struct 
> grub_dhcp_request_options *) nb->data;
> +
> +      grub_memcpy (nb->data, &request_options, sizeof (request_options));
> +      /* my_ip and srv_id are stored in network order so do not need 
> conversion. */
> +      grub_set_unaligned32 (&ro->server_identifier.data, iface->srv_id);
> +      grub_set_unaligned32 (&ro->requested_ip.data, iface->my_ip);
> +    }
> +
> +  err = grub_netbuff_push (nb, sizeof (*pack));
> +  if (err)
> +    goto out;
> +
> +  pack = (void *) nb->data;
> +  grub_memset (pack, 0, sizeof (*pack));
> +  pack->opcode = 1;
> +  pack->hw_type = 1;
> +  pack->hw_len = 6;

Constants instead of plain numbers? Or at least comments what these numbers 
mean.

> +  err = grub_get_datetime (&date);
> +  if (err || !grub_datetime2unixtime (&date, &t))
> +    {
> +      grub_errno = GRUB_ERR_NONE;
> +      t = 0;
> +    }
> +  pack->seconds = grub_cpu_to_be16 (t);
> +  if (!iface->srv_id)
> +    iface->xid = pack->ident = grub_cpu_to_be32 (t);
> +  else
> +    pack->ident = iface->xid;
> +
> +  grub_memcpy (&pack->mac_addr, &iface->hwaddress.mac, 6);
> +
> +  grub_netbuff_push (nb, sizeof (*udph));
> +
> +  udph = (struct udphdr *) nb->data;
> +  udph->src = grub_cpu_to_be16_compile_time (68);
> +  udph->dst = grub_cpu_to_be16_compile_time (67);

OK, these are port numbers but same as above.

> +  udph->chksum = 0;
> +  udph->len = grub_cpu_to_be16 (nb->tail - nb->data);
> +  target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
> +  target.ipv4 = 0xffffffff;

Ditto.

Daniel