All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE
@ 2015-05-12  8:49 Michael Chang
  2015-05-12  8:49 ` [PATCH 1/3] Added net_bootp6 command Michael Chang
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Michael Chang @ 2015-05-12  8:49 UTC (permalink / raw)
  To: grub-devel

This patch set tries to make support of configuring IPv6 network interface
through the DHCPv6 protocol. A new command, net_bootp6, is therefore introduced
to serve the purpose. You can think it as DHCPv6/IPv6 version of the existing
net_bootp command.

In addition to that, the UEFI IPv6 PXE support can be easily done by sharing
the same routine with net_bootp6 to parse DHCPv6 Reply packet cached in the
firmware.

changes in v1:
- Added upper boundary check in find_dhcpv6_option
- Fix memory leak and freeing NULL pointer
- Improved error message logging to not get lost
- Use grub_cpu_to_be16_compile_time for endianess conversion when appropriate
- Removed grub_dhcpv6_dns_servers structure and use 16 bytes blocks
- Avoud magic numbers and use more descriptive sizeof when populating netbuff
- Move include/grub/efi/api.h to UEFI IPv6 PXE support patch
- Document the net_bootp6 command



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] Added net_bootp6 command
  2015-05-12  8:49 [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE Michael Chang
@ 2015-05-12  8:49 ` Michael Chang
  2015-05-15  6:26   ` Andrei Borzenkov
  2015-05-12  8:49 ` [PATCH 2/3] UEFI IPv6 PXE support Michael Chang
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Michael Chang @ 2015-05-12  8:49 UTC (permalink / raw)
  To: grub-devel

The net_bootp6 is used to configure the ipv6 network interface through the
DHCPv6 protocol Solict/Advertise/Request/Reply.
---
 grub-core/net/bootp.c |  895 ++++++++++++++++++++++++++++++++++++++++++++++++-
 grub-core/net/ip.c    |   35 ++
 include/grub/net.h    |   19 +
 3 files changed, 948 insertions(+), 1 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 6136755..5c5eb6f 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -24,6 +24,8 @@
 #include <grub/net/netbuff.h>
 #include <grub/net/udp.h>
 #include <grub/datetime.h>
+#include <grub/time.h>
+#include <grub/list.h>
 
 static void
 parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
@@ -256,6 +258,646 @@ grub_net_configure_by_dhcp_ack (const char *name,
   return inter;
 }
 
+struct grub_dhcpv6_option {
+  grub_uint16_t code;
+  grub_uint16_t len;
+  grub_uint8_t data[0];
+} GRUB_PACKED;
+
+
+struct grub_dhcpv6_iana_option {
+  grub_uint32_t iaid;
+  grub_uint32_t t1;
+  grub_uint32_t t2;
+  grub_uint8_t data[0];
+} GRUB_PACKED;
+
+struct grub_dhcpv6_iaaddr_option {
+  grub_uint8_t addr[16];
+  grub_uint32_t preferred_lifetime;
+  grub_uint32_t valid_lifetime;
+  grub_uint8_t data[0];
+} GRUB_PACKED;
+
+struct grub_DUID_LL
+{
+  grub_uint16_t type;
+  grub_uint16_t hw_type;
+  grub_uint8_t hwaddr[6];
+} GRUB_PACKED;
+
+#define GRUB_DHCPv6_ADVERTISE 2
+#define GRUB_DHCPv6_REQUEST 3
+#define GRUB_DHCPv6_REPLY 7
+
+#define GRUB_DHCPv6_OPTION_CLIENTID 1
+#define GRUB_DHCPv6_OPTION_SERVERID 2
+#define GRUB_DHCPv6_OPTION_IA_NA 3
+#define GRUB_DHCPv6_OPTION_IAADDR 5
+#define GRUB_DHCPv6_OPTION_ORO 6
+#define GRUB_DHCPv6_OPTION_ELAPSED_TIME 8
+#define GRUB_DHCPv6_OPTION_DNS_SERVERS 23
+#define GRUB_DHCPv6_OPTION_BOOTFILE_URL 59
+
+/* The default netbuff size for sending DHCPv6 packets which should be
+   large enough to hold the information */
+#define GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE 512
+
+struct grub_dhcpv6_session
+{
+  struct grub_dhcpv6_session *next;
+  struct grub_dhcpv6_session **prev;
+  grub_uint32_t iaid;
+  grub_uint32_t transaction_id:24;
+  grub_uint64_t start_time;
+  struct grub_net_network_level_interface *iface;
+};
+
+static struct grub_dhcpv6_session *grub_dhcpv6_sessions = NULL;
+#define FOR_DHCPV6_SESSIONS(var) FOR_LIST_ELEMENTS (var, grub_dhcpv6_sessions)
+
+static void
+grub_dhcpv6_session_add (struct grub_dhcpv6_session *session)
+{
+  struct grub_datetime date;
+  grub_err_t err;
+  grub_int32_t t = 0;
+
+  err = grub_get_datetime (&date);
+  if (err || !grub_datetime2unixtime (&date, &t))
+    {
+      grub_errno = GRUB_ERR_NONE;
+      t = 0;
+    }
+
+  session->transaction_id = t;
+  session->start_time = grub_get_time_ms ();
+  grub_list_push (GRUB_AS_LIST_P (&grub_dhcpv6_sessions), GRUB_AS_LIST (session));
+
+  return;
+}
+
+static void
+grub_dhcpv6_sessions_free (void)
+{
+  struct grub_dhcpv6_session *session;
+
+  FOR_DHCPV6_SESSIONS (session)
+    {
+      grub_list_remove (GRUB_AS_LIST (session));
+      grub_free (session);
+      session = grub_dhcpv6_sessions;
+    }
+
+  return;
+}
+
+static const char *
+get_dhcpv6_option_name (grub_uint16_t option)
+{
+  switch (option)
+    {
+    case GRUB_DHCPv6_OPTION_BOOTFILE_URL:
+      return "BOOTFILE URL";
+    case GRUB_DHCPv6_OPTION_DNS_SERVERS:
+      return "DNS SERVERS";
+    case GRUB_DHCPv6_OPTION_IA_NA:
+      return "IA NA";
+    case GRUB_DHCPv6_OPTION_IAADDR:
+      return "IAADDR";
+    case GRUB_DHCPv6_OPTION_CLIENTID:
+      return "CLIENTID";
+    case GRUB_DHCPv6_OPTION_SERVERID:
+      return "SERVERID";
+    case GRUB_DHCPv6_OPTION_ORO:
+      return "ORO";
+    case GRUB_DHCPv6_OPTION_ELAPSED_TIME:
+      return "ELAPSED TIME";
+    default:
+      return "UNKNOWN";
+    }
+}
+
+static const struct grub_dhcpv6_option*
+find_dhcpv6_option (const struct grub_dhcpv6_option *popt, grub_size_t size, grub_uint16_t option)
+{
+  while (size > 0)
+    {
+      grub_uint16_t code, len;
+
+      code = grub_be_to_cpu16 (popt->code);
+      len = grub_be_to_cpu16 (popt->len) + sizeof (*popt);
+
+      if (size < len)
+	{
+	  grub_error (GRUB_ERR_OUT_OF_RANGE, N_("DHCPv6 option size overflow detected"));
+	  return NULL;
+	}
+
+      if (option == code)
+	return popt;
+
+      if (code == 0)
+	break;
+      else
+	{
+	  size -= len;
+	  popt = (const struct grub_dhcpv6_option *)((grub_uint8_t *)popt + len);
+	}
+    }
+
+  grub_error (GRUB_ERR_IO, N_("DHCPv6 Option (%u):%s not found"), option, get_dhcpv6_option_name(option));
+  return NULL;
+}
+
+static const struct grub_dhcpv6_option*
+find_dhcpv6_option_in_packet (const struct grub_net_dhcpv6_packet *packet,
+    grub_size_t size, grub_uint16_t option)
+{
+  if (!size || !packet)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("null or zero sized DHCPv6 packet buffer"));
+      return NULL;
+    }
+
+  if (size <= sizeof (*packet))
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("DHCPv6 packet size too small"));
+      return NULL;
+    }
+
+  return find_dhcpv6_option ((const struct grub_dhcpv6_option *)packet->dhcp_options,
+	  size - sizeof (*packet), option);
+}
+
+static const grub_uint8_t*
+find_dhcpv6_address (const struct grub_net_dhcpv6_packet *packet, grub_size_t size)
+{
+  const struct grub_dhcpv6_option* popt;
+  const struct grub_dhcpv6_iana_option *ia_na;
+  const struct grub_dhcpv6_iaaddr_option *iaaddr;
+  grub_uint16_t ia_na_len;
+
+  popt = find_dhcpv6_option_in_packet (packet, size, GRUB_DHCPv6_OPTION_IA_NA);
+
+  if (!popt)
+    return NULL;
+
+  ia_na = (const struct grub_dhcpv6_iana_option *) popt->data;
+  ia_na_len = grub_be_to_cpu16 (popt->len);
+
+  popt = find_dhcpv6_option ((const struct grub_dhcpv6_option *) ia_na->data,
+      ia_na_len - sizeof (*ia_na), GRUB_DHCPv6_OPTION_IAADDR);
+
+  if (!popt)
+    return NULL;
+
+  iaaddr = (const struct grub_dhcpv6_iaaddr_option *)popt->data;
+  return iaaddr->addr;
+}
+
+static void
+get_dhcpv6_dns_address (const struct grub_net_dhcpv6_packet *packet, grub_size_t size,
+	grub_net_network_level_address_t **addr, grub_uint16_t *naddr)
+{
+  const struct grub_dhcpv6_option* popt;
+  grub_uint16_t len, ln;
+  const grub_uint8_t *pa;
+  grub_net_network_level_address_t *la;
+
+  if (!addr || !naddr)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("bad argument for get_dhcpv6_dns_address"));
+      return;
+    }
+
+  *addr = NULL;
+  *naddr = 0;
+
+  popt = find_dhcpv6_option_in_packet (packet, size, GRUB_DHCPv6_OPTION_DNS_SERVERS);
+  if (!popt)
+    return;
+
+  len = grub_be_to_cpu16 (popt->len);
+  if (len == 0 || len & 0xf)
+    {
+      grub_error (GRUB_ERR_IO, N_("invalid dns address length"));
+      return;
+    }
+
+  *naddr = ln = len >> 4;
+  *addr = la =  grub_zalloc (sizeof (grub_net_network_level_address_t) * ln);
+
+  if (!la)
+    {
+      *addr = NULL;
+      *naddr = 0;
+      return;
+    }
+
+  for (pa = popt->data; ln > 0; pa += 0x10, la++, ln--)
+    {
+      la->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
+      la->ipv6[0] = grub_get_unaligned64 (pa);
+      la->ipv6[1] = grub_get_unaligned64 (pa + 8);
+      la->option = DNS_OPTION_PREFER_IPV6;
+    }
+
+  return;
+}
+
+static void
+find_dhcpv6_bootfile_url (const struct grub_net_dhcpv6_packet *packet, grub_size_t size,
+	char **proto, char **server_ip, char **boot_file)
+{
+  char *bootfile_url;
+  const struct grub_dhcpv6_option* opt_url;
+  char *ip_start, *ip_end;
+  char *path;
+  grub_size_t ip_len;
+  grub_uint16_t len;
+  const char *protos[] = {"tftp://", "http://", NULL};
+  const char *pr;
+  int i;
+
+  if (proto)
+    *proto = NULL;
+
+  if (server_ip)
+    *server_ip = NULL;
+
+  if (boot_file)
+    *boot_file = NULL;
+
+  opt_url = find_dhcpv6_option_in_packet (packet, size, GRUB_DHCPv6_OPTION_BOOTFILE_URL);
+
+  if (!opt_url)
+    return;
+
+  len = grub_be_to_cpu16 (opt_url->len);
+
+  bootfile_url = grub_malloc (len + 1);
+
+  if (!bootfile_url)
+    return;
+
+  grub_memcpy (bootfile_url, opt_url->data, len);
+  bootfile_url[len]   = '\0';
+
+  for (i = 0; (pr = *(protos + i)); ++i)
+      if (grub_strncmp (bootfile_url, pr, grub_strlen(pr)) == 0)
+	break;
+
+  if (!pr)
+    {
+      grub_error (GRUB_ERR_IO,
+	N_("unsupported protocol, only tftp and http are supported"));
+      goto cleanup;
+    }
+
+  ip_start = ip_end = NULL;
+  ip_start = bootfile_url + grub_strlen(pr);
+
+  /* Follow elilo and edk2 that check for starting and ending delimiter '[..]'
+     in which IPv6 server address is enclosed. */
+  if (*ip_start != '[')
+    ip_start = NULL;
+  else
+    ip_end = grub_strchr (++ip_start, ']');
+
+  if (!ip_start || !ip_end)
+    {
+      grub_error (GRUB_ERR_IO, N_("IPv6-address not in square brackets"));
+      goto cleanup;
+    }
+
+  ip_len = ip_end - ip_start;
+
+  if (proto)
+    {
+      grub_size_t proto_len  = grub_strlen (pr) - 3;
+
+      *proto = grub_malloc (proto_len + 1);
+      if (!*proto)
+	goto cleanup;
+
+      grub_memcpy (*proto, pr, proto_len);
+      *(*proto + proto_len)  = '\0';
+    }
+
+  if (server_ip)
+    {
+      *server_ip = grub_malloc (ip_len + 1);
+
+      if (!*server_ip)
+	goto cleanup;
+
+      grub_memcpy (*server_ip, ip_start, ip_len);
+      *(*server_ip + ip_len) = '\0';
+    }
+
+  path = ip_end + 1;
+
+  if (boot_file)
+    {
+      *boot_file = grub_strdup (path);
+
+      if (!*boot_file)
+	goto cleanup;
+    }
+
+cleanup:
+
+  if (bootfile_url)
+    grub_free (bootfile_url);
+
+  if (grub_errno)
+    {
+      if (proto && *proto)
+	{
+	  grub_free (*proto);
+	  *proto = NULL;
+	}
+
+      if (server_ip && *server_ip)
+	{
+	  grub_free (*server_ip);
+	  *server_ip = NULL;
+	}
+
+      if (boot_file && *boot_file)
+	{
+	  grub_free (*boot_file);
+	  *boot_file = NULL;
+	}
+    }
+
+  return;
+}
+
+
+static grub_err_t
+grub_net_configure_by_dhcpv6_adv (const struct grub_net_dhcpv6_packet *v6_adv,
+	grub_size_t size,
+	struct grub_dhcpv6_session *session)
+{
+  struct grub_net_buff *nb;
+  const struct grub_dhcpv6_option *opt_client, *opt_server, *opt_iana;
+  struct grub_dhcpv6_option *popt;
+  struct grub_net_dhcpv6_packet *v6;
+  struct udphdr *udph;
+  grub_net_network_level_address_t multicast;
+  grub_net_link_level_address_t ll_multicast;
+  struct grub_net_network_level_interface *inf;
+  grub_uint16_t len;
+  grub_uint64_t elapsed;
+  grub_err_t err = GRUB_ERR_NONE;
+
+  opt_client = find_dhcpv6_option_in_packet (v6_adv, size, GRUB_DHCPv6_OPTION_CLIENTID);
+  if (grub_errno)
+    grub_error_push ();
+
+  opt_server = find_dhcpv6_option_in_packet (v6_adv, size, GRUB_DHCPv6_OPTION_SERVERID);
+  if (grub_errno)
+    grub_error_push ();
+
+  opt_iana = find_dhcpv6_option_in_packet (v6_adv, size, GRUB_DHCPv6_OPTION_IA_NA);
+  if (grub_errno)
+    grub_error_push ();
+
+  grub_error_pop ();
+  if (grub_errno)
+    return grub_errno;
+
+  inf = session->iface;
+
+  multicast.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
+  multicast.ipv6[0] = grub_cpu_to_be64_compile_time (0xff02ULL << 48);
+  multicast.ipv6[1] = grub_cpu_to_be64_compile_time (0x10002ULL);
+
+  err = grub_net_link_layer_resolve (inf, &multicast, &ll_multicast);
+  if (err)
+    return err;
+
+  nb = grub_netbuff_alloc (GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
+
+  if (!nb)
+    return grub_errno;
+
+  err = grub_netbuff_reserve (nb, GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
+  if (err)
+    {
+      grub_netbuff_free (nb);
+      return err;
+    }
+
+  len = grub_cpu_to_be16(opt_client->len);
+  err = grub_netbuff_push (nb, len + sizeof (*opt_client));
+  if (err)
+    {
+      grub_netbuff_free (nb);
+      return err;
+    }
+  grub_memcpy (nb->data, opt_client, len + sizeof (*opt_client));
+
+  len = grub_cpu_to_be16(opt_server->len);
+  err = grub_netbuff_push (nb, len + sizeof (*opt_server));
+  if (err)
+    {
+      grub_netbuff_free (nb);
+      return err;
+    }
+  grub_memcpy (nb->data, opt_server, len + sizeof (*opt_server));
+
+  len = grub_cpu_to_be16(opt_iana->len);
+  err = grub_netbuff_push (nb, len + sizeof (*opt_iana));
+  if (err)
+    {
+      grub_netbuff_free (nb);
+      return err;
+    }
+  grub_memcpy (nb->data, opt_iana, len + sizeof (*opt_iana));
+
+  err = grub_netbuff_push (nb, sizeof (*popt) + 2 * sizeof (grub_uint16_t));
+  if (err)
+    {
+      grub_netbuff_free (nb);
+      return err;
+    }
+
+  popt = (struct grub_dhcpv6_option*) nb->data;
+  popt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_ORO);
+  popt->len = grub_cpu_to_be16_compile_time (2 * sizeof (grub_uint16_t));
+  grub_set_unaligned16 (popt->data, grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_BOOTFILE_URL));
+  grub_set_unaligned16 (popt->data + 2, grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_DNS_SERVERS));
+
+  err = grub_netbuff_push (nb, sizeof (*popt) + sizeof (grub_uint16_t));
+  if (err)
+    {
+      grub_netbuff_free (nb);
+      return err;
+    }
+  popt = (struct grub_dhcpv6_option*) nb->data;
+  popt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_ELAPSED_TIME);
+  popt->len = grub_cpu_to_be16_compile_time (sizeof (grub_uint16_t));
+
+  /* the time is expressed in hundredths of a second */
+  elapsed = grub_divmod64 (grub_get_time_ms () - session->start_time, 10, 0);
+
+  if (elapsed > 0xffff)
+    elapsed = 0xffff;
+
+  grub_set_unaligned16 (popt->data,  grub_cpu_to_be16 ((grub_uint16_t)elapsed));
+
+  err = grub_netbuff_push (nb, sizeof (*v6));
+  if (err)
+    {
+      grub_netbuff_free (nb);
+      return err;
+    }
+
+  v6 = (struct grub_net_dhcpv6_packet *) nb->data;
+  v6->message_type = GRUB_DHCPv6_REQUEST;
+  v6->transaction_id = v6_adv->transaction_id;
+
+  err = grub_netbuff_push (nb, sizeof (*udph));
+  if (err)
+    {
+      grub_netbuff_free (nb);
+      return err;
+    }
+
+  udph = (struct udphdr *) nb->data;
+  udph->src = grub_cpu_to_be16_compile_time (546);
+  udph->dst = grub_cpu_to_be16_compile_time (547);
+  udph->chksum = 0;
+  udph->len = grub_cpu_to_be16 (nb->tail - nb->data);
+
+  udph->chksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_UDP,
+						 &inf->address,
+						 &multicast);
+  err = grub_net_send_ip_packet (inf, &multicast, &ll_multicast, nb,
+				 GRUB_NET_IP_UDP);
+
+  grub_netbuff_free (nb);
+
+  return err;
+}
+
+
+struct grub_net_network_level_interface *
+grub_net_configure_by_dhcpv6_reply (const char *name,
+	struct grub_net_card *card,
+	grub_net_interface_flags_t flags,
+	const struct grub_net_dhcpv6_packet *v6,
+	grub_size_t size,
+	int is_def,
+	char **device, char **path)
+{
+  grub_net_network_level_address_t addr;
+  grub_net_network_level_netaddress_t netaddr;
+  struct grub_net_network_level_interface *inf;
+  const grub_uint8_t *your_ip;
+  char *proto;
+  char *server_ip;
+  char *boot_file;
+  grub_net_network_level_address_t *dns;
+  grub_uint16_t num_dns;
+
+  if (device)
+    *device = NULL;
+
+  if (path)
+    *path = NULL;
+
+  if (v6->message_type != GRUB_DHCPv6_REPLY)
+    {
+      grub_error (GRUB_ERR_IO, N_("DHCPv6 info not found"));
+      return NULL;
+    }
+
+  your_ip = find_dhcpv6_address(v6, size);
+
+  if (!your_ip)
+    return NULL;
+
+  addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
+  addr.ipv6[0] = grub_get_unaligned64 (your_ip);
+  addr.ipv6[1] = grub_get_unaligned64 (your_ip + 8);
+  inf = grub_net_add_addr (name, card, &addr, &card->default_address, flags);
+
+  netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
+  netaddr.ipv6.base[0] = grub_get_unaligned64 (your_ip);
+  netaddr.ipv6.base[1] = 0;
+  netaddr.ipv6.masksize = 64;
+  grub_net_add_route (name, netaddr, inf);
+
+  get_dhcpv6_dns_address (v6, size, &dns, &num_dns);
+
+  if (grub_errno)
+    grub_errno = GRUB_ERR_NONE;
+
+  if (dns && num_dns)
+    {
+      int i;
+
+      for (i = 0; i < num_dns; ++i)
+	grub_net_add_dns_server (dns + i);
+
+      grub_free (dns);
+    }
+
+  find_dhcpv6_bootfile_url (v6, size, &proto, &server_ip, &boot_file);
+
+  if (grub_errno)
+    grub_print_error ();
+
+  if (boot_file)
+    grub_env_set_net_property (name, "boot_file", boot_file,
+			  grub_strlen (boot_file));
+
+  if (is_def && server_ip)
+    {
+      grub_net_default_server = grub_strdup (server_ip);
+      grub_env_set ("net_default_interface", name);
+      grub_env_export ("net_default_interface");
+    }
+
+  if (device && server_ip && proto)
+    {
+      *device = grub_xasprintf ("%s,%s", proto, server_ip);
+      if (!*device)
+	grub_print_error ();
+    }
+
+  if (path && boot_file)
+    {
+      *path = grub_strdup (boot_file);
+      if (*path)
+	{
+	  char *slash;
+	  slash = grub_strrchr (*path, '/');
+	  if (slash)
+	    *slash = 0;
+	  else
+	    **path = 0;
+	}
+      else
+	grub_print_error ();
+    }
+
+  if (proto)
+    grub_free (proto);
+
+  if (server_ip)
+    grub_free (server_ip);
+
+  if (boot_file)
+    grub_free (boot_file);
+
+  return inf;
+}
+
 void
 grub_net_process_dhcp (struct grub_net_buff *nb,
 		       struct grub_net_card *card)
