[PATCH v2 0/9] net: bootp: add native DHCPv4 support

[PATCH v2 0/9] net: bootp: add native DHCPv4 support

[Andre Przywara]

This is a split-up of the previous all-in-one patch from Andrei,
to teach grub proper DHCP manners.
This split is based on my limited understanding of BOOTP/DHCP and grub,
but it should be an improvement over the single patch version anyway.
All patches compile and work, although only patch 8/9 actually enables
DHCP handling and makes grub work with DHCP servers without BOOTP
compatibility.
The series is build to first introduce some refactoring (patches 1-5),
than adding DHCP functionality in patches 6-8. Patch 9 adds the somewhat
optional net_dhcp command alias, which is identical to net_bootp.
I also fixed the white space and comment style options.

Original commit message:
----------------------------------
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).

---------
This 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
ignored the reply. I guess this is as close to a "BOOTP only server" as
we can get in 2019.

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.

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

Andrei Borzenkov (9):
  net: dhcp: remove dead code
  net: dhcp: replace parse_dhcp_vendor() with find_dhcp_option()
  net: dhcp: refactor DHCP packet transmission into separate function
  net: dhcp: make grub_net_process_dhcp take an interface
  net: dhcp: introduce per-interface timeout
  net: dhcp: use DHCP options for name and bootfile
  net: dhcp: allow receiving DHCP OFFER and ACK packets
  net: dhcp: actually send out DHCPv4 DISCOVER and REQUEST messages
  net: dhcp: add explicit net_dhcp command

 grub-core/net/bootp.c | 774 +++++++++++++++++++++++++++++-------------
 grub-core/net/ip.c    |   2 +-
 include/grub/net.h    |  14 +-
 3 files changed, 545 insertions(+), 245 deletions(-)

-- 
2.17.1

[PATCH v2 1/9] net: dhcp: remove dead code

[Andre Przywara]

From: Andrei Borzenkov <arvidjaar@gmail.com>

The comment is right, the "giaddr" fields holds the IP address of the
BOOTP relay, not a general purpose router address.
Just remove the commented code, archeologists can find it in the git
history.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 grub-core/net/bootp.c | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 9e2fdb795..c92dfbd3a 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -161,35 +161,6 @@ grub_net_configure_by_dhcp_ack (const char *name,
   if (!inter)
     return 0;
 
-#if 0
-  /* This is likely based on misunderstanding. gateway_ip refers to
-     address of BOOTP relay and should not be used after BOOTP transaction
-     is complete.
-     See RFC1542, 3.4 Interpretation of the 'giaddr' field
-   */
-  if (bp->gateway_ip)
-    {
-      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 = bp->server_ip;
-      target.ipv4.masksize = 32;
-      gw.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
-      gw.ipv4 = bp->gateway_ip;
-      rname = grub_xasprintf ("%s:gw", name);
-      if (rname)
-	grub_net_add_route_gw (rname, target, gw);
-      grub_free (rname);
-
-      target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
-      target.ipv4.base = bp->gateway_ip;
-      target.ipv4.masksize = 32;
-      grub_net_add_route (name, target, inter);
-    }
-#endif
-
   if (size > OFFSET_OF (boot_file, bp))
     grub_env_set_net_property (name, "boot_file", bp->boot_file,
                                sizeof (bp->boot_file));
-- 
2.17.1

[PATCH v2 2/9] net: dhcp: replace parse_dhcp_vendor() with find_dhcp_option()

[Andre Przywara]

From: Andrei Borzenkov <arvidjaar@gmail.com>

For proper DHCP support we will need to parse DHCP options from a packet
more often and at various places.

Refactor the option parsing into a new function, which will scan a
packet to find *a particular* option field.
Use that new function in places where we were dealing with DHCP options
before.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 grub-core/net/bootp.c | 285 ++++++++++++++++++++++++++----------------
 include/grub/net.h    |   1 +
 2 files changed, 176 insertions(+), 110 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index c92dfbd3a..c11038483 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -25,25 +25,50 @@
 #include <grub/net/udp.h>
 #include <grub/datetime.h>
 
-static void
-parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
+enum
 {
-  const grub_uint8_t *ptr, *ptr0;
+  GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
+  GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
+};
 
-  ptr = ptr0 = vend;
+static const void *
+find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
+		  grub_uint8_t opt_code, grub_uint8_t *opt_len)
+{
+  const grub_uint8_t *ptr;
+  grub_uint8_t overload = 0;
+  int end = 0;
+  grub_size_t i;
+
+  if (opt_len)
+    *opt_len = 0;
+
+  /* Is the packet big enough to hold at least the magic cookie? */
+  if (size < sizeof (*bp) + sizeof (grub_uint32_t))
+    goto out;
+
+  /*
+   * Pointer arithmetic to point behind the common stub packet, where
+   * the options start.
+   */
+  ptr = (grub_uint8_t *) (bp + 1);
 
   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,81 +76,76 @@ parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
 
       /* End tag.  */
       if (tagtype == GRUB_NET_BOOTP_END)
-	return;
-
-      taglength = *ptr++;
-
-      switch (tagtype)
 	{
-	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;
-	    }
+	  end = 1;
 	  break;
+	}
 
-	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:
-	  {
-	    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;
-	      }
-	  }
-	  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.  */
+      if (i >= size)
+	goto out;
+
+      taglength = ptr[i++];
+      if (i + taglength >= size)
+	goto out;
+
+      /* FIXME RFC 3396 options concatentation */
+      if (tagtype == opt_code)
+	{
+	  if (opt_len)
+	    *opt_len = taglength;
+	  return &ptr[i];
 	}
 
-      ptr += taglength;
+      if (tagtype == GRUB_NET_DHCP_OVERLOAD && taglength == 1)
+	overload = ptr[i];
+
+      i += taglength;
     }
+
+  if (!end)
+    return 0;
+
+  /* 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.
+   */
+  end = 0;
+  if (overload & GRUB_DHCP_OPT_OVERLOAD_FILE)
+  {
+    overload &= ~GRUB_DHCP_OPT_OVERLOAD_FILE;
+    ptr = (grub_uint8_t *) &bp->boot_file[0];
+    size = sizeof (bp->boot_file);
+    i = 0;
+    goto again;
+  }
+
+  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;
+  }
+
+out:
+  return 0;
 }
 
 #define OFFSET_OF(x, y) ((grub_size_t)((grub_uint8_t *)((y)->x) - (grub_uint8_t *)(y)))
@@ -143,6 +163,8 @@ grub_net_configure_by_dhcp_ack (const char *name,
   struct grub_net_network_level_interface *inter;
   int mask = -1;
   char server_ip[sizeof ("xxx.xxx.xxx.xxx")];
+  const grub_uint8_t *opt;
+  grub_uint8_t opt_len;
 
   addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
   addr.ipv4 = bp->your_ip;
@@ -225,9 +247,69 @@ grub_net_configure_by_dhcp_ack (const char *name,
 	    **path = 0;
 	}
     }
-  if (size > OFFSET_OF (vendor, bp))
-    parse_dhcp_vendor (name, &bp->vendor, size - OFFSET_OF (vendor, bp), &mask);
+
+  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_NETMASK, &opt_len);
+  if (opt && opt_len == 4)
+    {
+      int i;
+      for (i = 0; i < 32; i++)
+	if (!(opt[i / 8] & (1 << (7 - (i % 8)))))
+	  break;
+      mask = i;
+    }
   grub_net_add_ipv4_local (inter, mask);
