Re: [PATCH 1/3] Added net_bootp6 command

2015-05-30 Thread Andrei Borzenkov
В Tue, 19 May 2015 16:42:00 +0800
Michael Chang mch...@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.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH 1/3] Added net_bootp6 command

2015-05-19 Thread Michael Chang
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 mch...@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;
  

Re: [PATCH 1/3] Added net_bootp6 command

2015-05-15 Thread Andrei Borzenkov
В Tue, 12 May 2015 16:49:48 +0800
Michael Chang mch...@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)
 + {
 +   

Re: [PATCH 1/3] Added net_bootp6 command

2015-05-15 Thread Michael Chang
On Fri, May 15, 2015 at 09:26:06AM +0300, Andrei Borzenkov wrote:
 В Tue, 12 May 2015 16:49:48 +0800
 Michael Chang mch...@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

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH 1/3] Added net_bootp6 command

2015-05-15 Thread Andrei Borzenkov
В Fri, 15 May 2015 21:57:28 +0800
Michael Chang mch...@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 mch...@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).
 

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


[PATCH 1/3] Added net_bootp6 command

2015-05-12 Thread Michael Chang
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));

Re: [PATCH 1/3] Added net_bootp6 command

2015-04-19 Thread Andrei Borzenkov
В Fri, 17 Apr 2015 13:04:27 +0800
Michael Chang mch...@suse.com пишет:

  
   +static const struct grub_dhcpv6_option*
   +find_dhcpv6_option (const struct grub_net_dhcpv6_packet *packet,
   + grub_uint16_t option)
   +{
   +  grub_uint16_t code, len;
   +  const struct grub_dhcpv6_option *popt;
   +
   +  popt = (const struct grub_dhcpv6_option *)packet-dhcp_options;
   +  code = grub_be_to_cpu16 (popt-code);
   +  len = grub_be_to_cpu16 (popt-len);
   +
   +  while (0 != code  option != code)
  
  This probably needs upper boundary check in case we are dealing with
  corrupted packets.
 
 
 Do you mean checking the option length can't exceed netbuff length?
 

Yes (and in all other cases below as well).

 The ( 0!= code ) did certain kind of upper boundary check for me, but
 I know you may disagree that. :)
 
 Btw, in grub-core/net/ip.c::handle_dgram() has checksum for the receiving
 packets, so the corrputed packets wouldn't sneak in easily.
 

Correct checksum just means content was not altered; it does not mean
content was correct to start with.

   +
   +  ip_start = ip_end = NULL;
   +  ip_start = bootfile_url + grub_strlen(pr);
   +
   +  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;
   +}
   +
  
  Can bootfile_url contain a name and not IPv6 address? Or is address
  mandatory?
 
 Yes. The string in URL form is mandatory.


I probably was not clear. Must it always be IPv6 address literal as
implied by code or can it be e.g. DNS name?

   +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 __attribute__ ((unused)),
   + 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 != DHCPv6_REPLY)
  
  Is it really possible? We come here if packet is DHCPv6_REPLY only.
 
 I'm in case some evil firmware to cache some other packets type rather
 than reply (or even junks in it ..).


Yes, but what I mean - this function is only called when we already
checked for message_type == DHCPv6_REPLY. So the only reason it can be
different here is physical memory corruption.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH 1/3] Added net_bootp6 command

2015-04-19 Thread Michael Chang
On Sun, Apr 19, 2015 at 11:15:21AM +0300, Andrei Borzenkov wrote:
 В Fri, 17 Apr 2015 13:04:27 +0800
 Michael Chang mch...@suse.com пишет:
 
   
+static const struct grub_dhcpv6_option*
+find_dhcpv6_option (const struct grub_net_dhcpv6_packet *packet,
+   grub_uint16_t option)
+{
+  grub_uint16_t code, len;
+  const struct grub_dhcpv6_option *popt;
+
+  popt = (const struct grub_dhcpv6_option *)packet-dhcp_options;
+  code = grub_be_to_cpu16 (popt-code);
+  len = grub_be_to_cpu16 (popt-len);
+
+  while (0 != code  option != code)
   
   This probably needs upper boundary check in case we are dealing with
   corrupted packets.
  
  
  Do you mean checking the option length can't exceed netbuff length?
  
 
 Yes (and in all other cases below as well).

OK.

 
  The ( 0!= code ) did certain kind of upper boundary check for me, but
  I know you may disagree that. :)
  
  Btw, in grub-core/net/ip.c::handle_dgram() has checksum for the receiving
  packets, so the corrputed packets wouldn't sneak in easily.
  
 
 Correct checksum just means content was not altered; it does not mean
 content was correct to start with.