@@ -288,6 +930,70 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
     }
 }
 
+void
+grub_net_process_dhcp6 (struct grub_net_buff *nb,
+	struct grub_net_card *card __attribute__ ((unused)))
+{
+  const struct grub_net_dhcpv6_packet *v6;
+  struct grub_dhcpv6_session *session;
+  const struct grub_dhcpv6_option *opt_iana;
+  const struct grub_dhcpv6_iana_option *ia_na;
+  grub_size_t size;
+
+  v6 = (const struct grub_net_dhcpv6_packet *) nb->data;
+
+  size = nb->tail - nb->data;
+
+  opt_iana = find_dhcpv6_option_in_packet (v6, size, GRUB_DHCPv6_OPTION_IA_NA);
+  if (!opt_iana)
+    {
+      grub_print_error ();
+      return;
+    }
+
+  ia_na = (const struct grub_dhcpv6_iana_option *)opt_iana->data;
+  FOR_DHCPV6_SESSIONS (session)
+    {
+      if (session->transaction_id == v6->transaction_id
+	  && session->iaid == grub_cpu_to_be32 (ia_na->iaid))
+	break;
+    }
+
+  if (!session)
+    return;
+
+  if (v6->message_type == GRUB_DHCPv6_ADVERTISE)
+    {
+      grub_net_configure_by_dhcpv6_adv (v6, size, session);
+    }
+  else if (v6->message_type == GRUB_DHCPv6_REPLY)
+    {
+      char *name;
+      struct grub_net_network_level_interface *inf;
+
+      inf = session->iface;
+      name = grub_xasprintf ("%s:dhcp6", inf->card->name);
+      if (!name)
+	return;
+
+      grub_net_configure_by_dhcpv6_reply (name, inf->card,
+	  0, v6, size, 0, 0, 0);
+
+      if (!grub_errno)
+	{
+	  grub_list_remove (GRUB_AS_LIST (session));
+	  grub_free (session);
+	}
+
+      grub_free (name);
+    }
+
+  if (grub_errno)
+    grub_print_error ();
+
+  return;
+}
+
 static char
 hexdigit (grub_uint8_t val)
 {
@@ -564,7 +1270,190 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
   return err;
 }
 
-static grub_command_t cmd_getdhcp, cmd_bootp;
+
+static grub_err_t
+grub_cmd_bootp6 (struct grub_command *cmd __attribute__ ((unused)),
+	int argc, char **args)
+{
+  struct grub_net_card *card;
+  unsigned j = 0;
+  int interval;
+  grub_err_t err;
+  struct grub_dhcpv6_session *session;
+
+  err = GRUB_ERR_NONE;
+
+  FOR_NET_CARDS (card)
+  {
+    if (argc > 0 && grub_strcmp (card->name, args[0]) != 0)
+      continue;
+  }
+
+  FOR_NET_CARDS (card)
+  {
+    struct grub_net_network_level_interface *iface;
+
+    if (argc > 0 && grub_strcmp (card->name, args[0]) != 0)
+      continue;
+
+    iface = grub_net_ipv6_get_link_local (card, &card->default_address);
+    if (!iface)
+      {
+	grub_dhcpv6_sessions_free ();
+	return grub_errno;
+      }
+
+    session = grub_zalloc (sizeof (*session));
+    session->iface = iface;
+    session->iaid = j;
+    grub_dhcpv6_session_add (session);
+    j++;
+  }
+
+  for (interval = 200; interval < 10000; interval *= 2)
+    {
+      int done = 1;
+
+      FOR_DHCPV6_SESSIONS (session)
+	{
+	  struct grub_net_buff *nb;
+	  struct grub_dhcpv6_option *opt;
+	  struct grub_net_dhcpv6_packet *v6;
+	  struct grub_DUID_LL *duid;
+	  struct grub_dhcpv6_iana_option *ia_na;
+	  grub_net_network_level_address_t multicast;
+	  grub_net_link_level_address_t ll_multicast;
+	  struct udphdr *udph;
+
+	  multicast.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
+	  multicast.ipv6[0] = grub_cpu_to_be64_compile_time (0xff02ULL << 48);
+	  multicast.ipv6[1] = grub_cpu_to_be64_compile_time (0x10002ULL);
+
+	  err = grub_net_link_layer_resolve (session->iface,
+		    &multicast, &ll_multicast);
+	  if (err)
+	    {
+	      grub_dhcpv6_sessions_free ();
+	      return err;
+	    }
+
+	  nb = grub_netbuff_alloc (GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
+
+	  if (!nb)
+	    {
+	      grub_dhcpv6_sessions_free ();
+	      return grub_errno;
+	    }
+
+	  err = grub_netbuff_reserve (nb, GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
+	  if (err)
+	    {
+	      grub_dhcpv6_sessions_free ();
+	      grub_netbuff_free (nb);
+	      return err;
+	    }
+
+	  err = grub_netbuff_push (nb, sizeof (*opt) + sizeof (grub_uint16_t));
+	  if (err)
+	    {
+	      grub_dhcpv6_sessions_free ();
+	      grub_netbuff_free (nb);
+	      return err;
+	    }
+
+	  opt = (struct grub_dhcpv6_option *)nb->data;
+	  opt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_ELAPSED_TIME);
+	  opt->len = grub_cpu_to_be16_compile_time (sizeof (grub_uint16_t));
+	  grub_set_unaligned16 (opt->data, 0);
+
+	  err = grub_netbuff_push (nb, sizeof (*opt) + sizeof (*duid));
+	  if (err)
+	    {
+	      grub_dhcpv6_sessions_free ();
+	      grub_netbuff_free (nb);
+	      return err;
+	    }
+
+	  opt = (struct grub_dhcpv6_option *)nb->data;
+	  opt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_CLIENTID);
+	  opt->len = grub_cpu_to_be16 (sizeof (*duid));
+
+	  duid = (struct grub_DUID_LL *) opt->data;
+
+	  duid->type = grub_cpu_to_be16_compile_time (3) ;
+	  duid->hw_type = grub_cpu_to_be16_compile_time (1);
+	  grub_memcpy (&duid->hwaddr, &session->iface->hwaddress.mac,
+	      sizeof (session->iface->hwaddress.mac));
+
+	  err = grub_netbuff_push (nb, sizeof (*opt) + sizeof (*ia_na));
+	  if (err)
+	    {
+	      grub_dhcpv6_sessions_free ();
+	      grub_netbuff_free (nb);
+	      return err;
+	    }
+
+	  opt = (struct grub_dhcpv6_option *)nb->data;
+	  opt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_IA_NA);
+	  opt->len = grub_cpu_to_be16 (sizeof (*ia_na));
+	  ia_na = (struct grub_dhcpv6_iana_option *)opt->data;
+	  ia_na->iaid = grub_cpu_to_be32 (session->iaid);
+	  ia_na->t1 = 0;
+	  ia_na->t2 = 0;
+
+	  err = grub_netbuff_push (nb, sizeof (*v6));
+	  if (err)
+	    {
+	      grub_dhcpv6_sessions_free ();
+	      grub_netbuff_free (nb);
+	      return err;
+	    }
+
+	  v6 = (struct grub_net_dhcpv6_packet *)nb->data;
+	  v6->message_type = 1;
+	  v6->transaction_id = session->transaction_id;
+
+	  grub_netbuff_push (nb, sizeof (*udph));
+
+	  udph = (struct udphdr *) nb->data;
+	  udph->src = grub_cpu_to_be16_compile_time (546);
+	  udph->dst = grub_cpu_to_be16_compile_time (547);
+	  udph->chksum = 0;
+	  udph->len = grub_cpu_to_be16 (nb->tail - nb->data);
+
+	  udph->chksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_UDP,
+			    &session->iface->address, &multicast);
+
+	  err = grub_net_send_ip_packet (session->iface, &multicast,
+		    &ll_multicast, nb, GRUB_NET_IP_UDP);
+	  done = 0;
+	  grub_netbuff_free (nb);
+
+	  if (err)
+	    {
+	      grub_dhcpv6_sessions_free ();
+	      return err;
+	    }
+	}
+      if (!done)
+	grub_net_poll_cards (interval, 0);
+    }
+
+  FOR_DHCPV6_SESSIONS (session)
+    {
+      grub_error_push ();
+      err = grub_error (GRUB_ERR_FILE_NOT_FOUND,
+			N_("couldn't autoconfigure %s"),
+			session->iface->card->name);
+      grub_list_remove (GRUB_AS_LIST (session));
+      grub_free (session);
+      session = grub_dhcpv6_sessions;
+    }
+
+  return err;
+}
+
+static grub_command_t cmd_getdhcp, cmd_bootp, cmd_bootp6;
 
 void
 grub_bootp_init (void)
@@ -575,6 +1464,9 @@ grub_bootp_init (void)
   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."));
+  cmd_bootp6 = grub_register_command ("net_bootp6", grub_cmd_bootp6,
+				     N_("[CARD]"),
+				     N_("perform a DHCPv6 autoconfiguration"));
 }
 
 void
@@ -582,4 +1474,5 @@ grub_bootp_fini (void)
 {
   grub_unregister_command (cmd_getdhcp);
   grub_unregister_command (cmd_bootp);
+  grub_unregister_command (cmd_bootp6);
 }
diff --git a/grub-core/net/ip.c b/grub-core/net/ip.c
index 8c56baa..07d6871 100644
--- a/grub-core/net/ip.c
+++ b/grub-core/net/ip.c
@@ -238,6 +238,41 @@ handle_dgram (struct grub_net_buff *nb,
   {
     struct udphdr *udph;
     udph = (struct udphdr *) nb->data;
+
+    if (proto == GRUB_NET_IP_UDP && udph->dst == grub_cpu_to_be16_compile_time (546))
+      {
+	if (udph->chksum)
+	  {
+	    grub_uint16_t chk, expected;
+	    chk = udph->chksum;
+	    udph->chksum = 0;
+	    expected = grub_net_ip_transport_checksum (nb,
+						       GRUB_NET_IP_UDP,
+						       source,
+						       dest);
+	    if (expected != chk)
+	      {
+		grub_dprintf ("net", "Invalid UDP checksum. "
+			      "Expected %x, got %x\n",
+			      grub_be_to_cpu16 (expected),
+			      grub_be_to_cpu16 (chk));
+		grub_netbuff_free (nb);
+		return GRUB_ERR_NONE;
+	      }
+	    udph->chksum = chk;
+	  }
+
+	err = grub_netbuff_pull (nb, sizeof (*udph));
+	if (err)
+	  {
+	    grub_netbuff_free (nb);
+	    return err;
+	  }
+	grub_net_process_dhcp6 (nb, card);
+	grub_netbuff_free (nb);
+	return GRUB_ERR_NONE;
+      }
+
     if (proto == GRUB_NET_IP_UDP && grub_be_to_cpu16 (udph->dst) == 68)
       {
 	const struct grub_net_bootp_packet *bootp;
diff --git a/include/grub/net.h b/include/grub/net.h
index 538baa3..71dc243 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -418,6 +418,13 @@ struct grub_net_bootp_packet
   grub_uint8_t vendor[0];
 } GRUB_PACKED;
 
+struct grub_net_dhcpv6_packet
+{
+  grub_uint32_t message_type:8;
+  grub_uint32_t transaction_id:24;
+  grub_uint8_t dhcp_options[0];
+} GRUB_PACKED;
+
 #define	GRUB_NET_BOOTP_RFC1048_MAGIC_0	0x63
 #define	GRUB_NET_BOOTP_RFC1048_MAGIC_1	0x82
 #define	GRUB_NET_BOOTP_RFC1048_MAGIC_2	0x53
@@ -444,6 +451,14 @@ grub_net_configure_by_dhcp_ack (const char *name,
 				grub_size_t size,
 				int is_def, char **device, char **path);
 
+struct grub_net_network_level_interface *
+grub_net_configure_by_dhcpv6_reply (const char *name,
+				    struct grub_net_card *card,
+				    grub_net_interface_flags_t flags,
+				    const struct grub_net_dhcpv6_packet *v6,
+				    grub_size_t size,
+				    int is_def, char **device, char **path);
+
 grub_err_t
 grub_net_add_ipv4_local (struct grub_net_network_level_interface *inf,
 			 int mask);
@@ -452,6 +467,10 @@ void
 grub_net_process_dhcp (struct grub_net_buff *nb,
 		       struct grub_net_card *card);
 
+void
+grub_net_process_dhcp6 (struct grub_net_buff *nb,
+			struct grub_net_card *card);
+
 int
 grub_net_hwaddr_cmp (const grub_net_link_level_address_t *a,
 		     const grub_net_link_level_address_t *b);
-- 
1.7.3.4



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] UEFI IPv6 PXE support
  2015-05-12  8:49 [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE Michael Chang
  2015-05-12  8:49 ` [PATCH 1/3] Added net_bootp6 command Michael Chang
@ 2015-05-12  8:49 ` Michael Chang
  2015-05-12  8:49 ` [PATCH 3/3] Update document for net_bootp6 command Michael Chang
  2015-05-15  6:40 ` [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE Andrei Borzenkov
  3 siblings, 0 replies; 11+ messages in thread
From: Michael Chang @ 2015-05-12  8:49 UTC (permalink / raw)
  To: grub-devel

When system is booted from UEFI IPv6 PXE, the network interface can be
configured directly by cached DHCPv6 packet in firmware without doing it all
over again by contacting DHCP server.
---
 grub-core/net/drivers/efi/efinet.c |   24 ++++++++++++---
 include/grub/efi/api.h             |   55 +++++++++++++++++++++++++++++++++++-
 2 files changed, 73 insertions(+), 6 deletions(-)

diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
index c44d531..4b70171 100644
--- a/grub-core/net/drivers/efi/efinet.c
+++ b/grub-core/net/drivers/efi/efinet.c
@@ -349,11 +349,25 @@ 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,
-				    (struct grub_net_bootp_packet *)
-				    &pxe_mode->dhcp_ack,
-				    sizeof (pxe_mode->dhcp_ack),
-				    1, device, path);
+
+    if (pxe_mode->using_ipv6)
+      {
+	grub_net_configure_by_dhcpv6_reply (card->name, card, 0,
+					    (struct grub_net_dhcpv6_packet *)
+					    &pxe_mode->dhcp_ack,
+					    sizeof (pxe_mode->dhcp_ack),
+					    1, device, path);
+	if (grub_errno)
+	  grub_print_error ();
+      }
+    else
+      {
+	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);
+      }
     return;
   }
 }
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index e5dd543..e6684cd 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -1340,14 +1340,67 @@ typedef struct grub_efi_simple_text_output_interface grub_efi_simple_text_output
 
 typedef grub_uint8_t grub_efi_pxe_packet_t[1472];
 