+
+  /* We do not implement dead gateway detection and the first entry SHOULD
+     be preferred one */
+  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_ROUTER, &opt_len);
+  if (opt && opt_len && !(opt_len & 3))
+    {
+      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;
+      gw.ipv4 = grub_get_unaligned32 (opt);
+      rname = grub_xasprintf ("%s:default", name);
+      if (rname)
+	grub_net_add_route_gw (rname, target, gw, 0);
+      grub_free (rname);
+    }
+
+  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_DNS, &opt_len);
+  if (opt && opt_len && !(opt_len & 3))
+    {
+      int i;
+      for (i = 0; i < opt_len / 4; i++)
+	{
+	  struct grub_net_network_level_address s;
+
+	  s.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
+	  s.ipv4 = grub_get_unaligned32 (opt);
+	  s.option = DNS_OPTION_PREFER_IPV4;
+	  grub_net_add_dns_server (&s);
+	  opt += 4;
+	}
+    }
+
+  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_HOSTNAME, &opt_len);
+  if (opt && opt_len)
+    grub_env_set_net_property (name, "hostname", (const char *) opt, opt_len);
+
+  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_DOMAIN, &opt_len);
+  if (opt && opt_len)
+    grub_env_set_net_property (name, "domain", (const char *) opt, opt_len);
+
+  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_ROOT_PATH, &opt_len);
+  if (opt && opt_len)
+    grub_env_set_net_property (name, "rootpath", (const char *) opt, opt_len);
+
+  opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_EXTENSIONS_PATH, &opt_len);
+  if (opt && opt_len)
+    grub_env_set_net_property (name, "extensionspath", (const char *) opt, opt_len);
   
   inter->dhcp_ack = grub_malloc (size);
   if (inter->dhcp_ack)
@@ -286,8 +368,8 @@ grub_cmd_dhcpopt (struct grub_command *cmd __attribute__ ((unused)),
 		  int argc, char **args)
 {
   struct grub_net_network_level_interface *inter;
-  int num;
-  grub_uint8_t *ptr;
+  unsigned num;
+  const grub_uint8_t *ptr;
   grub_uint8_t taglength;
 
   if (argc < 4)
@@ -305,44 +387,27 @@ grub_cmd_dhcpopt (struct grub_command *cmd __attribute__ ((unused)),
   if (!inter->dhcp_ack)
     return grub_error (GRUB_ERR_IO, N_("no DHCP info found"));
 
-  if (inter->dhcp_acklen <= OFFSET_OF (vendor, inter->dhcp_ack))
-    return grub_error (GRUB_ERR_IO, N_("no DHCP options found"));
-
-  num = grub_strtoul (args[2], 0, 0);
-  if (grub_errno)
-    return grub_errno;
-
   ptr = inter->dhcp_ack->vendor;
 
-  if (ptr[0] != GRUB_NET_BOOTP_RFC1048_MAGIC_0
+  /* This duplicates check in find_dhcp_option to preserve previous error return */
+  if (inter->dhcp_acklen < OFFSET_OF (vendor, inter->dhcp_ack) + sizeof (grub_uint32_t)
+      || 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 grub_error (GRUB_ERR_IO, N_("no DHCP options found"));
-  ptr = ptr + sizeof (grub_uint32_t);
-  while (1)
-    {
-      grub_uint8_t tagtype;
-
-      if (ptr >= ((grub_uint8_t *) inter->dhcp_ack) + inter->dhcp_acklen)
-	return grub_error (GRUB_ERR_IO, N_("no DHCP option %d found"), num);
-
-      tagtype = *ptr++;
 
-      /* Pad tag.  */
-      if (tagtype == 0)
-	continue;
+  num = grub_strtoul (args[2], 0, 0);
+  if (grub_errno)
+    return grub_errno;
 
-      /* End tag.  */
-      if (tagtype == 0xff)
-	return grub_error (GRUB_ERR_IO, N_("no DHCP option %d found"), num);
+  /* Exclude PAD (0) and END (255) option codes */
+  if (num == 0 || num > 254)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid DHCP option code"));
 
-      taglength = *ptr++;
-	
-      if (tagtype == num)
-	break;
-      ptr += taglength;
-    }
+  ptr = find_dhcp_option (inter->dhcp_ack, inter->dhcp_acklen, num, &taglength);
+  if (!ptr)
+    return grub_error (GRUB_ERR_IO, N_("no DHCP option %u found"), num);
 
   if (grub_strcmp (args[3], "string") == 0)
     {
diff --git a/include/grub/net.h b/include/grub/net.h
index 1096b2432..0c7286bd2 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -457,6 +457,7 @@ enum
     GRUB_NET_BOOTP_DOMAIN = 0x0f,
     GRUB_NET_BOOTP_ROOT_PATH = 0x11,
     GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
+    GRUB_NET_DHCP_OVERLOAD = 52,
     GRUB_NET_BOOTP_END = 0xff
   };
 
-- 
2.17.1

[PATCH v2 3/9] net: dhcp: refactor DHCP packet transmission into separate function

[Andre Przywara]

From: Andrei Borzenkov <arvidjaar@gmail.com>

In contrast to BOOTP, DHCP uses a 4-way handshake, so requires to send
packets more often.

Refactor the generation and sending of the BOOTREQUEST packet into a
separate function, so that future code can more easily reuse this.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 grub-core/net/bootp.c | 147 +++++++++++++++++++++++-------------------
 1 file changed, 82 insertions(+), 65 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index c11038483..fd56de907 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -31,6 +31,8 @@ enum
   GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
 };
 
+#define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
+
 static const void *
 find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
 		  grub_uint8_t opt_code, grub_uint8_t *opt_len)
@@ -323,6 +325,77 @@ grub_net_configure_by_dhcp_ack (const char *name,
   return inter;
 }
 
+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;
+
+  nb = grub_netbuff_alloc (sizeof (*pack) + GRUB_BOOTP_MAX_OPTIONS_SIZE + 128);
+  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);
+
+  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;
+  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);
+  pack->ident = grub_cpu_to_be32 (t);
+
+  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);
+  udph->chksum = 0;
+  udph->len = grub_cpu_to_be16 (nb->tail - nb->data);
+  target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
+  target.ipv4 = 0xffffffff;
+  err = grub_net_link_layer_resolve (iface, &target, &ll_target);
+  if (err)
+    goto out;
+
+  udph->chksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_UDP,
+						 &iface->address,
+						 &target);
+
+  err = grub_net_send_ip_packet (iface, &target, &ll_target, nb,
+				 GRUB_NET_IP_UDP);
+
+out:
+  grub_netbuff_free (nb);
+  return err;
+}
+
 void
 grub_net_process_dhcp (struct grub_net_buff *nb,
 		       struct grub_net_card *card)
@@ -479,6 +552,7 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
   unsigned j = 0;
   int interval;
   grub_err_t err;