I agree with you that it doesn't mean the content was OK to start with,
the sender might intentionally or unintentionally fill the payload
length wrong that triggers buffer overflow on the client side.

FWIW, the answer is subject to corrupted packet that usually means
corruption during the transit and checksum is generally used for
detecting such errors . 

 
+
+  ip_start = ip_end = NULL;
+  ip_start = bootfile_url + grub_strlen(pr);
+
+  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;
+}
+
   
   Can bootfile_url contain a name and not IPv6 address? Or is address
   mandatory?
  
  Yes. The string in URL form is mandatory.
 
 
 I probably was not clear. Must it always be IPv6 address literal as
 implied by code or can it be e.g. DNS name?

The DNS name could be legit for URL, but current elilo and edk2
explicitly checks for the starting and ending delimiter '[..]' for using
the server address and any name is treated as invalid parameters. The
implementation just followed them.

 
+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 __attribute__ ((unused)),
+   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 != DHCPv6_REPLY)
   
   Is it really possible? We come here if packet is DHCPv6_REPLY only.
  
  I'm in case some evil firmware to cache some other packets type rather
  than reply (or even junks in it ..).
 
 
 Yes, but what I mean - this function is only called when we already
 checked for message_type == DHCPv6_REPLY. So the only reason it can be
 different here is physical memory corruption.

It also gets called from grub_efi_net_config_real () which doesn't have
any check for message type.

Thanks,
Michael

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH 1/3] Added net_bootp6 command

2015-04-19 Thread Andrei Borzenkov
В Mon, 20 Apr 2015 11:09:10 +0800
Michael Chang mch...@suse.com пишет:

 
  
 +
 +  ip_start = ip_end = NULL;
 +  ip_start = bootfile_url + grub_strlen(pr);
 +
 +  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;
 +}
 +

Can bootfile_url contain a name and not IPv6 address? Or is address
mandatory?
   
   Yes. The string in URL form is mandatory.
  
  
  I probably was not clear. Must it always be IPv6 address literal as
  implied by code or can it be e.g. DNS name?
 
 The DNS name could be legit for URL, but current elilo and edk2
 explicitly checks for the starting and ending delimiter '[..]' for using
 the server address and any name is treated as invalid parameters. The
 implementation just followed them.
 

OK, could you put in comments here which explain it, probably marked as
TODO.

  
  Yes, but what I mean - this function is only called when we already
  checked for message_type == DHCPv6_REPLY. So the only reason it can be
  different here is physical memory corruption.
 
 It also gets called from grub_efi_net_config_real () which doesn't have
 any check for message type.
 

So you do not trust firmware, do you? :) But OK, let's leave check in
place.

P.S. grub_efi_net_config_real should actually have checked
DhcpAckReceived to start with, but I'm afraid to add it now, at least
until we have real report of invalid packages.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH 1/3] Added net_bootp6 command

2015-04-16 Thread Andrei Borzenkov
В Wed, 15 Apr 2015 17:05:07 +0800
Michael Chang mch...@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  |  885 
 +++-
  grub-core/net/ip.c |   35 ++
  include/grub/efi/api.h |   56 +++-
  include/grub/net.h |   19 +
  4 files changed, 993 insertions(+), 2 deletions(-)
 
 diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
 index 6136755..477f205 100644
 --- a/grub-core/net/bootp.c
 +++ b/grub-core/net/bootp.c
 @@ -24,6 +24,7 @@
  #include grub/net/netbuff.h
  #include grub/net/udp.h
  #include grub/datetime.h
 +#include grub/time.h
  
  static void
  parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
 @@ -256,6 +257,653 @@ 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;
 +
 +struct grub_dhcpv6_dns_servers {
 +  grub_uint8_t addr[16];
 +  grub_uint8_t next_addr[0];
 +} GRUB_PACKED;
 +