+typedef struct {
+  grub_uint8_t addr[4];
+} grub_efi_pxe_ipv4_address_t;
+
+typedef struct {
+  grub_uint8_t addr[16];
+} grub_efi_pxe_ipv6_address_t;
+
+typedef struct {
+  grub_uint8_t addr[32];
+} grub_efi_pxe_mac_address_t;
+
+typedef union {
+    grub_uint32_t addr[4];
+    grub_efi_pxe_ipv4_address_t v4;
+    grub_efi_pxe_ipv6_address_t v6;
+} grub_efi_pxe_ip_address_t;
+
+#define GRUB_EFI_PXE_BASE_CODE_MAX_IPCNT 8
+typedef struct {
+    grub_uint8_t filters;
+    grub_uint8_t ip_cnt;
+    grub_uint16_t reserved;
+    grub_efi_pxe_ip_address_t ip_list[GRUB_EFI_PXE_BASE_CODE_MAX_IPCNT];
+} grub_efi_pxe_ip_filter_t;
+
+typedef struct {
+    grub_efi_pxe_ip_address_t ip_addr;
+    grub_efi_pxe_mac_address_t mac_addr;
+} grub_efi_pxe_arp_entry_t;
+
+typedef struct {
+    grub_efi_pxe_ip_address_t ip_addr;
+    grub_efi_pxe_ip_address_t subnet_mask;
+    grub_efi_pxe_ip_address_t gw_addr;
+} grub_efi_pxe_route_entry_t;
+
+
+#define GRUB_EFI_PXE_BASE_CODE_MAX_ARP_ENTRIES 8
+#define GRUB_EFI_PXE_BASE_CODE_MAX_ROUTE_ENTRIES 8
+
 typedef struct grub_efi_pxe_mode
 {
-  grub_uint8_t unused[52];
+  grub_uint8_t started;
+  grub_uint8_t ipv6_available;
+  grub_uint8_t ipv6_supported;
+  grub_uint8_t using_ipv6;
+  grub_uint8_t unused[16];
+  grub_efi_pxe_ip_address_t station_ip;
+  grub_efi_pxe_ip_address_t subnet_mask;
   grub_efi_pxe_packet_t dhcp_discover;
   grub_efi_pxe_packet_t dhcp_ack;
   grub_efi_pxe_packet_t proxy_offer;
   grub_efi_pxe_packet_t pxe_discover;
   grub_efi_pxe_packet_t pxe_reply;
+  grub_efi_pxe_packet_t pxe_bis_reply;
+  grub_efi_pxe_ip_filter_t ip_filter;
+  grub_uint32_t arp_cache_entries;
+  grub_efi_pxe_arp_entry_t arp_cache[GRUB_EFI_PXE_BASE_CODE_MAX_ARP_ENTRIES];
+  grub_uint32_t route_table_entries;
+  grub_efi_pxe_route_entry_t route_table[GRUB_EFI_PXE_BASE_CODE_MAX_ROUTE_ENTRIES];
 } grub_efi_pxe_mode_t;
 
 typedef struct grub_efi_pxe
-- 
1.7.3.4



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] Update document for net_bootp6 command
  2015-05-12  8:49 [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE Michael Chang
  2015-05-12  8:49 ` [PATCH 1/3] Added net_bootp6 command Michael Chang
  2015-05-12  8:49 ` [PATCH 2/3] UEFI IPv6 PXE support Michael Chang
@ 2015-05-12  8:49 ` Michael Chang
  2015-05-15  6:40 ` [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE Andrei Borzenkov
  3 siblings, 0 replies; 11+ messages in thread
From: Michael Chang @ 2015-05-12  8:49 UTC (permalink / raw)
  To: grub-devel

Update document for net_bootp6 command
---
 docs/grub.texi |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index 4af22af..72780e3 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -5110,6 +5110,7 @@ List available video modes. If resolution is given, show only matching modes.
 * net_add_dns::                 Add a DNS server
 * net_add_route::               Add routing entry
 * net_bootp::                   Perform a bootp autoconfiguration
+* net_bootp6::                  Perform a DHCPv6 autoconfiguration
 * net_del_addr::                Remove IP address from interface
 * net_del_dns::                 Remove a DNS server
 * net_del_route::               Remove a route entry
@@ -5189,6 +5190,22 @@ Sets environment variable @samp{net_}@var{<card>}@samp{_dhcp_extensionspath}
 
 @end deffn
 
+@node net_bootp6
+@subsection net_bootp6
+
+@deffn Command net_bootp6 [@var{card}]
+Perform configuration of @var{card} using DHCPv6 protocol. If no card name is
+specified, try to configure all existing cards. If configuration was
+successful, interface with name @var{card}@samp{:dhcp6} and configured address
+is added to @var{card}.
+
+@table @samp
+@item 1 (Domain Name Server)
+Adds all servers from option value to the list of servers used during name
+resolution.
+@end table
+
+@end deffn
 
 @node net_del_addr
 @subsection net_del_addr
-- 
1.7.3.4



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] Added net_bootp6 command
  2015-05-12  8:49 ` [PATCH 1/3] Added net_bootp6 command Michael Chang
@ 2015-05-15  6:26   ` Andrei Borzenkov
  2015-05-15 13:57     ` Michael Chang
  2015-05-19  8:42     ` Michael Chang
  0 siblings, 2 replies; 11+ messages in thread
From: Andrei Borzenkov @ 2015-05-15  6:26 UTC (permalink / raw)
  To: Michael Chang; +Cc: grub-devel

В Tue, 12 May 2015 16:49:48 +0800
Michael Chang <mchang@suse.com> пишет:

> The net_bootp6 is used to configure the ipv6 network interface through the
> DHCPv6 protocol Solict/Advertise/Request/Reply.
> ---
>  grub-core/net/bootp.c |  895 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  grub-core/net/ip.c    |   35 ++
>  include/grub/net.h    |   19 +
>  3 files changed, 948 insertions(+), 1 deletions(-)
> 
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index 6136755..5c5eb6f 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -24,6 +24,8 @@
>  #include <grub/net/netbuff.h>
>  #include <grub/net/udp.h>
>  #include <grub/datetime.h>
> +#include <grub/time.h>
> +#include <grub/list.h>
>  
>  static void
>  parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
> @@ -256,6 +258,646 @@ grub_net_configure_by_dhcp_ack (const char *name,
>    return inter;
>  }
>  
> +struct grub_dhcpv6_option {
> +  grub_uint16_t code;
> +  grub_uint16_t len;

Won't do; options in packet are unaligned (see below) so you have to
walk byte buffer using get_unaligned and

grub_uint8_t code[2]
grub_uint8_t len[2]

> +  grub_uint8_t data[0];
> +} GRUB_PACKED;
> +
> +
> +struct grub_dhcpv6_iana_option {
> +  grub_uint32_t iaid;
> +  grub_uint32_t t1;
> +  grub_uint32_t t2;
> +  grub_uint8_t data[0];
> +} GRUB_PACKED;
> +
> +struct grub_dhcpv6_iaaddr_option {
> +  grub_uint8_t addr[16];
> +  grub_uint32_t preferred_lifetime;
> +  grub_uint32_t valid_lifetime;
> +  grub_uint8_t data[0];
> +} GRUB_PACKED;
> +
> +struct grub_DUID_LL
> +{
> +  grub_uint16_t type;
> +  grub_uint16_t hw_type;
> +  grub_uint8_t hwaddr[6];
> +} GRUB_PACKED;
> +

Probably the same applies to all option definitions.

> +#define GRUB_DHCPv6_ADVERTISE 2
> +#define GRUB_DHCPv6_REQUEST 3
> +#define GRUB_DHCPv6_REPLY 7
> +

SOLICIT is missing; please enum as well.

> +#define GRUB_DHCPv6_OPTION_CLIENTID 1
> +#define GRUB_DHCPv6_OPTION_SERVERID 2
> +#define GRUB_DHCPv6_OPTION_IA_NA 3
> +#define GRUB_DHCPv6_OPTION_IAADDR 5
> +#define GRUB_DHCPv6_OPTION_ORO 6
> +#define GRUB_DHCPv6_OPTION_ELAPSED_TIME 8
> +#define GRUB_DHCPv6_OPTION_DNS_SERVERS 23
> +#define GRUB_DHCPv6_OPTION_BOOTFILE_URL 59
> +

enum please. These can also be anonymous, no need to invent fancy
names.

> +/* The default netbuff size for sending DHCPv6 packets which should be
> +   large enough to hold the information */
> +#define GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE 512
> +
> +struct grub_dhcpv6_session
> +{
> +  struct grub_dhcpv6_session *next;
> +  struct grub_dhcpv6_session **prev;
> +  grub_uint32_t iaid;
> +  grub_uint32_t transaction_id:24;
> +  grub_uint64_t start_time;
> +  struct grub_net_network_level_interface *iface;
> +};
> +
> +static struct grub_dhcpv6_session *grub_dhcpv6_sessions = NULL;
> +#define FOR_DHCPV6_SESSIONS(var) FOR_LIST_ELEMENTS (var, grub_dhcpv6_sessions)
> +
> +static void
> +grub_dhcpv6_session_add (struct grub_dhcpv6_session *session)
> +{
> +  struct grub_datetime date;
> +  grub_err_t err;
> +  grub_int32_t t = 0;
> +
> +  err = grub_get_datetime (&date);
> +  if (err || !grub_datetime2unixtime (&date, &t))
> +    {
> +      grub_errno = GRUB_ERR_NONE;
> +      t = 0;
> +    }
> +
> +  session->transaction_id = t;
> +  session->start_time = grub_get_time_ms ();
> +  grub_list_push (GRUB_AS_LIST_P (&grub_dhcpv6_sessions), GRUB_AS_LIST (session));
> +
> +  return;

return is redundant

> +}
> +
> +static void
> +grub_dhcpv6_sessions_free (void)
> +{
> +  struct grub_dhcpv6_session *session;
> +
> +  FOR_DHCPV6_SESSIONS (session)
> +    {
> +      grub_list_remove (GRUB_AS_LIST (session));
> +      grub_free (session);
> +      session = grub_dhcpv6_sessions;
> +    }
> +
> +  return;

and here too

> +}
> +
> +static const char *
> +get_dhcpv6_option_name (grub_uint16_t option)
> +{
> +  switch (option)
> +    {
> +    case GRUB_DHCPv6_OPTION_BOOTFILE_URL:
> +      return "BOOTFILE URL";
> +    case GRUB_DHCPv6_OPTION_DNS_SERVERS:
> +      return "DNS SERVERS";
> +    case GRUB_DHCPv6_OPTION_IA_NA:
> +      return "IA NA";
> +    case GRUB_DHCPv6_OPTION_IAADDR:
> +      return "IAADDR";
> +    case GRUB_DHCPv6_OPTION_CLIENTID:
> +      return "CLIENTID";
> +    case GRUB_DHCPv6_OPTION_SERVERID:
> +      return "SERVERID";
> +    case GRUB_DHCPv6_OPTION_ORO:
> +      return "ORO";
> +    case GRUB_DHCPv6_OPTION_ELAPSED_TIME:
> +      return "ELAPSED TIME";
> +    default:
> +      return "UNKNOWN";
> +    }
> +}
> +
> +static const struct grub_dhcpv6_option*
> +find_dhcpv6_option (const struct grub_dhcpv6_option *popt, grub_size_t size, grub_uint16_t option)
> +{
> +  while (size > 0)
> +    {
> +      grub_uint16_t code, len;
> +

if (size < sizeof(*popt))
  grub_error

> +      code = grub_be_to_cpu16 (popt->code);
> +      len = grub_be_to_cpu16 (popt->len) + sizeof (*popt);
> +

According to rfc3315 - Options are byte-aligned but are not aligned in
any other way such as on 2 or 4 byte boundaries; it should be
grub_get_unaligned.

To avoid warnings from static checkers (possible overflow) 

size -= sizeof(*popt)
len = grub_be_to_cpu...

> +      if (size < len)
> +	{
> +	  grub_error (GRUB_ERR_OUT_OF_RANGE, N_("DHCPv6 option size overflow detected"));
> +	  return NULL;
> +	}
> +
> +      if (option == code)
> +	return popt;
> +
> +      if (code == 0)
> +	break;
> +      else
> +	{
> +	  size -= len;
> +	  popt = (const struct grub_dhcpv6_option *)((grub_uint8_t *)popt + len);

assuming changes above - + sizeof (*popt)

> +	}
> +    }
> +
> +  grub_error (GRUB_ERR_IO, N_("DHCPv6 Option (%u):%s not found"), option, get_dhcpv6_option_name(option));
> +  return NULL;
> +}
> +
> +static const struct grub_dhcpv6_option*
> +find_dhcpv6_option_in_packet (const struct grub_net_dhcpv6_packet *packet,
> +    grub_size_t size, grub_uint16_t option)
> +{
> +  if (!size || !packet)
> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("null or zero sized DHCPv6 packet buffer"));
> +      return NULL;
> +    }
> +

Is it really possible? We come here from processing real packet or
from EFI with fixed buffer. Just curious in case I miss something
obvious.

> +  if (size <= sizeof (*packet))
> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("DHCPv6 packet size too small"));
> +      return NULL;
> +    }
> +

You need to check size in grub_net_process_dhcp6() anyway. When you come
from EFI you have fixed buffer size and packet exists so there is single
entry point where size has to be verified. No need to do it every time
here.

> +  return find_dhcpv6_option ((const struct grub_dhcpv6_option *)packet->dhcp_options,
> +	  size - sizeof (*packet), option);
> +}
> +
> +static const grub_uint8_t*
> +find_dhcpv6_address (const struct grub_net_dhcpv6_packet *packet, grub_size_t size)
> +{
> +  const struct grub_dhcpv6_option* popt;
> +  const struct grub_dhcpv6_iana_option *ia_na;
> +  const struct grub_dhcpv6_iaaddr_option *iaaddr;
> +  grub_uint16_t ia_na_len;
> +
> +  popt = find_dhcpv6_option_in_packet (packet, size, GRUB_DHCPv6_OPTION_IA_NA);
> +
> +  if (!popt)
> +    return NULL;
> +
> +  ia_na = (const struct grub_dhcpv6_iana_option *) popt->data;
> +  ia_na_len = grub_be_to_cpu16 (popt->len);

grub_get_unaligned. It makes sense to add helper for option length,
it's going to be used often.

> +
> +  popt = find_dhcpv6_option ((const struct grub_dhcpv6_option *) ia_na->data,
> +      ia_na_len - sizeof (*ia_na), GRUB_DHCPv6_OPTION_IAADDR);
> +
> +  if (!popt)
> +    return NULL;
> +
> +  iaaddr = (const struct grub_dhcpv6_iaaddr_option *)popt->data;
> +  return iaaddr->addr;
> +}
> +
> +static void
> +get_dhcpv6_dns_address (const struct grub_net_dhcpv6_packet *packet, grub_size_t size,
> +	grub_net_network_level_address_t **addr, grub_uint16_t *naddr)
> +{
> +  const struct grub_dhcpv6_option* popt;
> +  grub_uint16_t len, ln;
> +  const grub_uint8_t *pa;
> +  grub_net_network_level_address_t *la;
> +
> +  if (!addr || !naddr)
> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("bad argument for get_dhcpv6_dns_address"));
> +      return;
> +    }
> +

it is called exactly once and grub_errno is immediately reset to
GRUB_ERR_NONE. So what's the point of returning error at all?

> +  *addr = NULL;
> +  *naddr = 0;
> +
> +  popt = find_dhcpv6_option_in_packet (packet, size, GRUB_DHCPv6_OPTION_DNS_SERVERS);
> +  if (!popt)
> +    return;
> +
> +  len = grub_be_to_cpu16 (popt->len);

unaligned 

> +  if (len == 0 || len & 0xf)
> +    {
> +      grub_error (GRUB_ERR_IO, N_("invalid dns address length"));
> +      return;
> +    }
> +

same question about grub_error. May be grub_debug?

> +  *naddr = ln = len >> 4;
> +  *addr = la =  grub_zalloc (sizeof (grub_net_network_level_address_t) * ln);

You are overwriting it immediately, no need to zalloc

> +
> +  if (!la)
> +    {
> +      *addr = NULL;
We just checked it is NULL above

> +      *naddr = 0;

Just move assignment after NULL check

> +      return;
> +    }
> +
> +  for (pa = popt->data; ln > 0; pa += 0x10, la++, ln--)
> +    {
> +      la->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> +      la->ipv6[0] = grub_get_unaligned64 (pa);
> +      la->ipv6[1] = grub_get_unaligned64 (pa + 8);
> +      la->option = DNS_OPTION_PREFER_IPV6;
> +    }
> +
> +  return;

redundant return