+  unsigned i;
 
   FOR_NET_CARDS (card)
   {
@@ -507,7 +581,6 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
     card->num_ifaces++;
     if (!ifaces[j].name)
       {
-	unsigned i;
 	for (i = 0; i < j; i++)
 	  grub_free (ifaces[i].name);
 	grub_free (ifaces);
@@ -525,83 +598,27 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
   ifaces[0].prev = &grub_net_network_level_interfaces;
   for (interval = 200; interval < 10000; interval *= 2)
     {
-      int done = 0;
+      int need_poll = 0;
       for (j = 0; j < ncards; j++)
 	{
-	  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;
-
 	  if (!ifaces[j].prev)
 	    continue;
-	  nb = grub_netbuff_alloc (sizeof (*pack) + 64 + 128);
-	  if (!nb)
-	    {
-	      grub_netbuff_free (nb);
-	      return grub_errno;
-	    }
-	  err = grub_netbuff_reserve (nb, sizeof (*pack) + 64 + 128);
-	  if (err)
-	    {
-	      grub_netbuff_free (nb);
-	      return err;
-	    }
-	  err = grub_netbuff_push (nb, sizeof (*pack) + 64);
+
+	  err = send_dhcp_packet (&ifaces[j]);
 	  if (err)
 	    {
-	      grub_netbuff_free (nb);
-	      return err;
+	      grub_print_error ();
+	      continue;
 	    }
-	  pack = (void *) nb->data;
-	  done = 1;
-	  grub_memset (pack, 0, sizeof (*pack) + 64);
-	  pack->opcode = 1;
-	  pack->hw_type = 1;
-	  pack->hw_len = 6;
-	  err = grub_get_datetime (&date);
-	  if (err || !grub_datetime2unixtime (&date, &t))
-	    {
-	      grub_errno = GRUB_ERR_NONE;
-	      t = 0;
-	    }
-	  pack->ident = grub_cpu_to_be32 (t);
-	  pack->seconds = grub_cpu_to_be16 (t);
-
-	  grub_memcpy (&pack->mac_addr, &ifaces[j].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);
-	  udph->chksum = 0;
-	  udph->len = grub_cpu_to_be16 (nb->tail - nb->data);
-	  target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
-	  target.ipv4 = 0xffffffff;
-	  err = grub_net_link_layer_resolve (&ifaces[j], &target, &ll_target);
-	  if (err)
-	    return err;
-
-	  udph->chksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_UDP,
-							 &ifaces[j].address,
-							 &target);
-
-	  err = grub_net_send_ip_packet (&ifaces[j], &target, &ll_target, nb,
-					 GRUB_NET_IP_UDP);
-	  grub_netbuff_free (nb);
-	  if (err)
-	    return err;
+	  need_poll = 1;
 	}
-      if (!done)
+      if (!need_poll)
 	break;
       grub_net_poll_cards (interval, 0);
     }
 
   err = GRUB_ERR_NONE;
+
   for (j = 0; j < ncards; j++)
     {
       grub_free (ifaces[j].name);
-- 
2.17.1

[PATCH v2 4/9] net: dhcp: make grub_net_process_dhcp take an interface

[Andre Przywara]

From: Andrei Borzenkov <arvidjaar@gmail.com>

Change the interface of the function dealing with incoming BOOTP packets
to take an interface instead of a card, to allow more fine per-interface
state (timeout, handshake state) later on.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 grub-core/net/bootp.c | 29 ++++++++++++++++-------------
 grub-core/net/ip.c    |  2 +-
 include/grub/net.h    |  2 +-
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index fd56de907..573398aa4 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -396,12 +396,19 @@ out:
   return err;
 }
 
+/*
+ * This is called directly from net/ip.c:handle_dgram(), because those
+ * BOOTP/DHCP packets are a bit special due to their improper
+ * sender/receiver IP fields.
+ */
 void
 grub_net_process_dhcp (struct grub_net_buff *nb,
-		       struct grub_net_card *card)
+		       struct grub_net_network_level_interface *iface)
 {
   char *name;
-  struct grub_net_network_level_interface *inf;
+  struct grub_net_card *card = iface->card;
+  const struct grub_net_bootp_packet *bp = (const struct grub_net_bootp_packet *) nb->data;
+  grub_size_t size = nb->tail - nb->data;
 
   name = grub_xasprintf ("%s:dhcp", card->name);
   if (!name)
@@ -409,22 +416,18 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
       grub_print_error ();
       return;
     }