Do we really need it? Code just fetches 16 bytes blocks, it does not
really need any structure definition?

 +#define DHCPv6_REPLY 7
 +#define DHCPv6_ADVERTISE 2
 +#define DHCPv6_REQUEST 3
 +#define OPTION_BOOTFILE_URL 59
 +#define OPTION_DNS_SERVERS 23
 +#define OPTION_IA_NA 3
 +#define OPTION_IAADDR 5
 +#define OPTION_CLIENTID 1
 +#define OPTION_SERVERID 2
 +#define OPTION_ORO 6
 +#define OPTION_ELAPSED_TIME 8
 +

This is better as enum and GRUB_ prefix. Also options need GRUB_DHCPv6_
prefix.

 +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 *ifaces;

Can there be multiple interfaces as implied by plural?

 +};
 +
 +static struct grub_dhcpv6_session *grub_dhcpv6_sessions = NULL;
 +#define FOR_DHCPV6_SESSIONS(var) \
 +for (var = grub_dhcpv6_sessions ; var; var = var-next)
 +

FOR_LIST_ELEMENTS

 +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 ();
 +
 +  session-prev = grub_dhcpv6_sessions;
 +  session-next = grub_dhcpv6_sessions;
 +
 +  if (session-next)
 +session-next-prev = session-next;
 +

grub_list_push

 +  grub_dhcpv6_sessions = session;
 +  return;
 +}
 +
 +static void
 +grub_dhcpv6_session_remove (struct grub_dhcpv6_session *session)
 +{
 +  *session-prev = session-next;
 +  if (session-next)
 +session-next-prev = session-prev;
 +  session-next = NULL;
 +  session-prev = NULL;
 +  return;
 +}
 +

grub_list_remove

 +static const struct grub_dhcpv6_option*
 +find_dhcpv6_option (const struct grub_net_dhcpv6_packet *packet,
 + grub_uint16_t option)
 +{
 +  grub_uint16_t code, len;
 +  const struct grub_dhcpv6_option *popt;
 +
 +  popt = (const struct grub_dhcpv6_option *)packet-dhcp_options;
 +  code = grub_be_to_cpu16 (popt-code);
 +  len = grub_be_to_cpu16 (popt-len);
 +
 +  while (0 != code  option != code)

This probably needs upper boundary check in case we are dealing with
corrupted packets.

 +{
 +  popt = (const struct grub_dhcpv6_option *)((grub_uint8_t *)popt +
 + len + sizeof(*popt));
 +  code = grub_be_to_cpu16 (popt-code);
 +  len = grub_be_to_cpu16 (popt-len);
 +}
 +
 +  if (option == code)
 +  return popt;
 +

This can just be moved inside a loop, right?

 +  return NULL;
 +}
 +
 +static const grub_uint8_t*
 +find_dhcpv6_address (const struct grub_net_dhcpv6_packet *packet)
 +{
 +  const struct grub_dhcpv6_option* popt = find_dhcpv6_option (packet, 
 OPTION_IA_NA);
 +  const struct grub_dhcpv6_iana_option *ia_na;
 +  const struct grub_dhcpv6_option *iaaddr_hdr;
 +  const struct grub_dhcpv6_iaaddr_option *iaaddr;
 +  grub_uint16_t ia_na_data_offset, ia_na_data_len, len;
 +
 +  if (grub_be_to_cpu16 (popt-code) != OPTION_IA_NA)

if (popt == NULL) 

 +{
 +  grub_error (GRUB_ERR_IO, N_(not an IA_NA DHCPv6 option));
 +  return NULL;
 +}
 +
 +  ia_na = (const struct grub_dhcpv6_iana_option *)popt-data;
 +
 

Re: [PATCH 1/3] Added net_bootp6 command

2015-04-16 Thread Michael Chang
Hi Andrei,

Please see my comments and please let me know any problems remains.

I'll submit next version soon.

Thanks a lot for your detailed review.

Regards,
Michael

On Thu, Apr 16, 2015 at 05:40:56PM +0300, Andrei Borzenkov wrote:
 В Wed, 15 Apr 2015 17:05:07 +0800
 Michael Chang mch...@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  |  885 
  +++-
   grub-core/net/ip.c |   35 ++
   include/grub/efi/api.h |   56 +++-
   include/grub/net.h |   19 +
   4 files changed, 993 insertions(+), 2 deletions(-)
  
  diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
  index 6136755..477f205 100644
  --- a/grub-core/net/bootp.c
  +++ b/grub-core/net/bootp.c
  @@ -24,6 +24,7 @@
   #include grub/net/netbuff.h
   #include grub/net/udp.h
   #include grub/datetime.h
  +#include grub/time.h
   
   static void
   parse_dhcp_vendor (const char *name, const void *vend, int limit, int 
  *mask)
  @@ -256,6 +257,653 @@ 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;
  +
  +struct grub_dhcpv6_dns_servers {
  +  grub_uint8_t addr[16];
  +  grub_uint8_t next_addr[0];
  +} GRUB_PACKED;
  +
 
 Do we really need it? Code just fetches 16 bytes blocks, it does not
 really need any structure definition?

No, It doesn't really matter that much. I was just sticking to use
structure to represent any option data and hopefully that helps in
understanding what it is. But in this case it seems to be the oppsite by
treating it as 16 bytes chunk of data generates more tidy code and
easier to understand.

I can change to your suggested code as commented in get_dns function and
remove this structure.

 
  +#define DHCPv6_REPLY 7
  +#define DHCPv6_ADVERTISE 2
  +#define DHCPv6_REQUEST 3
  +#define OPTION_BOOTFILE_URL 59
  +#define OPTION_DNS_SERVERS 23
  +#define OPTION_IA_NA 3
  +#define OPTION_IAADDR 5
  +#define OPTION_CLIENTID 1
  +#define OPTION_SERVERID 2
  +#define OPTION_ORO 6
  +#define OPTION_ELAPSED_TIME 8
  +
 
 This is better as enum and GRUB_ prefix. Also options need GRUB_DHCPv6_
 prefix.

OK.

 
  +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 *ifaces;
 
 Can there be multiple interfaces as implied by plural?

No, One session for configuring one interface, the plural just created
confusing. I'll fix it.

 
  +};
  +
  +static struct grub_dhcpv6_session *grub_dhcpv6_sessions = NULL;
  +#define FOR_DHCPV6_SESSIONS(var) \
  +for (var = grub_dhcpv6_sessions ; var; var = var-next)
  +
 
 FOR_LIST_ELEMENTS