> +}
> +
> +static void
> +find_dhcpv6_bootfile_url (const struct grub_net_dhcpv6_packet *packet, grub_size_t size,
> +	char **proto, char **server_ip, char **boot_file)
> +{
> +  char *bootfile_url;
> +  const struct grub_dhcpv6_option* opt_url;
> +  char *ip_start, *ip_end;
> +  char *path;
> +  grub_size_t ip_len;
> +  grub_uint16_t len;
> +  const char *protos[] = {"tftp://", "http://", NULL};
> +  const char *pr;
> +  int i;
> +
> +  if (proto)
> +    *proto = NULL;
> +
> +  if (server_ip)
> +    *server_ip = NULL;
> +
> +  if (boot_file)
> +    *boot_file = NULL;
> +
> +  opt_url = find_dhcpv6_option_in_packet (packet, size, GRUB_DHCPv6_OPTION_BOOTFILE_URL);
> +
> +  if (!opt_url)
> +    return;
> +
> +  len = grub_be_to_cpu16 (opt_url->len);
> +

get_unaligned

> +  bootfile_url = grub_malloc (len + 1);
> +
> +  if (!bootfile_url)
> +    return;
> +
> +  grub_memcpy (bootfile_url, opt_url->data, len);
> +  bootfile_url[len]   = '\0';
> +
> +  for (i = 0; (pr = *(protos + i)); ++i)
for (pr = protos; *pr; pr++)

> +      if (grub_strncmp (bootfile_url, pr, grub_strlen(pr)) == 0)
> +	break;
> +
> +  if (!pr)
> +    {
> +      grub_error (GRUB_ERR_IO,
> +	N_("unsupported protocol, only tftp and http are supported"));
> +      goto cleanup;
> +    }
> +
> +  ip_start = ip_end = NULL;
> +  ip_start = bootfile_url + grub_strlen(pr);
> +
> +  /* Follow elilo and edk2 that check for starting and ending delimiter '[..]'
> +     in which IPv6 server address is enclosed. */
> +  if (*ip_start != '[')
> +    ip_start = NULL;
> +  else
> +    ip_end = grub_strchr (++ip_start, ']');
> +
> +  if (!ip_start || !ip_end)
> +    {
> +      grub_error (GRUB_ERR_IO, N_("IPv6-address not in square brackets"));
> +      goto cleanup;
> +    }
> +

RFC5970 says "Clients that have DNS implementations SHOULD support the
use of domain names in the URL" and in general string can also be valid
IPv4 address, nothing restricts it to IPv6. So I do not think you
should return error here, just return full string. I wish grub supported
IPv6 literals so we could just skip this check entirely.

> +  ip_len = ip_end - ip_start;
> +
> +  if (proto)
> +    {
> +      grub_size_t proto_len  = grub_strlen (pr) - 3;
> +
> +      *proto = grub_malloc (proto_len + 1);
> +      if (!*proto)
> +	goto cleanup;
> +
> +      grub_memcpy (*proto, pr, proto_len);
> +      *(*proto + proto_len)  = '\0';
> +    }
> +
> +  if (server_ip)
> +    {
> +      *server_ip = grub_malloc (ip_len + 1);
> +
> +      if (!*server_ip)
> +	goto cleanup;
> +
> +      grub_memcpy (*server_ip, ip_start, ip_len);
> +      *(*server_ip + ip_len) = '\0';
> +    }
> +
> +  path = ip_end + 1;
> +
> +  if (boot_file)
> +    {
> +      *boot_file = grub_strdup (path);
> +
> +      if (!*boot_file)
> +	goto cleanup;
> +    }
> +
> +cleanup:
> +
> +  if (bootfile_url)
> +    grub_free (bootfile_url);
> +
> +  if (grub_errno)
> +    {
> +      if (proto && *proto)
> +	{
> +	  grub_free (*proto);
> +	  *proto = NULL;
> +	}
> +
> +      if (server_ip && *server_ip)
> +	{
> +	  grub_free (*server_ip);
> +	  *server_ip = NULL;
> +	}
> +
> +      if (boot_file && *boot_file)
> +	{
> +	  grub_free (*boot_file);
> +	  *boot_file = NULL;
> +	}
> +    }
> +
> +  return;

redundant return

> +}
> +
> +
> +static grub_err_t
> +grub_net_configure_by_dhcpv6_adv (const struct grub_net_dhcpv6_packet *v6_adv,
> +	grub_size_t size,
> +	struct grub_dhcpv6_session *session)
> +{
> +  struct grub_net_buff *nb;
> +  const struct grub_dhcpv6_option *opt_client, *opt_server, *opt_iana;
> +  struct grub_dhcpv6_option *popt;
> +  struct grub_net_dhcpv6_packet *v6;
> +  struct udphdr *udph;
> +  grub_net_network_level_address_t multicast;
> +  grub_net_link_level_address_t ll_multicast;
> +  struct grub_net_network_level_interface *inf;
> +  grub_uint16_t len;
> +  grub_uint64_t elapsed;
> +  grub_err_t err = GRUB_ERR_NONE;
> +
> +  opt_client = find_dhcpv6_option_in_packet (v6_adv, size, GRUB_DHCPv6_OPTION_CLIENTID);
> +  if (grub_errno)
> +    grub_error_push ();
> +

Should not we check that is it actually one of our sessions? From
RFC3315 validation

   -  the contents of the Client Identifier option does not match the
      client's DUID.

   -  the "transaction-id" field value does not match the value the
      client used in its Solicit message.

transaction ID is checked earlier, we also need to check client ID
somewhere

> +  opt_server = find_dhcpv6_option_in_packet (v6_adv, size, GRUB_DHCPv6_OPTION_SERVERID);
> +  if (grub_errno)
> +    grub_error_push ();
> +
> +  opt_iana = find_dhcpv6_option_in_packet (v6_adv, size, GRUB_DHCPv6_OPTION_IA_NA);
> +  if (grub_errno)
> +    grub_error_push ();
> +
> +  grub_error_pop ();
> +  if (grub_errno)
> +    return grub_errno;
> +

Do we really need this complication? Why not just return on the first
missing option?

> +  inf = session->iface;
> +
> +  multicast.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> +  multicast.ipv6[0] = grub_cpu_to_be64_compile_time (0xff02ULL << 48);
> +  multicast.ipv6[1] = grub_cpu_to_be64_compile_time (0x10002ULL);
> +
> +  err = grub_net_link_layer_resolve (inf, &multicast, &ll_multicast);
> +  if (err)
> +    return err;
> +
> +  nb = grub_netbuff_alloc (GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
> +
> +  if (!nb)
> +    return grub_errno;
> +
> +  err = grub_netbuff_reserve (nb, GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
> +  if (err)
> +    {
> +      grub_netbuff_free (nb);
> +      return err;
> +    }
> +
> +  len = grub_cpu_to_be16(opt_client->len);
get_unaligned, grub_be_to_cpu16

> +  err = grub_netbuff_push (nb, len + sizeof (*opt_client));
> +  if (err)
> +    {
> +      grub_netbuff_free (nb);
> +      return err;
> +    }
> +  grub_memcpy (nb->data, opt_client, len + sizeof (*opt_client));
> +
> +  len = grub_cpu_to_be16(opt_server->len);

ditto

> +  err = grub_netbuff_push (nb, len + sizeof (*opt_server));
> +  if (err)
> +    {
> +      grub_netbuff_free (nb);
> +      return err;
> +    }
> +  grub_memcpy (nb->data, opt_server, len + sizeof (*opt_server));
> +
> +  len = grub_cpu_to_be16(opt_iana->len);

ditto

> +  err = grub_netbuff_push (nb, len + sizeof (*opt_iana));
> +  if (err)
> +    {
> +      grub_netbuff_free (nb);
> +      return err;
> +    }
> +  grub_memcpy (nb->data, opt_iana, len + sizeof (*opt_iana));
> +
> +  err = grub_netbuff_push (nb, sizeof (*popt) + 2 * sizeof (grub_uint16_t));
> +  if (err)
> +    {
> +      grub_netbuff_free (nb);
> +      return err;
> +    }
> +
> +  popt = (struct grub_dhcpv6_option*) nb->data;
> +  popt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_ORO);

From here it starts to be interesting. At least server ID option is out
of our control so everything after it may be unaligned.

> +  popt->len = grub_cpu_to_be16_compile_time (2 * sizeof (grub_uint16_t));

unaligned?

> +  grub_set_unaligned16 (popt->data, grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_BOOTFILE_URL));
> +  grub_set_unaligned16 (popt->data + 2, grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_DNS_SERVERS));
> +
> +  err = grub_netbuff_push (nb, sizeof (*popt) + sizeof (grub_uint16_t));
> +  if (err)
> +    {
> +      grub_netbuff_free (nb);
> +      return err;
> +    }
> +  popt = (struct grub_dhcpv6_option*) nb->data;
> +  popt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_ELAPSED_TIME);
> +  popt->len = grub_cpu_to_be16_compile_time (sizeof (grub_uint16_t));
> +

unaligned?

> +  /* the time is expressed in hundredths of a second */
> +  elapsed = grub_divmod64 (grub_get_time_ms () - session->start_time, 10, 0);
> +
> +  if (elapsed > 0xffff)
> +    elapsed = 0xffff;
> +
> +  grub_set_unaligned16 (popt->data,  grub_cpu_to_be16 ((grub_uint16_t)elapsed));
> +
> +  err = grub_netbuff_push (nb, sizeof (*v6));
> +  if (err)
> +    {
> +      grub_netbuff_free (nb);
> +      return err;
> +    }
> +
> +  v6 = (struct grub_net_dhcpv6_packet *) nb->data;
> +  v6->message_type = GRUB_DHCPv6_REQUEST;
> +  v6->transaction_id = v6_adv->transaction_id;
Not sure how compiler handles alignment in this case.

> +
> +  err = grub_netbuff_push (nb, sizeof (*udph));
> +  if (err)
> +    {
> +      grub_netbuff_free (nb);
> +      return err;
> +    }
> +
> +  udph = (struct udphdr *) nb->data;
> +  udph->src = grub_cpu_to_be16_compile_time (546);
> +  udph->dst = grub_cpu_to_be16_compile_time (547);
> +  udph->chksum = 0;
> +  udph->len = grub_cpu_to_be16 (nb->tail - nb->data);
> +

And even here header may be unaligned after starting from server ID
option. I suggest to do it differently - compute total length, reserve
aligned space and make server ID last to make sure everything before it
is properly aligned. And add comment explaining it :)