-  grub_net_configure_by_dhcp_ack (name, card,
-				  0, (const struct grub_net_bootp_packet *) nb->data,
-				  (nb->tail - nb->data), 0, 0, 0);
+  grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
   grub_free (name);
   if (grub_errno)
     grub_print_error ();
   else
     {
-      FOR_NET_NETWORK_LEVEL_INTERFACES(inf)
-	if (grub_memcmp (inf->name, card->name, grub_strlen (card->name)) == 0
-	    && grub_memcmp (inf->name + grub_strlen (card->name),
-			    ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
-	  {
-	    grub_net_network_level_interface_unregister (inf);
-	    break;
-	  }
+      if (grub_memcmp (iface->name, card->name, grub_strlen (card->name)) == 0
+	  && grub_memcmp (iface->name + grub_strlen (card->name),
+			  ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
+	{
+	  grub_net_network_level_interface_unregister (iface);
+	}
     }
 }
 
diff --git a/grub-core/net/ip.c b/grub-core/net/ip.c
index 7c95cc746..ea5edf8f1 100644
--- a/grub-core/net/ip.c
+++ b/grub-core/net/ip.c
@@ -279,7 +279,7 @@ handle_dgram (struct grub_net_buff *nb,
 	      && grub_memcmp (inf->hwaddress.mac, &bootp->mac_addr,
 			      sizeof (inf->hwaddress.mac)) == 0)
 	    {
-	      grub_net_process_dhcp (nb, inf->card);
+	      grub_net_process_dhcp (nb, inf);
 	      grub_netbuff_free (nb);
 	      return GRUB_ERR_NONE;
 	    }
diff --git a/include/grub/net.h b/include/grub/net.h
index 0c7286bd2..3f649d753 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -475,7 +475,7 @@ grub_net_add_ipv4_local (struct grub_net_network_level_interface *inf,
 
 void
 grub_net_process_dhcp (struct grub_net_buff *nb,
-		       struct grub_net_card *card);
+		       struct grub_net_network_level_interface *iface);
 
 int
 grub_net_hwaddr_cmp (const grub_net_link_level_address_t *a,
-- 
2.17.1

[PATCH v2 5/9] net: dhcp: introduce per-interface timeout

[Andre Przywara]

From: Andrei Borzenkov <arvidjaar@gmail.com>

Currently we have a global timeout for all network cards in the
BOOTP/DHCP discovery process.

Make this timeout a per-interface one, so better accommodate the
upcoming 4-way DHCP handshake and to also cover the lease time limit a
DHCP offer will come with.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 grub-core/net/bootp.c | 38 ++++++++++++++++++++++++++++++++++----
 include/grub/net.h    |  2 ++
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 573398aa4..42117b72d 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -33,6 +33,9 @@ enum
 
 #define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
 
+/* 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,
 		  grub_uint8_t opt_code, grub_uint8_t *opt_len)
@@ -553,7 +556,6 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
   struct grub_net_network_level_interface *ifaces;
   grub_size_t ncards = 0;
   unsigned j = 0;
-  int interval;
   grub_err_t err;
   unsigned i;
 
@@ -592,6 +594,7 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
     ifaces[j].address.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_DHCP_RECV;
     grub_memcpy (&ifaces[j].hwaddress, &card->default_address, 
 		 sizeof (ifaces[j].hwaddress));
+    ifaces[j].dhcp_tmo = ifaces[j].dhcp_tmo_left = 1;
     j++;
   }
   ifaces[ncards - 1].next = grub_net_network_level_interfaces;
@@ -599,25 +602,52 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
     grub_net_network_level_interfaces->prev = & ifaces[ncards - 1].next;
   grub_net_network_level_interfaces = &ifaces[0];
   ifaces[0].prev = &grub_net_network_level_interfaces;
-  for (interval = 200; interval < 10000; interval *= 2)
+
+  /*
+   * Running DHCP restransmission timer is kept per interface in dhcp_tmo_left.
+   * When it runs off, dhcp_tmo is increased exponentionally and dhcp_tmo_left
+   * initialized to it. Max value is 32 which gives approximately 12s total per
+   * packet timeout assuming 200ms poll tick. Timeout is reset when DHCP OFFER
+   * is received, so total timeout is 25s in the worst case.
+   *
+   * DHCP NAK also resets timer and transaction starts again.
+   *
+   * Total wait time is limited to ~25s to prevent endless loop in case of
+   * permanent NAK
+   */
+  for (i = 0; i < GRUB_DHCP_MAX_PACKET_TIMEOUT * 4; i++)
     {
       int need_poll = 0;
       for (j = 0; j < ncards; j++)
 	{
-	  if (!ifaces[j].prev)
+	  if (!ifaces[j].prev ||
+             ifaces[j].dhcp_tmo > GRUB_DHCP_MAX_PACKET_TIMEOUT)
+	    continue;
+
+	  if (--ifaces[j].dhcp_tmo_left)
+	    {
+	      need_poll = 1;
+	      continue;
+	    }
+
+	  ifaces[j].dhcp_tmo *= 2;
+	  if (ifaces[j].dhcp_tmo > GRUB_DHCP_MAX_PACKET_TIMEOUT)
 	    continue;
 
 	  err = send_dhcp_packet (&ifaces[j]);
 	  if (err)
 	    {
 	      grub_print_error ();
+	      /* To ignore it during next poll */
+	      ifaces[j].dhcp_tmo = GRUB_DHCP_MAX_PACKET_TIMEOUT + 1;
 	      continue;
 	    }
+	  ifaces[j].dhcp_tmo_left = ifaces[j].dhcp_tmo;
 	  need_poll = 1;
 	}
       if (!need_poll)
 	break;
-      grub_net_poll_cards (interval, 0);
+      grub_net_poll_cards (200, 0);
     }
 
   err = GRUB_ERR_NONE;
diff --git a/include/grub/net.h b/include/grub/net.h
index 3f649d753..a1138f5d4 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -292,6 +292,8 @@ struct grub_net_network_level_interface
   struct grub_net_bootp_packet *dhcp_ack;
   grub_size_t dhcp_acklen;
   grub_uint16_t vlantag;
+  unsigned dhcp_tmo_left; /* DHCPv4 running retransmission timeout */
+  unsigned dhcp_tmo;      /* DHCPv4 current retransmission timeout */
   void *data;
 };
 
-- 
2.17.1

[PATCH v2 6/9] net: dhcp: use DHCP options for name and bootfile

[Andre Przywara]

From: Andrei Borzenkov <arvidjaar@gmail.com>

The BOOTP RFC describes the boot file name and the server name as being
part of the integral BOOTP data structure, with some limits on the size
of them.
DHCP extends this by allowing them to be separate DHCP options, which is
more flexible.

Teach the code dealing with those fields to check for those DHCP options
first and use this information, if provided. We fall back to using the
BOOTP information if those options are not used.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 grub-core/net/bootp.c | 72 +++++++++++++++++++++++++++++++------------
 include/grub/net.h    |  2 ++
 2 files changed, 54 insertions(+), 20 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 42117b72d..0c6756ea0 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -169,7 +169,9 @@ grub_net_configure_by_dhcp_ack (const char *name,
   int mask = -1;
   char server_ip[sizeof ("xxx.xxx.xxx.xxx")];
   const grub_uint8_t *opt;
-  grub_uint8_t opt_len;
+  grub_uint8_t opt_len, overload = 0;
+  const char *boot_file = 0, *server_name = 0;
+  grub_size_t boot_file_len, server_name_len;
 
   addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
   addr.ipv4 = bp->your_ip;
@@ -188,9 +190,36 @@ grub_net_configure_by_dhcp_ack (const char *name,
   if (!inter)
     return 0;
 
-  if (size > OFFSET_OF (boot_file, bp))
-    grub_env_set_net_property (name, "boot_file", bp->boot_file,
-                               sizeof (bp->boot_file));
+  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_OVERLOAD, &opt_len);
+  if (opt && opt_len == 1)
+    overload = *opt;
+
+  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_TFTP_SERVER_NAME, &opt_len);
+  if (opt && opt_len)
+    {
+      server_name = (const char *) opt;
+      server_name_len = opt_len;
+    }
+  else if (size > OFFSET_OF (server_name, bp) && !(overload & GRUB_DHCP_OPT_OVERLOAD_SNAME) &&
+          bp->server_name[0])
+    {
+      server_name = bp->server_name;
+      server_name_len = sizeof (bp->server_name);
+    }
+
+  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_BOOTFILE_NAME, &opt_len);
+  if (opt && opt_len)
+    {
+      boot_file = (const char *) opt;
+      boot_file_len = opt_len;
+    }
+  else if (size > OFFSET_OF (boot_file, bp) && !(overload && GRUB_DHCP_OPT_OVERLOAD_FILE) &&
+          bp->boot_file[0])
+    {
+      boot_file = bp->boot_file;
+      boot_file_len = sizeof (bp->boot_file);
+    }
+
   if (bp->server_ip)
     {
       grub_snprintf (server_ip, sizeof (server_ip), "%d.%d.%d.%d",
@@ -221,35 +250,38 @@ grub_net_configure_by_dhcp_ack (const char *name,
       *device = grub_xasprintf ("tftp,%s", server_ip);
       grub_print_error ();
     }
-  if (size > OFFSET_OF (server_name, bp)
-      && bp->server_name[0])
+
+  if (server_name)
     {
-      grub_env_set_net_property (name, "dhcp_server_name", bp->server_name,
-                                 sizeof (bp->server_name));
+      grub_env_set_net_property (name, "dhcp_server_name", server_name, server_name_len);
       if (is_def && !grub_net_default_server)
 	{
-	  grub_net_default_server = grub_strdup (bp->server_name);
+	  grub_net_default_server = grub_strdup (server_name);
 	  grub_print_error ();
 	}
       if (device && !*device)
 	{
-	  *device = grub_xasprintf ("tftp,%s", bp->server_name);
+	  *device = grub_xasprintf ("tftp,%s", server_name);
 	  grub_print_error ();
 	}
     }
 
-  if (size > OFFSET_OF (boot_file, bp) && path)
+  if (boot_file)
     {
-      *path = grub_strndup (bp->boot_file, sizeof (bp->boot_file));
-      grub_print_error ();
-      if (*path)
+      grub_env_set_net_property (name, "boot_file", boot_file, boot_file_len);
+      if (path)
 	{
-	  char *slash;
-	  slash = grub_strrchr (*path, '/');
-	  if (slash)
-	    *slash = 0;
-	  else
-	    **path = 0;
+	  *path = grub_strndup (boot_file, boot_file_len);
+	  grub_print_error ();
+	  if (*path)
+	    {
+	      char *slash;
+	      slash = grub_strrchr (*path, '/');
+	      if (slash)
+		*slash = 0;
+	      else
+		**path = 0;
+	    }
 	}
     }
 
diff --git a/include/grub/net.h b/include/grub/net.h
index a1138f5d4..f7b546446 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -460,6 +460,8 @@ enum
     GRUB_NET_BOOTP_ROOT_PATH = 0x11,
     GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
     GRUB_NET_DHCP_OVERLOAD = 52,
+    GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
+    GRUB_NET_DHCP_BOOTFILE_NAME = 67,
     GRUB_NET_BOOTP_END = 0xff
   };
 
-- 
2.17.1

[PATCH v2 7/9] net: dhcp: allow receiving DHCP OFFER and ACK packets

[Andre Przywara]

From: Andrei Borzenkov <arvidjaar@gmail.com>

In respone to a BOOTREQUEST packet a BOOTP server would answer with a
BOOTREPLY packet, which ends the conversation for good.
DHCP uses a 4-way handshake, where the initial server respone is an OFFER,
which has to be answered with REQUEST by the client again, only to be
completed by an ACKNOWLEDGE packet from the server.

Teach the grub_net_process_dhcp() function to deal with OFFER packets,
and treat ACK packets the same es BOOTREPLY packets.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 grub-core/net/bootp.c | 76 +++++++++++++++++++++++++++++++++++--------
 include/grub/net.h    |  5 +++
 2 files changed, 67 insertions(+), 14 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 0c6756ea0..f02e73003 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -30,6 +30,21 @@ enum
   GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
   GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
 };
+enum
+{
+  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,
+};
+
+/* Max timeout when waiting for BOOTP/DHCP reply */
+#define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
 
 #define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
 
@@ -444,25 +459,58 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
   struct grub_net_card *card = iface->card;
   const struct grub_net_bootp_packet *bp = (const struct grub_net_bootp_packet *) nb->data;
   grub_size_t size = nb->tail - nb->data;
+  const grub_uint8_t *opt;
+  grub_uint8_t opt_len, type;
+  grub_uint32_t srv_id = 0;
 
-  name = grub_xasprintf ("%s:dhcp", card->name);
-  if (!name)
-    {
-      grub_print_error ();
-      return;
-    }
-  grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
-  grub_free (name);
-  if (grub_errno)
-    grub_print_error ();
+  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_MESSAGE_TYPE, &opt_len);
+  if (opt && opt_len == 1)
+    type = *opt;
   else
+    type = GRUB_DHCP_MESSAGE_UNKNOWN;
+
+  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_SERVER_IDENTIFIER, &opt_len);
+  if (opt && opt_len == sizeof (srv_id))
+    srv_id = grub_get_unaligned32 (opt);
+
+  /*
+   * If we received BOOTP reply or DHCPACK, proceed with configuration.
+   * Otherwise store offered address and server id for later processing
+   * of DHCPACK.
+   * xid and srv_id are stored in network order so do not need conversion.
+   */
+  if ((!iface->srv_id && type == GRUB_DHCP_MESSAGE_UNKNOWN)
+      || (iface->srv_id && type == GRUB_DHCP_MESSAGE_ACK
+	  && bp->ident == iface->xid
+	  && srv_id == iface->srv_id))
     {
-      if (grub_memcmp (iface->name, card->name, grub_strlen (card->name)) == 0
-	  && grub_memcmp (iface->name + grub_strlen (card->name),
-			  ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
+      name = grub_xasprintf ("%s:dhcp", card->name);
+      if (!name)
 	{
-	  grub_net_network_level_interface_unregister (iface);
+	  grub_print_error ();
+	  return;
 	}
+      grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
+      grub_free (name);
+      if (grub_errno)
+	grub_print_error ();
+      else
+	grub_net_network_level_interface_unregister (iface);
+    }
+  else if (!iface->srv_id && type == GRUB_DHCP_MESSAGE_OFFER && srv_id)
+    {
+      iface->srv_id = srv_id;
+      iface->my_ip = bp->your_ip;
+      /* Reset retransmission timer */
+      iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
+    }
+  else if (iface->srv_id && type == GRUB_DHCP_MESSAGE_NAK
+	   && bp->ident == iface->xid
+	   && srv_id == iface->srv_id)
+    {
+      iface->xid = iface->srv_id = iface->my_ip = 0;
+      /* Reset retransmission timer */
+      iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
     }
 }
 
diff --git a/include/grub/net.h b/include/grub/net.h
index f7b546446..68a9f1311 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -292,6 +292,9 @@ struct grub_net_network_level_interface
   struct grub_net_bootp_packet *dhcp_ack;
   grub_size_t dhcp_acklen;
   grub_uint16_t vlantag;
+  grub_uint32_t xid;      /* DHCPv4 transaction id */
+  grub_uint32_t srv_id;   /* DHCPv4 server_identifier */
+  grub_uint32_t my_ip;    /* DHCPv4 offered IP address */
   unsigned dhcp_tmo_left; /* DHCPv4 running retransmission timeout */
   unsigned dhcp_tmo;      /* DHCPv4 current retransmission timeout */
   void *data;
@@ -460,6 +463,8 @@ enum
     GRUB_NET_BOOTP_ROOT_PATH = 0x11,
     GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
     GRUB_NET_DHCP_OVERLOAD = 52,
+    GRUB_NET_DHCP_MESSAGE_TYPE = 53,
+    GRUB_NET_DHCP_SERVER_IDENTIFIER = 54,
     GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
     GRUB_NET_DHCP_BOOTFILE_NAME = 67,
     GRUB_NET_BOOTP_END = 0xff
-- 
2.17.1

[PATCH v2 8/9] net: dhcp: actually send out DHCPv4 DISCOVER and REQUEST messages

[Andre Przywara]

From: Andrei Borzenkov <arvidjaar@gmail.com>

Even though we were parsing some DHCP options sent by the server, so far
we are only using the BOOTP 2-way handshake, even when talking to a DHCP
server.

Change this by actually sending out DHCP DISCOVER packets instead of the
generic (mostly empty) BOOTP BOOTREQUEST packets.

A pure BOOTP server would ignore the extra DHCP options in the DISCOVER
packet and would just reply with a BOOTREPLY packet, which we also
handle in the code.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 grub-core/net/bootp.c | 120 +++++++++++++++++++++++++++++++++++++++++-
 include/grub/net.h    |   2 +
 2 files changed, 121 insertions(+), 1 deletion(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index f02e73003..4b20b1799 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -25,6 +25,48 @@
 #include <grub/net/udp.h>
 #include <grub/datetime.h>
 
+struct grub_dhcp_discover_options
+{
+  grub_uint8_t magic[4];
+  struct
+  {
+    grub_uint8_t code;
+    grub_uint8_t len;
+    grub_uint8_t data;
+  } GRUB_PACKED  message_type;
+  grub_uint8_t end;
+} GRUB_PACKED;
+
+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
 {
   GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
@@ -43,6 +85,8 @@ enum
   GRUB_DHCP_MESSAGE_INFORM,
 };
 
+#define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
+
 /* Max timeout when waiting for BOOTP/DHCP reply */
 #define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
 
@@ -387,6 +431,64 @@ send_dhcp_packet (struct grub_net_network_level_interface *iface)
   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);
   if (!nb)
     return grub_errno;
@@ -400,6 +502,19 @@ send_dhcp_packet (struct grub_net_network_level_interface *iface)
     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)