OK.
 
  +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 ();
  +
  +  session-prev = grub_dhcpv6_sessions;
  +  session-next = grub_dhcpv6_sessions;
  +
  +  if (session-next)
  +session-next-prev = session-next;
  +
 
 grub_list_push

OK.
 
  +  grub_dhcpv6_sessions = session;
  +  return;
  +}
  +
  +static void
  +grub_dhcpv6_session_remove (struct grub_dhcpv6_session *session)
  +{
  +  *session-prev = session-next;
  +  if (session-next)
  +session-next-prev = session-prev;
  +  session-next = NULL;
  +  session-prev = NULL;
  +  return;
  +}
  +
 
 grub_list_remove

OK.
 
  +static const struct grub_dhcpv6_option*
  +find_dhcpv6_option (const struct grub_net_dhcpv6_packet *packet,
  +   grub_uint16_t option)
  +{
  +  grub_uint16_t code, len;
  +  const struct grub_dhcpv6_option *popt;
  +
  +  popt = (const struct grub_dhcpv6_option *)packet-dhcp_options;
  +  code = grub_be_to_cpu16 (popt-code);
  +  len = grub_be_to_cpu16 (popt-len);
  +
  +  while (0 != code  option != code)
 
 This probably needs upper boundary check in case we are dealing with
 corrupted packets.


Do you mean checking the option length can't exceed netbuff length?

The 

[PATCH 1/3] Added net_bootp6 command

2015-04-15 Thread Michael Chang
The net_bootp6 is used to configure the ipv6 network interface through the
DHCPv6 protocol Solict/Advertise/Request/Reply.
---
 grub-core/net/bootp.c  |  885 +++-
 grub-core/net/ip.c |   35 ++
 include/grub/efi/api.h |   56 +++-
 include/grub/net.h |   19 +
 4 files changed, 993 insertions(+), 2 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 6136755..477f205 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -24,6 +24,7 @@
 #include grub/net/netbuff.h
 #include grub/net/udp.h
 #include grub/datetime.h
+#include grub/time.h
 
 static void
 parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
@@ -256,6 +257,653 @@ 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;
+
+struct grub_dhcpv6_dns_servers {
+  grub_uint8_t addr[16];
+  grub_uint8_t next_addr[0];
+} GRUB_PACKED;
+
+#define DHCPv6_REPLY 7
+#define DHCPv6_ADVERTISE 2
+#define DHCPv6_REQUEST 3
+#define OPTION_BOOTFILE_URL 59
+#define OPTION_DNS_SERVERS 23
+#define OPTION_IA_NA 3
+#define OPTION_IAADDR 5
+#define OPTION_CLIENTID 1
+#define OPTION_SERVERID 2
+#define OPTION_ORO 6
+#define OPTION_ELAPSED_TIME 8
+
+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 *ifaces;
+};
+
+static struct grub_dhcpv6_session *grub_dhcpv6_sessions = NULL;
+#define FOR_DHCPV6_SESSIONS(var) \
+for (var = grub_dhcpv6_sessions ; var; var = var-next)
+
+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 ();
+
+  session-prev = grub_dhcpv6_sessions;
+  session-next = grub_dhcpv6_sessions;
+
+  if (session-next)
+session-next-prev = session-next;
+
+  grub_dhcpv6_sessions = session;
+  return;
+}
+
+static void
+grub_dhcpv6_session_remove (struct grub_dhcpv6_session *session)
+{
+  *session-prev = session-next;
+  if (session-next)
+session-next-prev = session-prev;
+  session-next = NULL;
+  session-prev = NULL;
+  return;
+}
+
+static const struct grub_dhcpv6_option*
+find_dhcpv6_option (const struct grub_net_dhcpv6_packet *packet,
+   grub_uint16_t option)
+{
+  grub_uint16_t code, len;
+  const struct grub_dhcpv6_option *popt;
+
+  popt = (const struct grub_dhcpv6_option *)packet-dhcp_options;
+  code = grub_be_to_cpu16 (popt-code);
+  len = grub_be_to_cpu16 (popt-len);
+
+  while (0 != code  option != code)
+{
+  popt = (const struct grub_dhcpv6_option *)((grub_uint8_t *)popt +
+   len + sizeof(*popt));
+  code = grub_be_to_cpu16 (popt-code);
+  len = grub_be_to_cpu16 (popt-len);
+}
+
+  if (option == code)
+  return popt;
+
+  return NULL;
+}
+
+static const grub_uint8_t*
+find_dhcpv6_address (const struct grub_net_dhcpv6_packet *packet)
+{
+  const struct grub_dhcpv6_option* popt = find_dhcpv6_option (packet, 
OPTION_IA_NA);
+  const struct grub_dhcpv6_iana_option *ia_na;
+  const struct grub_dhcpv6_option *iaaddr_hdr;
+  const struct grub_dhcpv6_iaaddr_option *iaaddr;
+  grub_uint16_t ia_na_data_offset, ia_na_data_len, len;
+
+  if (grub_be_to_cpu16 (popt-code) != OPTION_IA_NA)
+{
+  grub_error (GRUB_ERR_IO, N_(not an IA_NA DHCPv6 option));
+  return NULL;
+}
+
+  ia_na = (const struct grub_dhcpv6_iana_option *)popt-data;
+
+  if (grub_be_to_cpu16(popt-len) = sizeof (*ia_na))
+{
+  grub_error (GRUB_ERR_IO, N_(invalid size for IAADDR));
+  return NULL;
+}
+
+  ia_na_data_len = grub_be_to_cpu16(popt-len) - sizeof (*ia_na);
+  ia_na_data_offset = 0;
+
+  iaaddr_hdr = (const struct grub_dhcpv6_option *) ia_na-data;
+  len = grub_be_to_cpu16 (iaaddr_hdr-len);
+
+  while (grub_be_to_cpu16(iaaddr_hdr-code) != OPTION_IAADDR)
+{
+  ia_na_data_offset += (len + sizeof (*iaaddr_hdr));
+
+  if (ia_na_data_offset  ia_na_data_len)
+   {
+ iaaddr_hdr =(const struct grub_dhcpv6_option *)(ia_na-data +
+   ia_na_data_offset);
+ len = grub_be_to_cpu16 (iaaddr_hdr-len);
+