> +  udph->chksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_UDP,
> +						 &inf->address,
> +						 &multicast);
> +  err = grub_net_send_ip_packet (inf, &multicast, &ll_multicast, nb,
> +				 GRUB_NET_IP_UDP);
> +
> +  grub_netbuff_free (nb);
> +
> +  return err;
> +}
> +
> +
> +struct grub_net_network_level_interface *
> +grub_net_configure_by_dhcpv6_reply (const char *name,
> +	struct grub_net_card *card,
> +	grub_net_interface_flags_t flags,
> +	const struct grub_net_dhcpv6_packet *v6,
> +	grub_size_t size,
> +	int is_def,
> +	char **device, char **path)
> +{
> +  grub_net_network_level_address_t addr;
> +  grub_net_network_level_netaddress_t netaddr;
> +  struct grub_net_network_level_interface *inf;
> +  const grub_uint8_t *your_ip;
> +  char *proto;
> +  char *server_ip;
> +  char *boot_file;
> +  grub_net_network_level_address_t *dns;
> +  grub_uint16_t num_dns;
> +
> +  if (device)
> +    *device = NULL;
> +
> +  if (path)
> +    *path = NULL;
> +
> +  if (v6->message_type != GRUB_DHCPv6_REPLY)
> +    {
> +      grub_error (GRUB_ERR_IO, N_("DHCPv6 info not found"));
> +      return NULL;
> +    }
> +
> +  your_ip = find_dhcpv6_address(v6, size);
> +
> +  if (!your_ip)
> +    return NULL;
> +
> +  addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> +  addr.ipv6[0] = grub_get_unaligned64 (your_ip);
> +  addr.ipv6[1] = grub_get_unaligned64 (your_ip + 8);
> +  inf = grub_net_add_addr (name, card, &addr, &card->default_address, flags);
> +

NULL check. It is not clear if it makes sense to continue if we failed
to actually set address at this point.

> +  netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> +  netaddr.ipv6.base[0] = grub_get_unaligned64 (your_ip);
> +  netaddr.ipv6.base[1] = 0;
> +  netaddr.ipv6.masksize = 64;
> +  grub_net_add_route (name, netaddr, inf);

This will crash in net_ls_routes later if inf was NULL. Yes, I know
that it is used this way currently, but that needs fixing in other
places as well.

> +
> +  get_dhcpv6_dns_address (v6, size, &dns, &num_dns);
> +
> +  if (grub_errno)
> +    grub_errno = GRUB_ERR_NONE;
> +

This ignores legitimate errors like out of memory. As mentioned above,
does it need to set any grub_errno on its own? If not, errors should
not be ignored.

> +  if (dns && num_dns)
> +    {
> +      int i;
> +
> +      for (i = 0; i < num_dns; ++i)
> +	grub_net_add_dns_server (dns + i);
> +
> +      grub_free (dns);
> +    }
> +
> +  find_dhcpv6_bootfile_url (v6, size, &proto, &server_ip, &boot_file);
> +
> +  if (grub_errno)
> +    grub_print_error ();
> +
> +  if (boot_file)
> +    grub_env_set_net_property (name, "boot_file", boot_file,
> +			  grub_strlen (boot_file));
> +
> +  if (is_def && server_ip)
> +    {
> +      grub_net_default_server = grub_strdup (server_ip);

NULL check

> +      grub_env_set ("net_default_interface", name);
> +      grub_env_export ("net_default_interface");
> +    }
> +
> +  if (device && server_ip && proto)
> +    {
> +      *device = grub_xasprintf ("%s,%s", proto, server_ip);
> +      if (!*device)
> +	grub_print_error ();
> +    }
> +
> +  if (path && boot_file)
> +    {
> +      *path = grub_strdup (boot_file);
> +      if (*path)
> +	{
> +	  char *slash;
> +	  slash = grub_strrchr (*path, '/');
> +	  if (slash)
> +	    *slash = 0;
> +	  else
> +	    **path = 0;
> +	}
> +      else
> +	grub_print_error ();
> +    }
> +
> +  if (proto)
> +    grub_free (proto);
> +
> +  if (server_ip)
> +    grub_free (server_ip);
> +
> +  if (boot_file)
> +    grub_free (boot_file);
> +
> +  return inf;
> +}
> +
>  void
>  grub_net_process_dhcp (struct grub_net_buff *nb,
>  		       struct grub_net_card *card)
> @@ -288,6 +930,70 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
>      }
>  }
>  
> +void
> +grub_net_process_dhcp6 (struct grub_net_buff *nb,
> +	struct grub_net_card *card __attribute__ ((unused)))
> +{
> +  const struct grub_net_dhcpv6_packet *v6;
> +  struct grub_dhcpv6_session *session;
> +  const struct grub_dhcpv6_option *opt_iana;
> +  const struct grub_dhcpv6_iana_option *ia_na;
> +  grub_size_t size;
> +
> +  v6 = (const struct grub_net_dhcpv6_packet *) nb->data;
> +
> +  size = nb->tail - nb->data;
> +

Size check here looks like the most natural place.

> +  opt_iana = find_dhcpv6_option_in_packet (v6, size, GRUB_DHCPv6_OPTION_IA_NA);
> +  if (!opt_iana)
> +    {
> +      grub_print_error ();
> +      return;
> +    }
> +
> +  ia_na = (const struct grub_dhcpv6_iana_option *)opt_iana->data;
> +  FOR_DHCPV6_SESSIONS (session)
> +    {
> +      if (session->transaction_id == v6->transaction_id
> +	  && session->iaid == grub_cpu_to_be32 (ia_na->iaid))

get_unaligned

> +	break;
> +    }
> +
> +  if (!session)
> +    return;
> +
> +  if (v6->message_type == GRUB_DHCPv6_ADVERTISE)
> +    {
> +      grub_net_configure_by_dhcpv6_adv (v6, size, session);
> +    }
> +  else if (v6->message_type == GRUB_DHCPv6_REPLY)
> +    {
> +      char *name;
> +      struct grub_net_network_level_interface *inf;
> +
> +      inf = session->iface;
> +      name = grub_xasprintf ("%s:dhcp6", inf->card->name);
> +      if (!name)
> +	return;
> +
> +      grub_net_configure_by_dhcpv6_reply (name, inf->card,
> +	  0, v6, size, 0, 0, 0);
> +
> +      if (!grub_errno)
> +	{
> +	  grub_list_remove (GRUB_AS_LIST (session));
> +	  grub_free (session);
> +	}
> +
> +      grub_free (name);
> +    }
> +
> +  if (grub_errno)
> +    grub_print_error ();
> +
> +  return;
> +}
> +
>  static char
>  hexdigit (grub_uint8_t val)
>  {
> @@ -564,7 +1270,190 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
>    return err;
>  }
>  
> -static grub_command_t cmd_getdhcp, cmd_bootp;
> +
> +static grub_err_t
> +grub_cmd_bootp6 (struct grub_command *cmd __attribute__ ((unused)),
> +	int argc, char **args)
> +{
> +  struct grub_net_card *card;
> +  unsigned j = 0;
> +  int interval;
> +  grub_err_t err;
> +  struct grub_dhcpv6_session *session;
> +
> +  err = GRUB_ERR_NONE;
> +
> +  FOR_NET_CARDS (card)
> +  {
> +    if (argc > 0 && grub_strcmp (card->name, args[0]) != 0)
> +      continue;
> +  }
> +

What is the goal of this loop?

> +  FOR_NET_CARDS (card)
> +  {
> +    struct grub_net_network_level_interface *iface;
> +
> +    if (argc > 0 && grub_strcmp (card->name, args[0]) != 0)
> +      continue;
> +
> +    iface = grub_net_ipv6_get_link_local (card, &card->default_address);
> +    if (!iface)
> +      {
> +	grub_dhcpv6_sessions_free ();
> +	return grub_errno;
> +      }
> +
> +    session = grub_zalloc (sizeof (*session));
> +    session->iface = iface;
> +    session->iaid = j;
> +    grub_dhcpv6_session_add (session);
> +    j++;
> +  }
> +
> +  for (interval = 200; interval < 10000; interval *= 2)

In general that does not follow RFC3315 required client behavior at all.
But we need at least something that can be improved later.

> +    {
> +      int done = 1;
> +
> +      FOR_DHCPV6_SESSIONS (session)
> +	{
> +	  struct grub_net_buff *nb;
> +	  struct grub_dhcpv6_option *opt;
> +	  struct grub_net_dhcpv6_packet *v6;
> +	  struct grub_DUID_LL *duid;
> +	  struct grub_dhcpv6_iana_option *ia_na;
> +	  grub_net_network_level_address_t multicast;
> +	  grub_net_link_level_address_t ll_multicast;
> +	  struct udphdr *udph;
> +
> +	  multicast.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> +	  multicast.ipv6[0] = grub_cpu_to_be64_compile_time (0xff02ULL << 48);
> +	  multicast.ipv6[1] = grub_cpu_to_be64_compile_time (0x10002ULL);
> +
> +	  err = grub_net_link_layer_resolve (session->iface,
> +		    &multicast, &ll_multicast);
> +	  if (err)
> +	    {
> +	      grub_dhcpv6_sessions_free ();
> +	      return err;
> +	    }
> +
> +	  nb = grub_netbuff_alloc (GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
> +
> +	  if (!nb)
> +	    {
> +	      grub_dhcpv6_sessions_free ();
> +	      return grub_errno;
> +	    }
> +
> +	  err = grub_netbuff_reserve (nb, GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
> +	  if (err)
> +	    {
> +	      grub_dhcpv6_sessions_free ();
> +	      grub_netbuff_free (nb);
> +	      return err;
> +	    }
> +
> +	  err = grub_netbuff_push (nb, sizeof (*opt) + sizeof (grub_uint16_t));
> +	  if (err)
> +	    {
> +	      grub_dhcpv6_sessions_free ();
> +	      grub_netbuff_free (nb);
> +	      return err;
> +	    }
> +
> +	  opt = (struct grub_dhcpv6_option *)nb->data;
> +	  opt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_ELAPSED_TIME);
> +	  opt->len = grub_cpu_to_be16_compile_time (sizeof (grub_uint16_t));

As far as I can tell, we remain 2 bytes aligned here. But we need to
change prototype to account for possible unaligned values on input, it
likely needs change through the whole function.

> +	  grub_set_unaligned16 (opt->data, 0);
> +
> +	  err = grub_netbuff_push (nb, sizeof (*opt) + sizeof (*duid));
> +	  if (err)
> +	    {
> +	      grub_dhcpv6_sessions_free ();
> +	      grub_netbuff_free (nb);
> +	      return err;
> +	    }
> +
> +	  opt = (struct grub_dhcpv6_option *)nb->data;
> +	  opt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_CLIENTID);
> +	  opt->len = grub_cpu_to_be16 (sizeof (*duid));
> +
> +	  duid = (struct grub_DUID_LL *) opt->data;
> +
> +	  duid->type = grub_cpu_to_be16_compile_time (3) ;
> +	  duid->hw_type = grub_cpu_to_be16_compile_time (1);
> +	  grub_memcpy (&duid->hwaddr, &session->iface->hwaddress.mac,
> +	      sizeof (session->iface->hwaddress.mac));
> +
> +	  err = grub_netbuff_push (nb, sizeof (*opt) + sizeof (*ia_na));
> +	  if (err)
> +	    {
> +	      grub_dhcpv6_sessions_free ();
> +	      grub_netbuff_free (nb);
> +	      return err;
> +	    }
> +
> +	  opt = (struct grub_dhcpv6_option *)nb->data;
> +	  opt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_IA_NA);
> +	  opt->len = grub_cpu_to_be16 (sizeof (*ia_na));
> +	  ia_na = (struct grub_dhcpv6_iana_option *)opt->data;
> +	  ia_na->iaid = grub_cpu_to_be32 (session->iaid);

Are you sure we are still aligned to 4 bytes here? You go from top to
bottom and some fields are not multiple of 4 bytes. As mentioned
earlier, may be ensure buffer is aligned in advance.

> +	  ia_na->t1 = 0;
> +	  ia_na->t2 = 0;
> +
> +	  err = grub_netbuff_push (nb, sizeof (*v6));
> +	  if (err)
> +	    {
> +	      grub_dhcpv6_sessions_free ();
> +	      grub_netbuff_free (nb);
> +	      return err;
> +	    }
> +
> +	  v6 = (struct grub_net_dhcpv6_packet *)nb->data;
> +	  v6->message_type = 1;

no magic constants, please.

> +	  v6->transaction_id = session->transaction_id;
> +
> +	  grub_netbuff_push (nb, sizeof (*udph));
> +
> +	  udph = (struct udphdr *) nb->data;
> +	  udph->src = grub_cpu_to_be16_compile_time (546);
> +	  udph->dst = grub_cpu_to_be16_compile_time (547);
> +	  udph->chksum = 0;
> +	  udph->len = grub_cpu_to_be16 (nb->tail - nb->data);
> +
> +	  udph->chksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_UDP,
> +			    &session->iface->address, &multicast);
> +
> +	  err = grub_net_send_ip_packet (session->iface, &multicast,
> +		    &ll_multicast, nb, GRUB_NET_IP_UDP);
> +	  done = 0;
> +	  grub_netbuff_free (nb);
> +
> +	  if (err)
> +	    {
> +	      grub_dhcpv6_sessions_free ();
> +	      return err;
> +	    }
> +	}
> +      if (!done)
> +	grub_net_poll_cards (interval, 0);
> +    }
> +
> +  FOR_DHCPV6_SESSIONS (session)
> +    {
> +      grub_error_push ();
> +      err = grub_error (GRUB_ERR_FILE_NOT_FOUND,
> +			N_("couldn't autoconfigure %s"),
> +			session->iface->card->name);
> +      grub_list_remove (GRUB_AS_LIST (session));
> +      grub_free (session);
> +      session = grub_dhcpv6_sessions;
> +    }
> +
> +  return err;
> +}
> +
> +static grub_command_t cmd_getdhcp, cmd_bootp, cmd_bootp6;
>  
>  void
>  grub_bootp_init (void)
> @@ -575,6 +1464,9 @@ grub_bootp_init (void)
>    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."));
> +  cmd_bootp6 = grub_register_command ("net_bootp6", grub_cmd_bootp6,
> +				     N_("[CARD]"),
> +				     N_("perform a DHCPv6 autoconfiguration"));
>  }
>  
>  void
> @@ -582,4 +1474,5 @@ grub_bootp_fini (void)
>  {
>    grub_unregister_command (cmd_getdhcp);
>    grub_unregister_command (cmd_bootp);
> +  grub_unregister_command (cmd_bootp6);
>  }
> diff --git a/grub-core/net/ip.c b/grub-core/net/ip.c
> index 8c56baa..07d6871 100644
> --- a/grub-core/net/ip.c
> +++ b/grub-core/net/ip.c
> @@ -238,6 +238,41 @@ handle_dgram (struct grub_net_buff *nb,
>    {
>      struct udphdr *udph;
>      udph = (struct udphdr *) nb->data;
> +
> +    if (proto == GRUB_NET_IP_UDP && udph->dst == grub_cpu_to_be16_compile_time (546))
> +      {
> +	if (udph->chksum)
> +	  {
> +	    grub_uint16_t chk, expected;
> +	    chk = udph->chksum;
> +	    udph->chksum = 0;
> +	    expected = grub_net_ip_transport_checksum (nb,
> +						       GRUB_NET_IP_UDP,
> +						       source,
> +						       dest);
> +	    if (expected != chk)
> +	      {
> +		grub_dprintf ("net", "Invalid UDP checksum. "
> +			      "Expected %x, got %x\n",
> +			      grub_be_to_cpu16 (expected),
> +			      grub_be_to_cpu16 (chk));
> +		grub_netbuff_free (nb);
> +		return GRUB_ERR_NONE;
> +	      }
> +	    udph->chksum = chk;
> +	  }
> +
> +	err = grub_netbuff_pull (nb, sizeof (*udph));
> +	if (err)
> +	  {
> +	    grub_netbuff_free (nb);
> +	    return err;
> +	  }
> +	grub_net_process_dhcp6 (nb, card);
> +	grub_netbuff_free (nb);
> +	return GRUB_ERR_NONE;
> +      }
> +
>      if (proto == GRUB_NET_IP_UDP && grub_be_to_cpu16 (udph->dst) == 68)
>        {
>  	const struct grub_net_bootp_packet *bootp;
> diff --git a/include/grub/net.h b/include/grub/net.h
> index 538baa3..71dc243 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -418,6 +418,13 @@ struct grub_net_bootp_packet
>    grub_uint8_t vendor[0];
>  } GRUB_PACKED;
>  
> +struct grub_net_dhcpv6_packet
> +{
> +  grub_uint32_t message_type:8;
> +  grub_uint32_t transaction_id:24;
> +  grub_uint8_t dhcp_options[0];
> +} GRUB_PACKED;
> +
>  #define	GRUB_NET_BOOTP_RFC1048_MAGIC_0	0x63
>  #define	GRUB_NET_BOOTP_RFC1048_MAGIC_1	0x82
>  #define	GRUB_NET_BOOTP_RFC1048_MAGIC_2	0x53
> @@ -444,6 +451,14 @@ grub_net_configure_by_dhcp_ack (const char *name,
>  				grub_size_t size,
>  				int is_def, char **device, char **path);
>  
> +struct grub_net_network_level_interface *
> +grub_net_configure_by_dhcpv6_reply (const char *name,
> +				    struct grub_net_card *card,
> +				    grub_net_interface_flags_t flags,
> +				    const struct grub_net_dhcpv6_packet *v6,
> +				    grub_size_t size,
> +				    int is_def, char **device, char **path);
> +
>  grub_err_t
>  grub_net_add_ipv4_local (struct grub_net_network_level_interface *inf,
>  			 int mask);
> @@ -452,6 +467,10 @@ void
>  grub_net_process_dhcp (struct grub_net_buff *nb,
>  		       struct grub_net_card *card);
>  
> +void
> +grub_net_process_dhcp6 (struct grub_net_buff *nb,
> +			struct grub_net_card *card);
> +
>  int
>  grub_net_hwaddr_cmp (const grub_net_link_level_address_t *a,
>  		     const grub_net_link_level_address_t *b);



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE
  2015-05-12  8:49 [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE Michael Chang
                   ` (2 preceding siblings ...)
  2015-05-12  8:49 ` [PATCH 3/3] Update document for net_bootp6 command Michael Chang
@ 2015-05-15  6:40 ` Andrei Borzenkov
  2015-05-15 14:15   ` Michael Chang
  3 siblings, 1 reply; 11+ messages in thread
From: Andrei Borzenkov @ 2015-05-15  6:40 UTC (permalink / raw)
  To: Michael Chang; +Cc: grub-devel

В Tue, 12 May 2015 16:49:47 +0800
Michael Chang <mchang@suse.com> пишет:

> This patch set tries to make support of configuring IPv6 network interface
> through the DHCPv6 protocol. A new command, net_bootp6, is therefore introduced
> to serve the purpose. You can think it as DHCPv6/IPv6 version of the existing
> net_bootp command.
> 

Note that spec actually suggests (requires?) that DHCPv6 transaction is
triggered by receiving Router Advertisement with M/O bits set.

> In addition to that, the UEFI IPv6 PXE support can be easily done by sharing
> the same routine with net_bootp6 to parse DHCPv6 Reply packet cached in the
> firmware.
> 

Looking in EFI spec I do not see it explaining what content this
dhcp_ack packet has for IPv6. I mean, DHCPv6 has neither Discover not
Acknowledge, and full DHCPv6 transaction requires *two* packets - one
for getting address and one for getting other options (DNS/boot
server). Let's see how it works in real world :)

> changes in v1:
> - Added upper boundary check in find_dhcpv6_option
> - Fix memory leak and freeing NULL pointer
> - Improved error message logging to not get lost
> - Use grub_cpu_to_be16_compile_time for endianess conversion when appropriate
> - Removed grub_dhcpv6_dns_servers structure and use 16 bytes blocks
> - Avoud magic numbers and use more descriptive sizeof when populating netbuff
> - Move include/grub/efi/api.h to UEFI IPv6 PXE support patch
> - Document the net_bootp6 command
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] Added net_bootp6 command
  2015-05-15  6:26   ` Andrei Borzenkov
@ 2015-05-15 13:57     ` Michael Chang
  2015-05-16  5:42       ` Andrei Borzenkov
  2015-05-19  8:42     ` Michael Chang
  1 sibling, 1 reply; 11+ messages in thread
From: Michael Chang @ 2015-05-15 13:57 UTC (permalink / raw)
  To: Andrei Borzenkov; +Cc: grub-devel

On Fri, May 15, 2015 at 09:26:06AM +0300, Andrei Borzenkov wrote:
> В Tue, 12 May 2015 16:49:48 +0800
> Michael Chang <mchang@suse.com> пишет:
> 
> > The net_bootp6 is used to configure the ipv6 network interface through the
> > DHCPv6 protocol Solict/Advertise/Request/Reply.
> > ---
> >  grub-core/net/bootp.c |  895 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  grub-core/net/ip.c    |   35 ++
> >  include/grub/net.h    |   19 +
> >  3 files changed, 948 insertions(+), 1 deletions(-)
> > 
> > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> > index 6136755..5c5eb6f 100644
> > --- a/grub-core/net/bootp.c
> > +++ b/grub-core/net/bootp.c
> > @@ -24,6 +24,8 @@
> >  #include <grub/net/netbuff.h>
> >  #include <grub/net/udp.h>
> >  #include <grub/datetime.h>
> > +#include <grub/time.h>
> > +#include <grub/list.h>
> >  
> >  static void
> >  parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
> > @@ -256,6 +258,646 @@ grub_net_configure_by_dhcp_ack (const char *name,
> >    return inter;
> >  }
> >  
> > +struct grub_dhcpv6_option {
> > +  grub_uint16_t code;
> > +  grub_uint16_t len;
> 
> Won't do; options in packet are unaligned (see below) so you have to
> walk byte buffer using get_unaligned and

I don't get it. Wouldn't the structure attribute GRUB_PACKED do the
trick for us?

> 
> grub_uint8_t code[2]
> grub_uint8_t len[2]
> 
> > +  grub_uint8_t data[0];
> > +} GRUB_PACKED;
> > +
> > +
> > +struct grub_dhcpv6_iana_option {
> > +  grub_uint32_t iaid;
> > +  grub_uint32_t t1;
> > +  grub_uint32_t t2;
> > +  grub_uint8_t data[0];
> > +} GRUB_PACKED;
> > +
> > +struct grub_dhcpv6_iaaddr_option {
> > +  grub_uint8_t addr[16];
> > +  grub_uint32_t preferred_lifetime;
> > +  grub_uint32_t valid_lifetime;
> > +  grub_uint8_t data[0];
> > +} GRUB_PACKED;
> > +
> > +struct grub_DUID_LL
> > +{
> > +  grub_uint16_t type;
> > +  grub_uint16_t hw_type;
> > +  grub_uint8_t hwaddr[6];
> > +} GRUB_PACKED;
> > +
> 
> Probably the same applies to all option definitions.

That would require quite a lot of rewritting basically. (So I'd like it
to be clear to me in the first).

Thanks,
Michael


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE
  2015-05-15  6:40 ` [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE Andrei Borzenkov
@ 2015-05-15 14:15   ` Michael Chang
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Chang @ 2015-05-15 14:15 UTC (permalink / raw)
  To: Andrei Borzenkov; +Cc: grub-devel

On Fri, May 15, 2015 at 09:40:26AM +0300, Andrei Borzenkov wrote:
> В Tue, 12 May 2015 16:49:47 +0800
> Michael Chang <mchang@suse.com> пишет:
> 
> > This patch set tries to make support of configuring IPv6 network interface
> > through the DHCPv6 protocol. A new command, net_bootp6, is therefore introduced
> > to serve the purpose. You can think it as DHCPv6/IPv6 version of the existing
> > net_bootp command.
> > 
> 
> Note that spec actually suggests (requires?) that DHCPv6 transaction is
> triggered by receiving Router Advertisement with M/O bits set.

I know that. But UEFI IPv6 PXE booting just works without it. We can
still add it in the future if it is really mandatory. 

> 
> > In addition to that, the UEFI IPv6 PXE support can be easily done by sharing
> > the same routine with net_bootp6 to parse DHCPv6 Reply packet cached in the
> > firmware.
> > 
> 
> Looking in EFI spec I do not see it explaining what content this
> dhcp_ack packet has for IPv6. I mean, DHCPv6 has neither Discover not
> Acknowledge, and full DHCPv6 transaction requires *two* packets - one
> for getting address and one for getting other options (DNS/boot
> server). Let's see how it works in real world :)

Yes, I also noticed the problem. But In real world, it has been tested
for a while, as that's the same with how elilo handles it. That is
treating dhcp_ack packet as a single DHCPv6 reply packet and parse the
options in it.

Thanks,
Michael

> 
> > changes in v1:
> > - Added upper boundary check in find_dhcpv6_option
> > - Fix memory leak and freeing NULL pointer
> > - Improved error message logging to not get lost
> > - Use grub_cpu_to_be16_compile_time for endianess conversion when appropriate
> > - Removed grub_dhcpv6_dns_servers structure and use 16 bytes blocks
> > - Avoud magic numbers and use more descriptive sizeof when populating netbuff
> > - Move include/grub/efi/api.h to UEFI IPv6 PXE support patch
> > - Document the net_bootp6 command
> > 
> > 
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] Added net_bootp6 command
  2015-05-15 13:57     ` Michael Chang
@ 2015-05-16  5:42       ` Andrei Borzenkov
  0 siblings, 0 replies; 11+ messages in thread
From: Andrei Borzenkov @ 2015-05-16  5:42 UTC (permalink / raw)
  To: Michael Chang; +Cc: grub-devel

В Fri, 15 May 2015 21:57:28 +0800
Michael Chang <mchang@suse.com> пишет:

> On Fri, May 15, 2015 at 09:26:06AM +0300, Andrei Borzenkov wrote:
> > В Tue, 12 May 2015 16:49:48 +0800
> > Michael Chang <mchang@suse.com> пишет:
> > 
> > > The net_bootp6 is used to configure the ipv6 network interface through the
> > > DHCPv6 protocol Solict/Advertise/Request/Reply.
> > > ---
> > >  grub-core/net/bootp.c |  895 ++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  grub-core/net/ip.c    |   35 ++
> > >  include/grub/net.h    |   19 +
> > >  3 files changed, 948 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> > > index 6136755..5c5eb6f 100644
> > > --- a/grub-core/net/bootp.c
> > > +++ b/grub-core/net/bootp.c
> > > @@ -24,6 +24,8 @@
> > >  #include <grub/net/netbuff.h>
> > >  #include <grub/net/udp.h>
> > >  #include <grub/datetime.h>
> > > +#include <grub/time.h>
> > > +#include <grub/list.h>
> > >  
> > >  static void
> > >  parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
> > > @@ -256,6 +258,646 @@ grub_net_configure_by_dhcp_ack (const char *name,
> > >    return inter;
> > >  }
> > >  
> > > +struct grub_dhcpv6_option {
> > > +  grub_uint16_t code;
> > > +  grub_uint16_t len;
> > 
> > Won't do; options in packet are unaligned (see below) so you have to
> > walk byte buffer using get_unaligned and
> 
> I don't get it. Wouldn't the structure attribute GRUB_PACKED do the
> trick for us?
> 


Yes, you are right. I apologize for confusion.

Sorry :(

> > 
> > grub_uint8_t code[2]
> > grub_uint8_t len[2]
> > 
> > > +  grub_uint8_t data[0];
> > > +} GRUB_PACKED;
> > > +
> > > +
> > > +struct grub_dhcpv6_iana_option {
> > > +  grub_uint32_t iaid;
> > > +  grub_uint32_t t1;
> > > +  grub_uint32_t t2;
> > > +  grub_uint8_t data[0];
> > > +} GRUB_PACKED;
> > > +
> > > +struct grub_dhcpv6_iaaddr_option {
> > > +  grub_uint8_t addr[16];
> > > +  grub_uint32_t preferred_lifetime;
> > > +  grub_uint32_t valid_lifetime;
> > > +  grub_uint8_t data[0];
> > > +} GRUB_PACKED;
> > > +
> > > +struct grub_DUID_LL
> > > +{
> > > +  grub_uint16_t type;
> > > +  grub_uint16_t hw_type;
> > > +  grub_uint8_t hwaddr[6];
> > > +} GRUB_PACKED;
> > > +
> > 
> > Probably the same applies to all option definitions.
> 
> That would require quite a lot of rewritting basically. (So I'd like it
> to be clear to me in the first).
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] Added net_bootp6 command
  2015-05-15  6:26   ` Andrei Borzenkov
  2015-05-15 13:57     ` Michael Chang
@ 2015-05-19  8:42     ` Michael Chang
  2015-05-30  7:25       ` Andrei Borzenkov
  1 sibling, 1 reply; 11+ messages in thread
From: Michael Chang @ 2015-05-19  8:42 UTC (permalink / raw)
  To: Andrei Borzenkov; +Cc: grub-devel

Hi Andrey,

Please see my inline comments. And if I have left anything un-answered
or not clear please let me know.

And thanks a lot to your time and effort for doing the review.

Thanks,
Michael

On Fri, May 15, 2015 at 09:26:06AM +0300, Andrei Borzenkov wrote:
> В Tue, 12 May 2015 16:49:48 +0800
> Michael Chang <mchang@suse.com> пишет:
> 
> > The net_bootp6 is used to configure the ipv6 network interface through the
> > DHCPv6 protocol Solict/Advertise/Request/Reply.
> > ---
> >  grub-core/net/bootp.c |  895 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  grub-core/net/ip.c    |   35 ++
> >  include/grub/net.h    |   19 +
> >  3 files changed, 948 insertions(+), 1 deletions(-)
> > 
> > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> > index 6136755..5c5eb6f 100644
> > --- a/grub-core/net/bootp.c
> > +++ b/grub-core/net/bootp.c
> > @@ -24,6 +24,8 @@
> >  #include <grub/net/netbuff.h>
> >  #include <grub/net/udp.h>
> >  #include <grub/datetime.h>
> > +#include <grub/time.h>
> > +#include <grub/list.h>
> >  
> >  static void
> >  parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
> > @@ -256,6 +258,646 @@ grub_net_configure_by_dhcp_ack (const char *name,
> >    return inter;
> >  }
> >  
> > +struct grub_dhcpv6_option {
> > +  grub_uint16_t code;
> > +  grub_uint16_t len;
> 
> Won't do; options in packet are unaligned (see below) so you have to
> walk byte buffer using get_unaligned and
> 
> grub_uint8_t code[2]
> grub_uint8_t len[2]
> 
> > +  grub_uint8_t data[0];
> > +} GRUB_PACKED;
> > +
> > +
> > +struct grub_dhcpv6_iana_option {
> > +  grub_uint32_t iaid;
> > +  grub_uint32_t t1;
> > +  grub_uint32_t t2;
> > +  grub_uint8_t data[0];
> > +} GRUB_PACKED;
> > +
> > +struct grub_dhcpv6_iaaddr_option {
> > +  grub_uint8_t addr[16];
> > +  grub_uint32_t preferred_lifetime;
> > +  grub_uint32_t valid_lifetime;
> > +  grub_uint8_t data[0];
> > +} GRUB_PACKED;
> > +
> > +struct grub_DUID_LL
> > +{
> > +  grub_uint16_t type;
> > +  grub_uint16_t hw_type;
> > +  grub_uint8_t hwaddr[6];
> > +} GRUB_PACKED;
> > +
> 
> Probably the same applies to all option definitions.
> 
> > +#define GRUB_DHCPv6_ADVERTISE 2
> > +#define GRUB_DHCPv6_REQUEST 3
> > +#define GRUB_DHCPv6_REPLY 7
> > +
> 
> SOLICIT is missing; please enum as well.

OK.

> 
> > +#define GRUB_DHCPv6_OPTION_CLIENTID 1
> > +#define GRUB_DHCPv6_OPTION_SERVERID 2
> > +#define GRUB_DHCPv6_OPTION_IA_NA 3
> > +#define GRUB_DHCPv6_OPTION_IAADDR 5
> > +#define GRUB_DHCPv6_OPTION_ORO 6
> > +#define GRUB_DHCPv6_OPTION_ELAPSED_TIME 8
> > +#define GRUB_DHCPv6_OPTION_DNS_SERVERS 23
> > +#define GRUB_DHCPv6_OPTION_BOOTFILE_URL 59
> > +
> 
> enum please. These can also be anonymous, no need to invent fancy
> names.

I don't understand. Do you want me to use enum type here rather than
macro ? Where are the fancy names invented here ? (All of the listed
maros have it's name taken from spec, and the prefix according to your
previous reviews).

> 
> > +/* The default netbuff size for sending DHCPv6 packets which should be
> > +   large enough to hold the information */
> > +#define GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE 512
> > +
> > +struct grub_dhcpv6_session
> > +{
> > +  struct grub_dhcpv6_session *next;
> > +  struct grub_dhcpv6_session **prev;
> > +  grub_uint32_t iaid;
> > +  grub_uint32_t transaction_id:24;
> > +  grub_uint64_t start_time;
> > +  struct grub_net_network_level_interface *iface;
> > +};
> > +
> > +static struct grub_dhcpv6_session *grub_dhcpv6_sessions = NULL;
> > +#define FOR_DHCPV6_SESSIONS(var) FOR_LIST_ELEMENTS (var, grub_dhcpv6_sessions)
> > +
> > +static void
> > +grub_dhcpv6_session_add (struct grub_dhcpv6_session *session)
> > +{
> > +  struct grub_datetime date;
> > +  grub_err_t err;
> > +  grub_int32_t t = 0;
> > +
> > +  err = grub_get_datetime (&date);
> > +  if (err || !grub_datetime2unixtime (&date, &t))
> > +    {
> > +      grub_errno = GRUB_ERR_NONE;
> > +      t = 0;
> > +    }
> > +
> > +  session->transaction_id = t;
> > +  session->start_time = grub_get_time_ms ();
> > +  grub_list_push (GRUB_AS_LIST_P (&grub_dhcpv6_sessions), GRUB_AS_LIST (session));
> > +
> > +  return;
> 
> return is redundant

OK.

> 
> > +}
> > +
> > +static void
> > +grub_dhcpv6_sessions_free (void)
> > +{
> > +  struct grub_dhcpv6_session *session;
> > +
> > +  FOR_DHCPV6_SESSIONS (session)
> > +    {
> > +      grub_list_remove (GRUB_AS_LIST (session));
> > +      grub_free (session);
> > +      session = grub_dhcpv6_sessions;
> > +    }
> > +
> > +  return;
> 
> and here too

OK.

> 
> > +}
> > +
> > +static const char *
> > +get_dhcpv6_option_name (grub_uint16_t option)
> > +{
> > +  switch (option)
> > +    {
> > +    case GRUB_DHCPv6_OPTION_BOOTFILE_URL:
> > +      return "BOOTFILE URL";
> > +    case GRUB_DHCPv6_OPTION_DNS_SERVERS:
> > +      return "DNS SERVERS";
> > +    case GRUB_DHCPv6_OPTION_IA_NA:
> > +      return "IA NA";
> > +    case GRUB_DHCPv6_OPTION_IAADDR:
> > +      return "IAADDR";
> > +    case GRUB_DHCPv6_OPTION_CLIENTID:
> > +      return "CLIENTID";
> > +    case GRUB_DHCPv6_OPTION_SERVERID:
> > +      return "SERVERID";
> > +    case GRUB_DHCPv6_OPTION_ORO:
> > +      return "ORO";
> > +    case GRUB_DHCPv6_OPTION_ELAPSED_TIME:
> > +      return "ELAPSED TIME";
> > +    default:
> > +      return "UNKNOWN";
> > +    }
> > +}
> > +
> > +static const struct grub_dhcpv6_option*
> > +find_dhcpv6_option (const struct grub_dhcpv6_option *popt, grub_size_t size, grub_uint16_t option)
> > +{
> > +  while (size > 0)
> > +    {
> > +      grub_uint16_t code, len;
> > +
> 
> if (size < sizeof(*popt))
>   grub_error

Nice catch. Thanks.

> 
> > +      code = grub_be_to_cpu16 (popt->code);
> > +      len = grub_be_to_cpu16 (popt->len) + sizeof (*popt);
> > +
> 
> According to rfc3315 - Options are byte-aligned but are not aligned in
> any other way such as on 2 or 4 byte boundaries; it should be
> grub_get_unaligned.
> 
> To avoid warnings from static checkers (possible overflow) 
> 
> size -= sizeof(*popt)
> len = grub_be_to_cpu...

OK.

> 
> > +      if (size < len)
> > +	{
> > +	  grub_error (GRUB_ERR_OUT_OF_RANGE, N_("DHCPv6 option size overflow detected"));
> > +	  return NULL;
> > +	}
> > +
> > +      if (option == code)
> > +	return popt;
> > +
> > +      if (code == 0)
> > +	break;
> > +      else
> > +	{
> > +	  size -= len;
> > +	  popt = (const struct grub_dhcpv6_option *)((grub_uint8_t *)popt + len);
> 
> assuming changes above - + sizeof (*popt)

OK.
> 
> > +	}
> > +    }
> > +
> > +  grub_error (GRUB_ERR_IO, N_("DHCPv6 Option (%u):%s not found"), option, get_dhcpv6_option_name(option));
> > +  return NULL;
> > +}
> > +
> > +static const struct grub_dhcpv6_option*
> > +find_dhcpv6_option_in_packet (const struct grub_net_dhcpv6_packet *packet,
> > +    grub_size_t size, grub_uint16_t option)
> > +{
> > +  if (!size || !packet)
> > +    {
> > +      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("null or zero sized DHCPv6 packet buffer"));
> > +      return NULL;
> > +    }
> > +
> 
> Is it really possible? We come here from processing real packet or
> from EFI with fixed buffer. Just curious in case I miss something
> obvious.

In general, a function should check it's input before processing them,
that's why these checks were added.

With the understanding it's in static scope with a few known callers,
probably these check are not doing anything so that I'm fine to remove
them.

> 
> > +  if (size <= sizeof (*packet))
> > +    {
> > +      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("DHCPv6 packet size too small"));
> > +      return NULL;
> > +    }
> > +
> 
> You need to check size in grub_net_process_dhcp6() anyway. When you come
> from EFI you have fixed buffer size and packet exists so there is single
> entry point where size has to be verified. No need to do it every time
> here.