@@ -417,7 +532,10 @@ send_dhcp_packet (struct grub_net_network_level_interface *iface)
       t = 0;
     }
   pack->seconds = grub_cpu_to_be16 (t);
-  pack->ident = grub_cpu_to_be32 (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);
 
diff --git a/include/grub/net.h b/include/grub/net.h
index 68a9f1311..4a9069a14 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -462,9 +462,11 @@ enum
     GRUB_NET_BOOTP_DOMAIN = 0x0f,
     GRUB_NET_BOOTP_ROOT_PATH = 0x11,
     GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
+    GRUB_NET_DHCP_REQUESTED_IP_ADDRESS = 50,
     GRUB_NET_DHCP_OVERLOAD = 52,
     GRUB_NET_DHCP_MESSAGE_TYPE = 53,
     GRUB_NET_DHCP_SERVER_IDENTIFIER = 54,
+    GRUB_NET_DHCP_PARAMETER_REQUEST_LIST = 55,
     GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
     GRUB_NET_DHCP_BOOTFILE_NAME = 67,
     GRUB_NET_BOOTP_END = 0xff
-- 
2.17.1

[PATCH v2 9/9] net: dhcp: add explicit net_dhcp command

[Andre Przywara]

From: Andrei Borzenkov <arvidjaar@gmail.com>