The reason is pretty much the same with above, if you don't like
redundant checks I can remove them.

> 
> > +  return find_dhcpv6_option ((const struct grub_dhcpv6_option *)packet->dhcp_options,
> > +	  size - sizeof (*packet), option);
> > +}
> > +
> > +static const grub_uint8_t*
> > +find_dhcpv6_address (const struct grub_net_dhcpv6_packet *packet, grub_size_t size)
> > +{
> > +  const struct grub_dhcpv6_option* popt;
> > +  const struct grub_dhcpv6_iana_option *ia_na;
> > +  const struct grub_dhcpv6_iaaddr_option *iaaddr;
> > +  grub_uint16_t ia_na_len;
> > +
> > +  popt = find_dhcpv6_option_in_packet (packet, size, GRUB_DHCPv6_OPTION_IA_NA);
> > +
> > +  if (!popt)
> > +    return NULL;
> > +
> > +  ia_na = (const struct grub_dhcpv6_iana_option *) popt->data;
> > +  ia_na_len = grub_be_to_cpu16 (popt->len);
> 
> grub_get_unaligned. It makes sense to add helper for option length,
> it's going to be used often.
> 
> > +
> > +  popt = find_dhcpv6_option ((const struct grub_dhcpv6_option *) ia_na->data,
> > +      ia_na_len - sizeof (*ia_na), GRUB_DHCPv6_OPTION_IAADDR);
> > +
> > +  if (!popt)
> > +    return NULL;
> > +
> > +  iaaddr = (const struct grub_dhcpv6_iaaddr_option *)popt->data;
> > +  return iaaddr->addr;
> > +}
> > +
> > +static void
> > +get_dhcpv6_dns_address (const struct grub_net_dhcpv6_packet *packet, grub_size_t size,
> > +	grub_net_network_level_address_t **addr, grub_uint16_t *naddr)
> > +{
> > +  const struct grub_dhcpv6_option* popt;
> > +  grub_uint16_t len, ln;
> > +  const grub_uint8_t *pa;
> > +  grub_net_network_level_address_t *la;
> > +
> > +  if (!addr || !naddr)
> > +    {
> > +      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("bad argument for get_dhcpv6_dns_address"));
> > +      return;
> > +    }
> > +
> 
> it is called exactly once and grub_errno is immediately reset to
> GRUB_ERR_NONE. So what's the point of returning error at all?

Because  a function wants to report error for bad inputs, while it's
caller wants to suppress the error message because dns information could
be missing in packets and is allowed (not treated as error).

> 
> > +  *addr = NULL;
> > +  *naddr = 0;
> > +
> > +  popt = find_dhcpv6_option_in_packet (packet, size, GRUB_DHCPv6_OPTION_DNS_SERVERS);
> > +  if (!popt)
> > +    return;
> > +
> > +  len = grub_be_to_cpu16 (popt->len);
> 
> unaligned 
> 
> > +  if (len == 0 || len & 0xf)
> > +    {
> > +      grub_error (GRUB_ERR_IO, N_("invalid dns address length"));
> > +      return;
> > +    }
> > +
> 
> same question about grub_error. May be grub_debug?

No grub_debug, maybe grub_dprintf ? Maybe we should keep it and use
specific error number for option not found ? See below for details.


> 
> > +  *naddr = ln = len >> 4;
> > +  *addr = la =  grub_zalloc (sizeof (grub_net_network_level_address_t) * ln);
> 
> You are overwriting it immediately, no need to zalloc

OK.

> 
> > +
> > +  if (!la)
> > +    {
> > +      *addr = NULL;
> We just checked it is NULL above
OK.

> 
> > +      *naddr = 0;
> 
> Just move assignment after NULL check
OK.

> 
> > +      return;
> > +    }
> > +
> > +  for (pa = popt->data; ln > 0; pa += 0x10, la++, ln--)
> > +    {
> > +      la->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> > +      la->ipv6[0] = grub_get_unaligned64 (pa);
> > +      la->ipv6[1] = grub_get_unaligned64 (pa + 8);
> > +      la->option = DNS_OPTION_PREFER_IPV6;
> > +    }
> > +
> > +  return;
> 
> redundant return

OK.

> 
> > +}
> > +
> > +static void
> > +find_dhcpv6_bootfile_url (const struct grub_net_dhcpv6_packet *packet, grub_size_t size,
> > +	char **proto, char **server_ip, char **boot_file)
> > +{
> > +  char *bootfile_url;
> > +  const struct grub_dhcpv6_option* opt_url;
> > +  char *ip_start, *ip_end;
> > +  char *path;
> > +  grub_size_t ip_len;
> > +  grub_uint16_t len;
> > +  const char *protos[] = {"tftp://", "http://", NULL};
> > +  const char *pr;
> > +  int i;
> > +
> > +  if (proto)
> > +    *proto = NULL;
> > +
> > +  if (server_ip)
> > +    *server_ip = NULL;
> > +
> > +  if (boot_file)
> > +    *boot_file = NULL;
> > +
> > +  opt_url = find_dhcpv6_option_in_packet (packet, size, GRUB_DHCPv6_OPTION_BOOTFILE_URL);
> > +
> > +  if (!opt_url)
> > +    return;
> > +
> > +  len = grub_be_to_cpu16 (opt_url->len);
> > +
> 
> get_unaligned
> 
> > +  bootfile_url = grub_malloc (len + 1);
> > +
> > +  if (!bootfile_url)
> > +    return;
> > +
> > +  grub_memcpy (bootfile_url, opt_url->data, len);
> > +  bootfile_url[len]   = '\0';
> > +
> > +  for (i = 0; (pr = *(protos + i)); ++i)
> for (pr = protos; *pr; pr++)

OK.

> 
> > +      if (grub_strncmp (bootfile_url, pr, grub_strlen(pr)) == 0)
> > +	break;
> > +
> > +  if (!pr)
> > +    {
> > +      grub_error (GRUB_ERR_IO,
> > +	N_("unsupported protocol, only tftp and http are supported"));
> > +      goto cleanup;
> > +    }
> > +
> > +  ip_start = ip_end = NULL;
> > +  ip_start = bootfile_url + grub_strlen(pr);
> > +
> > +  /* Follow elilo and edk2 that check for starting and ending delimiter '[..]'
> > +     in which IPv6 server address is enclosed. */
> > +  if (*ip_start != '[')
> > +    ip_start = NULL;
> > +  else
> > +    ip_end = grub_strchr (++ip_start, ']');
> > +
> > +  if (!ip_start || !ip_end)
> > +    {
> > +      grub_error (GRUB_ERR_IO, N_("IPv6-address not in square brackets"));
> > +      goto cleanup;
> > +    }
> > +
> 
> RFC5970 says "Clients that have DNS implementations SHOULD support the
> use of domain names in the URL" and in general string can also be valid
> IPv4 address, nothing restricts it to IPv6. 

But the descriptions preceding it did explicitly say IPv6:

" If the host in the URL is expressed using an IPv6 address rather than
a domain name, the address in the URL then MUST be enclosed in "[" and
"]" characters, conforming to [RFC3986]. "

> So I do not think you
> should return error here, just return full string. I wish grub supported
> IPv6 literals so we could just skip this check entirely.

OK. I'll return the string by assuming it's domain name.

> 
> > +  ip_len = ip_end - ip_start;
> > +
> > +  if (proto)
> > +    {
> > +      grub_size_t proto_len  = grub_strlen (pr) - 3;
> > +
> > +      *proto = grub_malloc (proto_len + 1);
> > +      if (!*proto)
> > +	goto cleanup;
> > +
> > +      grub_memcpy (*proto, pr, proto_len);
> > +      *(*proto + proto_len)  = '\0';
> > +    }
> > +
> > +  if (server_ip)
> > +    {
> > +      *server_ip = grub_malloc (ip_len + 1);
> > +
> > +      if (!*server_ip)
> > +	goto cleanup;
> > +
> > +      grub_memcpy (*server_ip, ip_start, ip_len);
> > +      *(*server_ip + ip_len) = '\0';
> > +    }
> > +
> > +  path = ip_end + 1;
> > +
> > +  if (boot_file)
> > +    {
> > +      *boot_file = grub_strdup (path);
> > +
> > +      if (!*boot_file)
> > +	goto cleanup;
> > +    }
> > +
> > +cleanup:
> > +
> > +  if (bootfile_url)
> > +    grub_free (bootfile_url);
> > +
> > +  if (grub_errno)
> > +    {
> > +      if (proto && *proto)
> > +	{
> > +	  grub_free (*proto);
> > +	  *proto = NULL;
> > +	}
> > +
> > +      if (server_ip && *server_ip)
> > +	{
> > +	  grub_free (*server_ip);
> > +	  *server_ip = NULL;
> > +	}
> > +
> > +      if (boot_file && *boot_file)
> > +	{
> > +	  grub_free (*boot_file);
> > +	  *boot_file = NULL;
> > +	}
> > +    }
> > +
> > +  return;
> 
> redundant return

OK.

> 
> > +}
> > +
> > +
> > +static grub_err_t
> > +grub_net_configure_by_dhcpv6_adv (const struct grub_net_dhcpv6_packet *v6_adv,
> > +	grub_size_t size,
> > +	struct grub_dhcpv6_session *session)
> > +{
> > +  struct grub_net_buff *nb;
> > +  const struct grub_dhcpv6_option *opt_client, *opt_server, *opt_iana;
> > +  struct grub_dhcpv6_option *popt;
> > +  struct grub_net_dhcpv6_packet *v6;
> > +  struct udphdr *udph;
> > +  grub_net_network_level_address_t multicast;
> > +  grub_net_link_level_address_t ll_multicast;
> > +  struct grub_net_network_level_interface *inf;
> > +  grub_uint16_t len;
> > +  grub_uint64_t elapsed;
> > +  grub_err_t err = GRUB_ERR_NONE;
> > +
> > +  opt_client = find_dhcpv6_option_in_packet (v6_adv, size, GRUB_DHCPv6_OPTION_CLIENTID);
> > +  if (grub_errno)
> > +    grub_error_push ();
> > +
> 
> Should not we check that is it actually one of our sessions? From
> RFC3315 validation
> 
>    -  the contents of the Client Identifier option does not match the
>       client's DUID.
> 
>    -  the "transaction-id" field value does not match the value the
>       client used in its Solicit message.
> 
> transaction ID is checked earlier, we also need to check client ID
> somewhere

Yes, you're right, that check is necessary to have.

> 
> > +  opt_server = find_dhcpv6_option_in_packet (v6_adv, size, GRUB_DHCPv6_OPTION_SERVERID);
> > +  if (grub_errno)
> > +    grub_error_push ();
> > +
> > +  opt_iana = find_dhcpv6_option_in_packet (v6_adv, size, GRUB_DHCPv6_OPTION_IA_NA);
> > +  if (grub_errno)
> > +    grub_error_push ();
> > +
> > +  grub_error_pop ();
> > +  if (grub_errno)
> > +    return grub_errno;
> > +
> 
> Do we really need this complication? Why not just return on the first
> missing option?

The intention is to report all missing options once so that admin can
understand better what's going wrong. It's not a strict thing to follow
so that I'm fine with returing first missing option.

> 
> > +  inf = session->iface;
> > +
> > +  multicast.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> > +  multicast.ipv6[0] = grub_cpu_to_be64_compile_time (0xff02ULL << 48);
> > +  multicast.ipv6[1] = grub_cpu_to_be64_compile_time (0x10002ULL);
> > +
> > +  err = grub_net_link_layer_resolve (inf, &multicast, &ll_multicast);
> > +  if (err)
> > +    return err;
> > +
> > +  nb = grub_netbuff_alloc (GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
> > +
> > +  if (!nb)
> > +    return grub_errno;
> > +
> > +  err = grub_netbuff_reserve (nb, GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
> > +  if (err)
> > +    {
> > +      grub_netbuff_free (nb);
> > +      return err;
> > +    }
> > +
> > +  len = grub_cpu_to_be16(opt_client->len);
> get_unaligned, grub_be_to_cpu16
> 
> > +  err = grub_netbuff_push (nb, len + sizeof (*opt_client));
> > +  if (err)
> > +    {
> > +      grub_netbuff_free (nb);
> > +      return err;
> > +    }
> > +  grub_memcpy (nb->data, opt_client, len + sizeof (*opt_client));
> > +
> > +  len = grub_cpu_to_be16(opt_server->len);
> 
> ditto
> 
> > +  err = grub_netbuff_push (nb, len + sizeof (*opt_server));
> > +  if (err)
> > +    {
> > +      grub_netbuff_free (nb);
> > +      return err;
> > +    }
> > +  grub_memcpy (nb->data, opt_server, len + sizeof (*opt_server));
> > +
> > +  len = grub_cpu_to_be16(opt_iana->len);
> 
> ditto
> 
> > +  err = grub_netbuff_push (nb, len + sizeof (*opt_iana));
> > +  if (err)
> > +    {
> > +      grub_netbuff_free (nb);
> > +      return err;
> > +    }
> > +  grub_memcpy (nb->data, opt_iana, len + sizeof (*opt_iana));
> > +
> > +  err = grub_netbuff_push (nb, sizeof (*popt) + 2 * sizeof (grub_uint16_t));
> > +  if (err)
> > +    {
> > +      grub_netbuff_free (nb);
> > +      return err;
> > +    }
> > +
> > +  popt = (struct grub_dhcpv6_option*) nb->data;
> > +  popt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_ORO);
> 
> From here it starts to be interesting. At least server ID option is out
> of our control so everything after it may be unaligned.
> 
> > +  popt->len = grub_cpu_to_be16_compile_time (2 * sizeof (grub_uint16_t));
> 
> unaligned?
> 
> > +  grub_set_unaligned16 (popt->data, grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_BOOTFILE_URL));
> > +  grub_set_unaligned16 (popt->data + 2, grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_DNS_SERVERS));
> > +
> > +  err = grub_netbuff_push (nb, sizeof (*popt) + sizeof (grub_uint16_t));
> > +  if (err)
> > +    {
> > +      grub_netbuff_free (nb);
> > +      return err;
> > +    }
> > +  popt = (struct grub_dhcpv6_option*) nb->data;
> > +  popt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_ELAPSED_TIME);
> > +  popt->len = grub_cpu_to_be16_compile_time (sizeof (grub_uint16_t));
> > +
> 
> unaligned?
> 
> > +  /* the time is expressed in hundredths of a second */
> > +  elapsed = grub_divmod64 (grub_get_time_ms () - session->start_time, 10, 0);
> > +
> > +  if (elapsed > 0xffff)
> > +    elapsed = 0xffff;
> > +
> > +  grub_set_unaligned16 (popt->data,  grub_cpu_to_be16 ((grub_uint16_t)elapsed));
> > +
> > +  err = grub_netbuff_push (nb, sizeof (*v6));
> > +  if (err)
> > +    {
> > +      grub_netbuff_free (nb);
> > +      return err;
> > +    }
> > +
> > +  v6 = (struct grub_net_dhcpv6_packet *) nb->data;
> > +  v6->message_type = GRUB_DHCPv6_REQUEST;
> > +  v6->transaction_id = v6_adv->transaction_id;
> Not sure how compiler handles alignment in this case.
> 
> > +
> > +  err = grub_netbuff_push (nb, sizeof (*udph));
> > +  if (err)
> > +    {
> > +      grub_netbuff_free (nb);
> > +      return err;
> > +    }
> > +
> > +  udph = (struct udphdr *) nb->data;
> > +  udph->src = grub_cpu_to_be16_compile_time (546);
> > +  udph->dst = grub_cpu_to_be16_compile_time (547);
> > +  udph->chksum = 0;
> > +  udph->len = grub_cpu_to_be16 (nb->tail - nb->data);
> > +
> 
> And even here header may be unaligned after starting from server ID
> option. I suggest to do it differently - compute total length, reserve
> aligned space and make server ID last to make sure everything before it
> is properly aligned. And add comment explaining it :)
> 
> > +  udph->chksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_UDP,
> > +						 &inf->address,
> > +						 &multicast);
> > +  err = grub_net_send_ip_packet (inf, &multicast, &ll_multicast, nb,
> > +				 GRUB_NET_IP_UDP);
> > +
> > +  grub_netbuff_free (nb);
> > +
> > +  return err;
> > +}
> > +
> > +
> > +struct grub_net_network_level_interface *
> > +grub_net_configure_by_dhcpv6_reply (const char *name,
> > +	struct grub_net_card *card,
> > +	grub_net_interface_flags_t flags,
> > +	const struct grub_net_dhcpv6_packet *v6,
> > +	grub_size_t size,
> > +	int is_def,
> > +	char **device, char **path)
> > +{
> > +  grub_net_network_level_address_t addr;
> > +  grub_net_network_level_netaddress_t netaddr;
> > +  struct grub_net_network_level_interface *inf;
> > +  const grub_uint8_t *your_ip;
> > +  char *proto;
> > +  char *server_ip;
> > +  char *boot_file;
> > +  grub_net_network_level_address_t *dns;
> > +  grub_uint16_t num_dns;
> > +
> > +  if (device)
> > +    *device = NULL;
> > +
> > +  if (path)
> > +    *path = NULL;
> > +
> > +  if (v6->message_type != GRUB_DHCPv6_REPLY)
> > +    {
> > +      grub_error (GRUB_ERR_IO, N_("DHCPv6 info not found"));
> > +      return NULL;
> > +    }
> > +
> > +  your_ip = find_dhcpv6_address(v6, size);
> > +
> > +  if (!your_ip)
> > +    return NULL;
> > +
> > +  addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> > +  addr.ipv6[0] = grub_get_unaligned64 (your_ip);
> > +  addr.ipv6[1] = grub_get_unaligned64 (your_ip + 8);
> > +  inf = grub_net_add_addr (name, card, &addr, &card->default_address, flags);
> > +
> 
> NULL check. It is not clear if it makes sense to continue if we failed
> to actually set address at this point.

Ok.

> 
> > +  netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> > +  netaddr.ipv6.base[0] = grub_get_unaligned64 (your_ip);
> > +  netaddr.ipv6.base[1] = 0;
> > +  netaddr.ipv6.masksize = 64;
> > +  grub_net_add_route (name, netaddr, inf);
> 
> This will crash in net_ls_routes later if inf was NULL. Yes, I know
> that it is used this way currently, but that needs fixing in other
> places as well.\

Ok.

> 
> > +
> > +  get_dhcpv6_dns_address (v6, size, &dns, &num_dns);
> > +
> > +  if (grub_errno)
> > +    grub_errno = GRUB_ERR_NONE;
> > +
> 
> This ignores legitimate errors like out of memory. As mentioned above,
> does it need to set any grub_errno on its own? If not, errors should
> not be ignored.

Because missing dns option is totally fine so that I don't want to treat
it as error. We can use a specific error number for missing options and
report other errors, but I can't find a suitable one for it (in
include/grub/err.h).