Mostly for cosmetic reasons, we add a "net_dhcp" command, which is
(at the moment) identical to the existing "net_bootp" command. Both
actually trigger a DHCP handshake now, and both should be able to deal
with pure BOOTP servers.
We could think about dropping the DHCP options from the initial DISCOVER
packet when the user issues the net_bootp command, but it's unclear
whether this is really useful, as both protocols should be able to
coexist.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 grub-core/net/bootp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 4b20b1799..5584d0da3 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -866,7 +866,7 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
   return err;
 }
 
-static grub_command_t cmd_getdhcp, cmd_bootp;
+static grub_command_t cmd_getdhcp, cmd_bootp, cmd_dhcp;
 
 void
 grub_bootp_init (void)
@@ -874,6 +874,9 @@ grub_bootp_init (void)
   cmd_bootp = grub_register_command ("net_bootp", grub_cmd_bootp,
 				     N_("[CARD]"),
 				     N_("perform a bootp autoconfiguration"));
+  cmd_dhcp = grub_register_command ("net_dhcp", grub_cmd_bootp,
+				     N_("[CARD]"),
+				     N_("perform a DHCP autoconfiguration"));
   cmd_getdhcp = grub_register_command ("net_get_dhcp_option", grub_cmd_dhcpopt,
 				       N_("VAR INTERFACE NUMBER DESCRIPTION"),
 				       N_("retrieve DHCP option and save it into VAR. If VAR is - then print the value."));
@@ -884,4 +887,5 @@ grub_bootp_fini (void)
 {
   grub_unregister_command (cmd_getdhcp);
   grub_unregister_command (cmd_bootp);
+  grub_unregister_command (cmd_dhcp);
 }
-- 
2.17.1

Re: [PATCH v2 1/9] net: dhcp: remove dead code

[Daniel Kiper]

Hi Andre,

First of all, sorry for late reply. I am clearing my backlog after the
travel. I hope that I will be replying more often starting from now.

On Tue, Feb 12, 2019 at 05:46:52PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> The comment is right, the "giaddr" fields holds the IP address of the
> BOOTP relay, not a general purpose router address.
> Just remove the commented code, archeologists can find it in the git
> history.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 2/9] net: dhcp: replace parse_dhcp_vendor() with find_dhcp_option()

[Daniel Kiper]

On Tue, Feb 12, 2019 at 05:46:53PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> For proper DHCP support we will need to parse DHCP options from a packet
> more often and at various places.
>
> Refactor the option parsing into a new function, which will scan a
> packet to find *a particular* option field.
> Use that new function in places where we were dealing with DHCP options
> before.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

This patch also adds support for overload. Could you split it into two?

Daniel

Re: [PATCH v2 3/9] net: dhcp: refactor DHCP packet transmission into separate function

[Daniel Kiper]

On Tue, Feb 12, 2019 at 05:46:54PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> In contrast to BOOTP, DHCP uses a 4-way handshake, so requires to send
> packets more often.
>
> Refactor the generation and sending of the BOOTREQUEST packet into a
> separate function, so that future code can more easily reuse this.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

But one nitpick below...

> -	  err = grub_net_send_ip_packet (&ifaces[j], &target, &ll_target, nb,
> -					 GRUB_NET_IP_UDP);
> -	  grub_netbuff_free (nb);
> -	  if (err)
> -	    return err;
> +	  need_poll = 1;
>  	}
> -      if (!done)
> +      if (!need_poll)
>  	break;
>        grub_net_poll_cards (interval, 0);
>      }
>
>    err = GRUB_ERR_NONE;
> +

This is not needed.

Daniel

Re: [PATCH v2 4/9] net: dhcp: make grub_net_process_dhcp take an interface

[Daniel Kiper]

On Tue, Feb 12, 2019 at 05:46:55PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> Change the interface of the function dealing with incoming BOOTP packets
> to take an interface instead of a card, to allow more fine per-interface
> state (timeout, handshake state) later on.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

In general Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

However, please add a blurb to the commit message that you are also
cleaning up the code a bit...