> 
> > +  if (dns && num_dns)
> > +    {
> > +      int i;
> > +
> > +      for (i = 0; i < num_dns; ++i)
> > +	grub_net_add_dns_server (dns + i);
> > +
> > +      grub_free (dns);
> > +    }
> > +
> > +  find_dhcpv6_bootfile_url (v6, size, &proto, &server_ip, &boot_file);
> > +
> > +  if (grub_errno)
> > +    grub_print_error ();
> > +
> > +  if (boot_file)
> > +    grub_env_set_net_property (name, "boot_file", boot_file,
> > +			  grub_strlen (boot_file));
> > +
> > +  if (is_def && server_ip)
> > +    {
> > +      grub_net_default_server = grub_strdup (server_ip);
> 
> NULL check

OK.

> 
> > +      grub_env_set ("net_default_interface", name);
> > +      grub_env_export ("net_default_interface");
> > +    }
> > +
> > +  if (device && server_ip && proto)
> > +    {
> > +      *device = grub_xasprintf ("%s,%s", proto, server_ip);
> > +      if (!*device)
> > +	grub_print_error ();
> > +    }
> > +
> > +  if (path && boot_file)
> > +    {
> > +      *path = grub_strdup (boot_file);
> > +      if (*path)
> > +	{
> > +	  char *slash;
> > +	  slash = grub_strrchr (*path, '/');
> > +	  if (slash)
> > +	    *slash = 0;
> > +	  else
> > +	    **path = 0;
> > +	}
> > +      else
> > +	grub_print_error ();
> > +    }
> > +
> > +  if (proto)
> > +    grub_free (proto);
> > +
> > +  if (server_ip)
> > +    grub_free (server_ip);
> > +
> > +  if (boot_file)
> > +    grub_free (boot_file);
> > +
> > +  return inf;
> > +}
> > +
> >  void
> >  grub_net_process_dhcp (struct grub_net_buff *nb,
> >  		       struct grub_net_card *card)
> > @@ -288,6 +930,70 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
> >      }
> >  }
> >  
> > +void
> > +grub_net_process_dhcp6 (struct grub_net_buff *nb,
> > +	struct grub_net_card *card __attribute__ ((unused)))
> > +{
> > +  const struct grub_net_dhcpv6_packet *v6;
> > +  struct grub_dhcpv6_session *session;
> > +  const struct grub_dhcpv6_option *opt_iana;
> > +  const struct grub_dhcpv6_iana_option *ia_na;
> > +  grub_size_t size;
> > +
> > +  v6 = (const struct grub_net_dhcpv6_packet *) nb->data;
> > +
> > +  size = nb->tail - nb->data;
> > +
> 
> Size check here looks like the most natural place.

OK.

> 
> > +  opt_iana = find_dhcpv6_option_in_packet (v6, size, GRUB_DHCPv6_OPTION_IA_NA);
> > +  if (!opt_iana)
> > +    {
> > +      grub_print_error ();
> > +      return;
> > +    }
> > +
> > +  ia_na = (const struct grub_dhcpv6_iana_option *)opt_iana->data;
> > +  FOR_DHCPV6_SESSIONS (session)
> > +    {
> > +      if (session->transaction_id == v6->transaction_id
> > +	  && session->iaid == grub_cpu_to_be32 (ia_na->iaid))
> 
> get_unaligned
> 
> > +	break;
> > +    }
> > +
> > +  if (!session)
> > +    return;
> > +
> > +  if (v6->message_type == GRUB_DHCPv6_ADVERTISE)
> > +    {
> > +      grub_net_configure_by_dhcpv6_adv (v6, size, session);
> > +    }
> > +  else if (v6->message_type == GRUB_DHCPv6_REPLY)
> > +    {
> > +      char *name;
> > +      struct grub_net_network_level_interface *inf;
> > +
> > +      inf = session->iface;
> > +      name = grub_xasprintf ("%s:dhcp6", inf->card->name);
> > +      if (!name)
> > +	return;
> > +
> > +      grub_net_configure_by_dhcpv6_reply (name, inf->card,
> > +	  0, v6, size, 0, 0, 0);
> > +
> > +      if (!grub_errno)
> > +	{
> > +	  grub_list_remove (GRUB_AS_LIST (session));
> > +	  grub_free (session);
> > +	}
> > +
> > +      grub_free (name);
> > +    }
> > +
> > +  if (grub_errno)
> > +    grub_print_error ();
> > +
> > +  return;
> > +}
> > +
> >  static char
> >  hexdigit (grub_uint8_t val)
> >  {
> > @@ -564,7 +1270,190 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
> >    return err;
> >  }
> >  
> > -static grub_command_t cmd_getdhcp, cmd_bootp;
> > +
> > +static grub_err_t
> > +grub_cmd_bootp6 (struct grub_command *cmd __attribute__ ((unused)),
> > +	int argc, char **args)
> > +{
> > +  struct grub_net_card *card;
> > +  unsigned j = 0;
> > +  int interval;
> > +  grub_err_t err;
> > +  struct grub_dhcpv6_session *session;
> > +
> > +  err = GRUB_ERR_NONE;
> > +
> > +  FOR_NET_CARDS (card)
> > +  {
> > +    if (argc > 0 && grub_strcmp (card->name, args[0]) != 0)
> > +      continue;
> > +  }
> > +
> 
> What is the goal of this loop?

It's dead code somehow not get cleaned from previous version. Sorry.

> 
> > +  FOR_NET_CARDS (card)
> > +  {
> > +    struct grub_net_network_level_interface *iface;
> > +
> > +    if (argc > 0 && grub_strcmp (card->name, args[0]) != 0)
> > +      continue;
> > +
> > +    iface = grub_net_ipv6_get_link_local (card, &card->default_address);
> > +    if (!iface)
> > +      {
> > +	grub_dhcpv6_sessions_free ();
> > +	return grub_errno;
> > +      }
> > +
> > +    session = grub_zalloc (sizeof (*session));
> > +    session->iface = iface;
> > +    session->iaid = j;
> > +    grub_dhcpv6_session_add (session);
> > +    j++;
> > +  }
> > +
> > +  for (interval = 200; interval < 10000; interval *= 2)
> 
> In general that does not follow RFC3315 required client behavior at all.
> But we need at least something that can be improved later.

OK.

> 
> > +    {
> > +      int done = 1;
> > +
> > +      FOR_DHCPV6_SESSIONS (session)
> > +	{
> > +	  struct grub_net_buff *nb;
> > +	  struct grub_dhcpv6_option *opt;
> > +	  struct grub_net_dhcpv6_packet *v6;
> > +	  struct grub_DUID_LL *duid;
> > +	  struct grub_dhcpv6_iana_option *ia_na;
> > +	  grub_net_network_level_address_t multicast;
> > +	  grub_net_link_level_address_t ll_multicast;
> > +	  struct udphdr *udph;
> > +
> > +	  multicast.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> > +	  multicast.ipv6[0] = grub_cpu_to_be64_compile_time (0xff02ULL << 48);
> > +	  multicast.ipv6[1] = grub_cpu_to_be64_compile_time (0x10002ULL);
> > +
> > +	  err = grub_net_link_layer_resolve (session->iface,
> > +		    &multicast, &ll_multicast);
> > +	  if (err)
> > +	    {
> > +	      grub_dhcpv6_sessions_free ();
> > +	      return err;
> > +	    }
> > +
> > +	  nb = grub_netbuff_alloc (GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
> > +
> > +	  if (!nb)
> > +	    {
> > +	      grub_dhcpv6_sessions_free ();
> > +	      return grub_errno;
> > +	    }
> > +
> > +	  err = grub_netbuff_reserve (nb, GRUB_DHCPv6_DEFAULT_NETBUFF_ALLOC_SIZE);
> > +	  if (err)
> > +	    {
> > +	      grub_dhcpv6_sessions_free ();
> > +	      grub_netbuff_free (nb);
> > +	      return err;
> > +	    }
> > +
> > +	  err = grub_netbuff_push (nb, sizeof (*opt) + sizeof (grub_uint16_t));
> > +	  if (err)
> > +	    {
> > +	      grub_dhcpv6_sessions_free ();
> > +	      grub_netbuff_free (nb);
> > +	      return err;
> > +	    }
> > +
> > +	  opt = (struct grub_dhcpv6_option *)nb->data;
> > +	  opt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_ELAPSED_TIME);
> > +	  opt->len = grub_cpu_to_be16_compile_time (sizeof (grub_uint16_t));
> 
> As far as I can tell, we remain 2 bytes aligned here. But we need to
> change prototype to account for possible unaligned values on input, it
> likely needs change through the whole function.
> 
> > +	  grub_set_unaligned16 (opt->data, 0);
> > +
> > +	  err = grub_netbuff_push (nb, sizeof (*opt) + sizeof (*duid));
> > +	  if (err)
> > +	    {
> > +	      grub_dhcpv6_sessions_free ();
> > +	      grub_netbuff_free (nb);
> > +	      return err;
> > +	    }
> > +
> > +	  opt = (struct grub_dhcpv6_option *)nb->data;
> > +	  opt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_CLIENTID);
> > +	  opt->len = grub_cpu_to_be16 (sizeof (*duid));
> > +
> > +	  duid = (struct grub_DUID_LL *) opt->data;
> > +
> > +	  duid->type = grub_cpu_to_be16_compile_time (3) ;
> > +	  duid->hw_type = grub_cpu_to_be16_compile_time (1);
> > +	  grub_memcpy (&duid->hwaddr, &session->iface->hwaddress.mac,
> > +	      sizeof (session->iface->hwaddress.mac));
> > +
> > +	  err = grub_netbuff_push (nb, sizeof (*opt) + sizeof (*ia_na));
> > +	  if (err)
> > +	    {
> > +	      grub_dhcpv6_sessions_free ();
> > +	      grub_netbuff_free (nb);
> > +	      return err;
> > +	    }
> > +
> > +	  opt = (struct grub_dhcpv6_option *)nb->data;
> > +	  opt->code = grub_cpu_to_be16_compile_time (GRUB_DHCPv6_OPTION_IA_NA);
> > +	  opt->len = grub_cpu_to_be16 (sizeof (*ia_na));
> > +	  ia_na = (struct grub_dhcpv6_iana_option *)opt->data;
> > +	  ia_na->iaid = grub_cpu_to_be32 (session->iaid);
> 
> Are you sure we are still aligned to 4 bytes here? You go from top to
> bottom and some fields are not multiple of 4 bytes. As mentioned
> earlier, may be ensure buffer is aligned in advance.
> 
> > +	  ia_na->t1 = 0;
> > +	  ia_na->t2 = 0;
> > +
> > +	  err = grub_netbuff_push (nb, sizeof (*v6));
> > +	  if (err)
> > +	    {
> > +	      grub_dhcpv6_sessions_free ();
> > +	      grub_netbuff_free (nb);
> > +	      return err;
> > +	    }
> > +
> > +	  v6 = (struct grub_net_dhcpv6_packet *)nb->data;
> > +	  v6->message_type = 1;
> 
> no magic constants, please.

OK.

> 
> > +	  v6->transaction_id = session->transaction_id;
> > +
> > +	  grub_netbuff_push (nb, sizeof (*udph));
> > +
> > +	  udph = (struct udphdr *) nb->data;
> > +	  udph->src = grub_cpu_to_be16_compile_time (546);
> > +	  udph->dst = grub_cpu_to_be16_compile_time (547);
> > +	  udph->chksum = 0;
> > +	  udph->len = grub_cpu_to_be16 (nb->tail - nb->data);
> > +
> > +	  udph->chksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_UDP,
> > +			    &session->iface->address, &multicast);
> > +
> > +	  err = grub_net_send_ip_packet (session->iface, &multicast,
> > +		    &ll_multicast, nb, GRUB_NET_IP_UDP);
> > +	  done = 0;
> > +	  grub_netbuff_free (nb);
> > +
> > +	  if (err)
> > +	    {
> > +	      grub_dhcpv6_sessions_free ();
> > +	      return err;
> > +	    }
> > +	}
> > +      if (!done)
> > +	grub_net_poll_cards (interval, 0);
> > +    }
> > +
> > +  FOR_DHCPV6_SESSIONS (session)
> > +    {
> > +      grub_error_push ();
> > +      err = grub_error (GRUB_ERR_FILE_NOT_FOUND,
> > +			N_("couldn't autoconfigure %s"),
> > +			session->iface->card->name);
> > +      grub_list_remove (GRUB_AS_LIST (session));
> > +      grub_free (session);
> > +      session = grub_dhcpv6_sessions;
> > +    }
> > +
> > +  return err;
> > +}
> > +
> > +static grub_command_t cmd_getdhcp, cmd_bootp, cmd_bootp6;
> >  
> >  void
> >  grub_bootp_init (void)
> > @@ -575,6 +1464,9 @@ grub_bootp_init (void)
> >    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."));
> > +  cmd_bootp6 = grub_register_command ("net_bootp6", grub_cmd_bootp6,
> > +				     N_("[CARD]"),
> > +				     N_("perform a DHCPv6 autoconfiguration"));
> >  }
> >  
> >  void
> > @@ -582,4 +1474,5 @@ grub_bootp_fini (void)
> >  {
> >    grub_unregister_command (cmd_getdhcp);
> >    grub_unregister_command (cmd_bootp);
> > +  grub_unregister_command (cmd_bootp6);
> >  }
> > diff --git a/grub-core/net/ip.c b/grub-core/net/ip.c
> > index 8c56baa..07d6871 100644
> > --- a/grub-core/net/ip.c
> > +++ b/grub-core/net/ip.c
> > @@ -238,6 +238,41 @@ handle_dgram (struct grub_net_buff *nb,
> >    {
> >      struct udphdr *udph;
> >      udph = (struct udphdr *) nb->data;
> > +
> > +    if (proto == GRUB_NET_IP_UDP && udph->dst == grub_cpu_to_be16_compile_time (546))
> > +      {
> > +	if (udph->chksum)
> > +	  {
> > +	    grub_uint16_t chk, expected;
> > +	    chk = udph->chksum;
> > +	    udph->chksum = 0;
> > +	    expected = grub_net_ip_transport_checksum (nb,
> > +						       GRUB_NET_IP_UDP,
> > +						       source,
> > +						       dest);
> > +	    if (expected != chk)
> > +	      {
> > +		grub_dprintf ("net", "Invalid UDP checksum. "
> > +			      "Expected %x, got %x\n",
> > +			      grub_be_to_cpu16 (expected),
> > +			      grub_be_to_cpu16 (chk));
> > +		grub_netbuff_free (nb);
> > +		return GRUB_ERR_NONE;
> > +	      }
> > +	    udph->chksum = chk;
> > +	  }
> > +
> > +	err = grub_netbuff_pull (nb, sizeof (*udph));
> > +	if (err)
> > +	  {
> > +	    grub_netbuff_free (nb);
> > +	    return err;
> > +	  }
> > +	grub_net_process_dhcp6 (nb, card);
> > +	grub_netbuff_free (nb);
> > +	return GRUB_ERR_NONE;
> > +      }
> > +
> >      if (proto == GRUB_NET_IP_UDP && grub_be_to_cpu16 (udph->dst) == 68)
> >        {
> >  	const struct grub_net_bootp_packet *bootp;
> > diff --git a/include/grub/net.h b/include/grub/net.h
> > index 538baa3..71dc243 100644
> > --- a/include/grub/net.h
> > +++ b/include/grub/net.h
> > @@ -418,6 +418,13 @@ struct grub_net_bootp_packet
> >    grub_uint8_t vendor[0];
> >  } GRUB_PACKED;
> >  
> > +struct grub_net_dhcpv6_packet
> > +{
> > +  grub_uint32_t message_type:8;
> > +  grub_uint32_t transaction_id:24;
> > +  grub_uint8_t dhcp_options[0];
> > +} GRUB_PACKED;
> > +
> >  #define	GRUB_NET_BOOTP_RFC1048_MAGIC_0	0x63
> >  #define	GRUB_NET_BOOTP_RFC1048_MAGIC_1	0x82
> >  #define	GRUB_NET_BOOTP_RFC1048_MAGIC_2	0x53
> > @@ -444,6 +451,14 @@ grub_net_configure_by_dhcp_ack (const char *name,
> >  				grub_size_t size,
> >  				int is_def, char **device, char **path);
> >  
> > +struct grub_net_network_level_interface *
> > +grub_net_configure_by_dhcpv6_reply (const char *name,
> > +				    struct grub_net_card *card,
> > +				    grub_net_interface_flags_t flags,
> > +				    const struct grub_net_dhcpv6_packet *v6,
> > +				    grub_size_t size,
> > +				    int is_def, char **device, char **path);
> > +
> >  grub_err_t
> >  grub_net_add_ipv4_local (struct grub_net_network_level_interface *inf,
> >  			 int mask);
> > @@ -452,6 +467,10 @@ void
> >  grub_net_process_dhcp (struct grub_net_buff *nb,
> >  		       struct grub_net_card *card);
> >  
> > +void
> > +grub_net_process_dhcp6 (struct grub_net_buff *nb,
> > +			struct grub_net_card *card);
> > +
> >  int
> >  grub_net_hwaddr_cmp (const grub_net_link_level_address_t *a,
> >  		     const grub_net_link_level_address_t *b);
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] Added net_bootp6 command
  2015-05-19  8:42     ` Michael Chang
@ 2015-05-30  7:25       ` Andrei Borzenkov
  0 siblings, 0 replies; 11+ messages in thread
From: Andrei Borzenkov @ 2015-05-30  7:25 UTC (permalink / raw)
  To: Michael Chang; +Cc: grub-devel

В Tue, 19 May 2015 16:42:00 +0800
Michael Chang <mchang@suse.com> пишет:

> > 
> > > +#define GRUB_DHCPv6_OPTION_CLIENTID 1
> > > +#define GRUB_DHCPv6_OPTION_SERVERID 2
> > > +#define GRUB_DHCPv6_OPTION_IA_NA 3
> > > +#define GRUB_DHCPv6_OPTION_IAADDR 5
> > > +#define GRUB_DHCPv6_OPTION_ORO 6
> > > +#define GRUB_DHCPv6_OPTION_ELAPSED_TIME 8
> > > +#define GRUB_DHCPv6_OPTION_DNS_SERVERS 23
> > > +#define GRUB_DHCPv6_OPTION_BOOTFILE_URL 59
> > > +
> > 
> > enum please. These can also be anonymous, no need to invent fancy
> > names.
> 
> I don't understand. Do you want me to use enum type here rather than
> macro ? Where are the fancy names invented here ? 

Sorry I mean there is no need to name enume type. Just

enum
  {
     GRUB_DHCPv6_OPTION_CLIENTID = 1
    ...
  };

> > > +static void
> > > +get_dhcpv6_dns_address (const struct grub_net_dhcpv6_packet *packet, grub_size_t size,
> > > +	grub_net_network_level_address_t **addr, grub_uint16_t *naddr)
> > > +{
> > > +  const struct grub_dhcpv6_option* popt;
> > > +  grub_uint16_t len, ln;
> > > +  const grub_uint8_t *pa;
> > > +  grub_net_network_level_address_t *la;
> > > +
> > > +  if (!addr || !naddr)
> > > +    {
> > > +      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("bad argument for get_dhcpv6_dns_address"));
> > > +      return;
> > > +    }
> > > +
> > 
> > it is called exactly once and grub_errno is immediately reset to
> > GRUB_ERR_NONE. So what's the point of returning error at all?
> 
> Because  a function wants to report error for bad inputs, while it's
> caller wants to suppress the error message because dns information could
> be missing in packets and is allowed (not treated as error).
> 

Sure but it is called exactly once and result is not used. It is not
like this is library function, it is more of a convenience macro. For
the same reasons NULL check on input is probably redundant as well.

> > 
> > > +  if (len == 0 || len & 0xf)
> > > +    {
> > > +      grub_error (GRUB_ERR_IO, N_("invalid dns address length"));
> > > +      return;
> > > +    }
> > > +
> > 
> > same question about grub_error. May be grub_debug?
> 
> No grub_debug, maybe grub_dprintf ? Maybe we should keep it and use
> specific error number for option not found ? See below for details.
> 

Sure, I mean dprintf, sorry.

> > > +
> > > +  /* Follow elilo and edk2 that check for starting and ending delimiter '[..]'
> > > +     in which IPv6 server address is enclosed. */
> > > +  if (*ip_start != '[')
> > > +    ip_start = NULL;
> > > +  else
> > > +    ip_end = grub_strchr (++ip_start, ']');
> > > +
> > > +  if (!ip_start || !ip_end)
> > > +    {
> > > +      grub_error (GRUB_ERR_IO, N_("IPv6-address not in square brackets"));
> > > +      goto cleanup;
> > > +    }
> > > +
> > 
> > RFC5970 says "Clients that have DNS implementations SHOULD support the
> > use of domain names in the URL" and in general string can also be valid
> > IPv4 address, nothing restricts it to IPv6. 
> 
> But the descriptions preceding it did explicitly say IPv6:
> 
> " If the host in the URL is expressed using an IPv6 address rather than
> a domain name, the address in the URL then MUST be enclosed in "[" and
> "]" characters, conforming to [RFC3986]. "
> 

it says "*if* it is using IPv6". Or at least so I understand it.

> > So I do not think you
> > should return error here, just return full string. I wish grub supported
> > IPv6 literals so we could just skip this check entirely.
> 
> OK. I'll return the string by assuming it's domain name.
> 
...
> > 
> > > +
> > > +  get_dhcpv6_dns_address (v6, size, &dns, &num_dns);
> > > +
> > > +  if (grub_errno)
> > > +    grub_errno = GRUB_ERR_NONE;
> > > +
> > 
> > This ignores legitimate errors like out of memory. As mentioned above,
> > does it need to set any grub_errno on its own? If not, errors should
> > not be ignored.
> 
> Because missing dns option is totally fine so that I don't want to treat
> it as error. We can use a specific error number for missing options and
> report other errors, but I can't find a suitable one for it (in
> include/grub/err.h).
> 

If missing DNS option is totally fine it should not be returned as
error. You already have indication (num_dns == 0) which is enough if
you want to inform user. Errors should be returned in case of e-h-h
errors :) 

Thanks! And sorry for delay.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-05-30  7:25 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-12  8:49 [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE Michael Chang
2015-05-12  8:49 ` [PATCH 1/3] Added net_bootp6 command Michael Chang
2015-05-15  6:26   ` Andrei Borzenkov
2015-05-15 13:57     ` Michael Chang
2015-05-16  5:42       ` Andrei Borzenkov
2015-05-19  8:42     ` Michael Chang
2015-05-30  7:25       ` Andrei Borzenkov
2015-05-12  8:49 ` [PATCH 2/3] UEFI IPv6 PXE support Michael Chang
2015-05-12  8:49 ` [PATCH 3/3] Update document for net_bootp6 command Michael Chang
2015-05-15  6:40 ` [PATCH v1] Support DHCPv6 and UEFI IPv6 PXE Andrei Borzenkov
2015-05-15 14:15   ` Michael Chang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.