> ---
>  grub-core/net/bootp.c | 29 ++++++++++++++++-------------
>  grub-core/net/ip.c    |  2 +-
>  include/grub/net.h    |  2 +-
>  3 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index fd56de907..573398aa4 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -396,12 +396,19 @@ out:
>    return err;
>  }
>
> +/*
> + * This is called directly from net/ip.c:handle_dgram(), because those
> + * BOOTP/DHCP packets are a bit special due to their improper
> + * sender/receiver IP fields.
> + */
>  void
>  grub_net_process_dhcp (struct grub_net_buff *nb,
> -		       struct grub_net_card *card)
> +		       struct grub_net_network_level_interface *iface)
>  {
>    char *name;
> -  struct grub_net_network_level_interface *inf;
> +  struct grub_net_card *card = iface->card;
> +  const struct grub_net_bootp_packet *bp = (const struct grub_net_bootp_packet *) nb->data;
> +  grub_size_t size = nb->tail - nb->data;
>
>    name = grub_xasprintf ("%s:dhcp", card->name);
>    if (!name)
> @@ -409,22 +416,18 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
>        grub_print_error ();
>        return;
>      }
> -  grub_net_configure_by_dhcp_ack (name, card,
> -				  0, (const struct grub_net_bootp_packet *) nb->data,
> -				  (nb->tail - nb->data), 0, 0, 0);
> +  grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
>    grub_free (name);
>    if (grub_errno)
>      grub_print_error ();
>    else
>      {
> -      FOR_NET_NETWORK_LEVEL_INTERFACES(inf)
> -	if (grub_memcmp (inf->name, card->name, grub_strlen (card->name)) == 0
> -	    && grub_memcmp (inf->name + grub_strlen (card->name),
> -			    ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
> -	  {
> -	    grub_net_network_level_interface_unregister (inf);
> -	    break;
> -	  }
> +      if (grub_memcmp (iface->name, card->name, grub_strlen (card->name)) == 0
> +	  && grub_memcmp (iface->name + grub_strlen (card->name),
> +			  ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
> +	{
> +	  grub_net_network_level_interface_unregister (iface);
> +	}
>      }

And I think that you can drop four curly braces here.

Daniel

Re: [PATCH v2 5/9] net: dhcp: introduce per-interface timeout

[Daniel Kiper]

On Tue, Feb 12, 2019 at 05:46:56PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> Currently we have a global timeout for all network cards in the
> BOOTP/DHCP discovery process.
>
> Make this timeout a per-interface one, so better accommodate the
> upcoming 4-way DHCP handshake and to also cover the lease time limit a
> DHCP offer will come with.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 6/9] net: dhcp: use DHCP options for name and bootfile

[Daniel Kiper]

On Tue, Feb 12, 2019 at 05:46:57PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> The BOOTP RFC describes the boot file name and the server name as being
> part of the integral BOOTP data structure, with some limits on the size
> of them.
> DHCP extends this by allowing them to be separate DHCP options, which is
> more flexible.
>
> Teach the code dealing with those fields to check for those DHCP options
> first and use this information, if provided. We fall back to using the
> BOOTP information if those options are not used.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  grub-core/net/bootp.c | 72 +++++++++++++++++++++++++++++++------------
>  include/grub/net.h    |  2 ++
>  2 files changed, 54 insertions(+), 20 deletions(-)
>
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index 42117b72d..0c6756ea0 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -169,7 +169,9 @@ grub_net_configure_by_dhcp_ack (const char *name,
>    int mask = -1;
>    char server_ip[sizeof ("xxx.xxx.xxx.xxx")];
>    const grub_uint8_t *opt;
> -  grub_uint8_t opt_len;
> +  grub_uint8_t opt_len, overload = 0;
> +  const char *boot_file = 0, *server_name = 0;
> +  grub_size_t boot_file_len, server_name_len;
>
>    addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
>    addr.ipv4 = bp->your_ip;
> @@ -188,9 +190,36 @@ grub_net_configure_by_dhcp_ack (const char *name,
>    if (!inter)
>      return 0;
>
> -  if (size > OFFSET_OF (boot_file, bp))
> -    grub_env_set_net_property (name, "boot_file", bp->boot_file,
> -                               sizeof (bp->boot_file));
> +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_OVERLOAD, &opt_len);
> +  if (opt && opt_len == 1)
> +    overload = *opt;
> +
> +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_TFTP_SERVER_NAME, &opt_len);
> +  if (opt && opt_len)
> +    {
> +      server_name = (const char *) opt;
> +      server_name_len = opt_len;
> +    }
> +  else if (size > OFFSET_OF (server_name, bp) && !(overload & GRUB_DHCP_OPT_OVERLOAD_SNAME) &&
> +          bp->server_name[0])
> +    {
> +      server_name = bp->server_name;
> +      server_name_len = sizeof (bp->server_name);
> +    }
> +
> +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_BOOTFILE_NAME, &opt_len);
> +  if (opt && opt_len)
> +    {
> +      boot_file = (const char *) opt;
> +      boot_file_len = opt_len;
> +    }
> +  else if (size > OFFSET_OF (boot_file, bp) && !(overload && GRUB_DHCP_OPT_OVERLOAD_FILE) &&
> +          bp->boot_file[0])
> +    {
> +      boot_file = bp->boot_file;
> +      boot_file_len = sizeof (bp->boot_file);
> +    }
> +
>    if (bp->server_ip)
>      {
>        grub_snprintf (server_ip, sizeof (server_ip), "%d.%d.%d.%d",
> @@ -221,35 +250,38 @@ grub_net_configure_by_dhcp_ack (const char *name,
>        *device = grub_xasprintf ("tftp,%s", server_ip);
>        grub_print_error ();
>      }
> -  if (size > OFFSET_OF (server_name, bp)
> -      && bp->server_name[0])
> +
> +  if (server_name)
>      {
> -      grub_env_set_net_property (name, "dhcp_server_name", bp->server_name,
> -                                 sizeof (bp->server_name));
> +      grub_env_set_net_property (name, "dhcp_server_name", server_name, server_name_len);
>        if (is_def && !grub_net_default_server)
>  	{
> -	  grub_net_default_server = grub_strdup (bp->server_name);
> +	  grub_net_default_server = grub_strdup (server_name);
>  	  grub_print_error ();
>  	}
>        if (device && !*device)
>  	{
> -	  *device = grub_xasprintf ("tftp,%s", bp->server_name);
> +	  *device = grub_xasprintf ("tftp,%s", server_name);
>  	  grub_print_error ();
>  	}
>      }
>
> -  if (size > OFFSET_OF (boot_file, bp) && path)
> +  if (boot_file)
>      {
> -      *path = grub_strndup (bp->boot_file, sizeof (bp->boot_file));
> -      grub_print_error ();
> -      if (*path)
> +      grub_env_set_net_property (name, "boot_file", boot_file, boot_file_len);
> +      if (path)
>  	{
> -	  char *slash;
> -	  slash = grub_strrchr (*path, '/');
> -	  if (slash)
> -	    *slash = 0;
> -	  else
> -	    **path = 0;
> +	  *path = grub_strndup (boot_file, boot_file_len);
> +	  grub_print_error ();
> +	  if (*path)
> +	    {
> +	      char *slash;
> +	      slash = grub_strrchr (*path, '/');
> +	      if (slash)
> +		*slash = 0;
> +	      else
> +		**path = 0;
> +	    }

I am not sure why you are changing the code starting from
"if (size > OFFSET_OF (boot_file, bp) && path)". Could you
enlighten me?

Daniel

Re: [PATCH v2 7/9] net: dhcp: allow receiving DHCP OFFER and ACK packets

[Daniel Kiper]

On Tue, Feb 12, 2019 at 05:46:58PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> In respone to a BOOTREQUEST packet a BOOTP server would answer with a
> BOOTREPLY packet, which ends the conversation for good.
> DHCP uses a 4-way handshake, where the initial server respone is an OFFER,
> which has to be answered with REQUEST by the client again, only to be
> completed by an ACKNOWLEDGE packet from the server.
>
> Teach the grub_net_process_dhcp() function to deal with OFFER packets,
> and treat ACK packets the same es BOOTREPLY packets.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 8/9] net: dhcp: actually send out DHCPv4 DISCOVER and REQUEST messages

[Daniel Kiper]

On Tue, Feb 12, 2019 at 05:46:59PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> Even though we were parsing some DHCP options sent by the server, so far
> we are only using the BOOTP 2-way handshake, even when talking to a DHCP
> server.
>
> Change this by actually sending out DHCP DISCOVER packets instead of the
> generic (mostly empty) BOOTP BOOTREQUEST packets.
>
> A pure BOOTP server would ignore the extra DHCP options in the DISCOVER
> packet and would just reply with a BOOTREPLY packet, which we also
> handle in the code.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 9/9] net: dhcp: add explicit net_dhcp command

[Daniel Kiper]

On Tue, Feb 12, 2019 at 05:47:00PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> Mostly for cosmetic reasons, we add a "net_dhcp" command, which is
> (at the moment) identical to the existing "net_bootp" command. Both
> actually trigger a DHCP handshake now, and both should be able to deal
> with pure BOOTP servers.
> We could think about dropping the DHCP options from the initial DISCOVER
> packet when the user issues the net_bootp command, but it's unclear
> whether this is really useful, as both protocols should be able to
> coexist.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 6/9] net: dhcp: use DHCP options for name and bootfile

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 5548 bytes --]

On Thu, 21 Feb 2019 19:49:08 +0100
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Tue, Feb 12, 2019 at 05:46:57PM +0000, Andre Przywara wrote:
> > From: Andrei Borzenkov <arvidjaar@gmail.com>
> >
> > The BOOTP RFC describes the boot file name and the server name as being
> > part of the integral BOOTP data structure, with some limits on the size
> > of them.
> > DHCP extends this by allowing them to be separate DHCP options, which is
> > more flexible.
> >
> > Teach the code dealing with those fields to check for those DHCP options
> > first and use this information, if provided. We fall back to using the
> > BOOTP information if those options are not used.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  grub-core/net/bootp.c | 72 +++++++++++++++++++++++++++++++------------
> >  include/grub/net.h    |  2 ++
> >  2 files changed, 54 insertions(+), 20 deletions(-)
> >
> > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> > index 42117b72d..0c6756ea0 100644
> > --- a/grub-core/net/bootp.c
> > +++ b/grub-core/net/bootp.c
> > @@ -169,7 +169,9 @@ grub_net_configure_by_dhcp_ack (const char *name,
> >    int mask = -1;
> >    char server_ip[sizeof ("xxx.xxx.xxx.xxx")];
> >    const grub_uint8_t *opt;
> > -  grub_uint8_t opt_len;
> > +  grub_uint8_t opt_len, overload = 0;
> > +  const char *boot_file = 0, *server_name = 0;
> > +  grub_size_t boot_file_len, server_name_len;
> >
> >    addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
> >    addr.ipv4 = bp->your_ip;
> > @@ -188,9 +190,36 @@ grub_net_configure_by_dhcp_ack (const char *name,
> >    if (!inter)
> >      return 0;
> >
> > -  if (size > OFFSET_OF (boot_file, bp))
> > -    grub_env_set_net_property (name, "boot_file", bp->boot_file,
> > -                               sizeof (bp->boot_file));
> > +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_OVERLOAD, &opt_len);
> > +  if (opt && opt_len == 1)
> > +    overload = *opt;
> > +
> > +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_TFTP_SERVER_NAME, &opt_len);
> > +  if (opt && opt_len)
> > +    {
> > +      server_name = (const char *) opt;
> > +      server_name_len = opt_len;
> > +    }
> > +  else if (size > OFFSET_OF (server_name, bp) && !(overload & GRUB_DHCP_OPT_OVERLOAD_SNAME) &&
> > +          bp->server_name[0])
> > +    {
> > +      server_name = bp->server_name;
> > +      server_name_len = sizeof (bp->server_name);
> > +    }
> > +
> > +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_BOOTFILE_NAME, &opt_len);
> > +  if (opt && opt_len)
> > +    {
> > +      boot_file = (const char *) opt;
> > +      boot_file_len = opt_len;
> > +    }
> > +  else if (size > OFFSET_OF (boot_file, bp) && !(overload && GRUB_DHCP_OPT_OVERLOAD_FILE) &&
> > +          bp->boot_file[0])
> > +    {
> > +      boot_file = bp->boot_file;
> > +      boot_file_len = sizeof (bp->boot_file);
> > +    }
> > +
> >    if (bp->server_ip)
> >      {
> >        grub_snprintf (server_ip, sizeof (server_ip), "%d.%d.%d.%d",
> > @@ -221,35 +250,38 @@ grub_net_configure_by_dhcp_ack (const char *name,
> >        *device = grub_xasprintf ("tftp,%s", server_ip);
> >        grub_print_error ();
> >      }
> > -  if (size > OFFSET_OF (server_name, bp)
> > -      && bp->server_name[0])
> > +
> > +  if (server_name)
> >      {
> > -      grub_env_set_net_property (name, "dhcp_server_name", bp->server_name,
> > -                                 sizeof (bp->server_name));
> > +      grub_env_set_net_property (name, "dhcp_server_name", server_name, server_name_len);
> >        if (is_def && !grub_net_default_server)
> >  	{
> > -	  grub_net_default_server = grub_strdup (bp->server_name);
> > +	  grub_net_default_server = grub_strdup (server_name);
> >  	  grub_print_error ();
> >  	}
> >        if (device && !*device)
> >  	{
> > -	  *device = grub_xasprintf ("tftp,%s", bp->server_name);
> > +	  *device = grub_xasprintf ("tftp,%s", server_name);
> >  	  grub_print_error ();
> >  	}
> >      }
> >
> > -  if (size > OFFSET_OF (boot_file, bp) && path)
> > +  if (boot_file)
> >      {
> > -      *path = grub_strndup (bp->boot_file, sizeof (bp->boot_file));
> > -      grub_print_error ();
> > -      if (*path)
> > +      grub_env_set_net_property (name, "boot_file", boot_file, boot_file_len);
> > +      if (path)
> >  	{
> > -	  char *slash;
> > -	  slash = grub_strrchr (*path, '/');
> > -	  if (slash)
> > -	    *slash = 0;
> > -	  else
> > -	    **path = 0;
> > +	  *path = grub_strndup (boot_file, boot_file_len);
> > +	  grub_print_error ();
> > +	  if (*path)
> > +	    {
> > +	      char *slash;
> > +	      slash = grub_strrchr (*path, '/');
> > +	      if (slash)
> > +		*slash = 0;
> > +	      else
> > +		**path = 0;
> > +	    }  
> 
> I am not sure why you are changing the code starting from
> "if (size > OFFSET_OF (boot_file, bp) && path)". Could you
> enlighten me?

The diff is indeed somewhat confusing. So several things are changed here:
- The "boot file" could be specified via *two* ways now, as a DHCP option or using the legacy BOOTP structure field. This is detected at separate places, that's why we change the first line to check for a valid boot_file pointer, instead of hardcoding the BOOTP structure field way here.
- grub_env_set_net_property() was called before when detecting the BOOTP structure field (see above), we do it now here, to cover both ways.
- There is an additional molly guard to check that "path" is not a NULL pointer.

I am attaching a slightly modified wide diff (diff -yb) to make this clearer.

Cheers,
Andre.

[-- Attachment #2: boot_file.diff --]
[-- Type: text/x-patch, Size: 757 bytes --]

  if (size > OFFSET_OF (boot_file, bp) && path)		      |	  if (boot_file)
    {								    {
							      >	      if (path)
							      >		{
							      >           grub_env_set_net_property (name, "boot_file", boot_file
      *path = grub_strndup (bp->boot_file, sizeof (bp->boot_f ~		  *path = grub_strndup (boot_file, boot_file_len);
      grub_print_error ();						  grub_print_error ();
      if (*path)							  if (*path)
	{								    {
	  char *slash;							      char *slash;
	  slash = grub_strrchr (*path, '/');				      slash = grub_strrchr (*path, '/');
	  if (slash)							      if (slash)
	    *slash = 0;								*slash = 0;
	  else								      else
	    **path = 0;								**path = 0;
							      >		    }
	}								}
    }								    }