[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/3] Added net_bootp6 command
|
From: |
Michael Chang |
|
Subject: |
Re: [PATCH 1/3] Added net_bootp6 command |
|
Date: |
Fri, 17 Apr 2015 13:04:27 +0800 |
|
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden> пишет:
>
> > 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 ( 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.
>
> > + {
> > + 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?
OK.
>
> > + 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)
OK.
>
> > + {
> > + 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;
> > + }
> > +
>
> Does it need upper boundary check?
If the check has been done in find_dhcpv6_option I don't think we need
to check again here, The find_dhcpv6_option should ensure that all
length are within the netbuff range before returning it.
>
> > + 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)
>
> grub_cpu_to_be16_compile_time(OPTION_IAADDR)
OK.
>
> > + {
> > + 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);
> > + }
> > + else
> > + {
> > + iaaddr_hdr = NULL;
> > + break;
> > + }
> > + }
> > +
> > + if (!iaaddr_hdr)
> > + {
> > + grub_error (GRUB_ERR_IO, N_("IAADDR not found"));
> > + return NULL;
> > + }
> > +
> > + if ((ia_na_data_offset + sizeof (*iaaddr_hdr) + len) > ia_na_data_len)
> > + {
> > + grub_error (GRUB_ERR_IO, N_("IAADDR size check failed"));
> > + return NULL;
> > + }
> > +
> > + iaaddr = (const struct grub_dhcpv6_iaaddr_option *) iaaddr_hdr->data;
> > +
> > + return iaaddr->addr;
>
> It sounds again like most code could be folded inside a loop
>
> while (remaining_length > 0)
> if (option_length > remaining_length)
> error
> if (option_code == IAADDR)
> return hdr->addr
> hdr = (char *)hdr + option_length
> remaining_length -= option_length
>
> return not found
OK. I'll clean it for more readable code like what you suggested.
> > +}
> > +
> > +static void
> > +get_dhcpv6_dns_address (const struct grub_net_dhcpv6_packet *packet,
> > + grub_net_network_level_address_t **addr, grub_uint16_t *naddr)
> > +{
> > + const struct grub_dhcpv6_option* popt;
> > + const struct grub_dhcpv6_dns_servers *dns;
> > + grub_uint16_t len;
> > + const grub_uint8_t *pa;
> > + int i, ln;
> > + grub_net_network_level_address_t *la;
> > +
> > + if (addr)
> > + *addr = NULL;
> > +
> > + if (naddr)
> > + *naddr = 0;
> > +
> > + popt = find_dhcpv6_option (packet, OPTION_DNS_SERVERS);
> > + if (!popt)
> > + return;
> > +
> > + len = grub_be_to_cpu16 (popt->len);
> > + if ((len % 16) != 0)
>
> len & 0xf; 0 comparison probably redundant as well.
OK.
>
> Again, what about upper boundary check?
See above.
>
> > + {
> > + grub_error (GRUB_ERR_IO, N_("invalid dns address length"));
> > + return;
> > + }
> > +
> > + dns = (const struct grub_dhcpv6_dns_servers *)popt->data;
> > +
> > + ln = len / 16;
>
> len >> 4
OK.
>
> > + la = grub_zalloc (sizeof (grub_net_network_level_address_t) * ln);
> > +
>
> *addr = la = grub_zalloc (...)
>
> NULL check
OK.
>
> > + for (i = 0, pa = dns->addr; i < ln; i++, pa = dns->next_addr)
> ^^^ not needed pa += 16, la++
> > + {
> > + (la + i)->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> > + (la + i)->ipv6[0] = grub_get_unaligned64 (pa);
> > + (la + i)->ipv6[1] = grub_get_unaligned64 (pa + 8);
> > + (la + i)->option = DNS_OPTION_PREFER_IPV6;
>
> la->...
>
> > + }
> > +
> > + *addr = la;
> not needed
See above (agree to use 16 byte chuncks).
>
> > + *naddr = ln;
> > +
> > + return;
> > +}
> > +
> > +static void
> > +find_dhcpv6_bootfile_url (const struct grub_net_dhcpv6_packet *packet,
> > + 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 (packet, OPTION_BOOTFILE_URL);
> > +
> > + if (!opt_url)
> > + {
> > + grub_error (GRUB_ERR_IO, N_("no bootfile-url in DHCPv6 option"));
> > + return;
> > + }
> > +
> > + len = grub_be_to_cpu16 (opt_url->len);
> > +
>
> Obligatory question about upper boundary check :)
See above. Do it in find_dhcpv6_option once and for all? :)
>
> > + 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);
> > +
> > + 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.
>
> > + 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';
>
> Umm ... we really need something like grub_mem2str. Actually
> grub_xasprintf("%.*s") would be just fine if grub supported it.
OK.
>
> > + }
> > +
> > + 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);
> > +
>
> grub_free checks for NULL
OK.
>
> > + if (grub_errno)
> > + {
> > + if (proto && *proto)
> > + {
> > + grub_free (proto);
>
> grub_free (*proto)
OK.
>
> > + *proto = NULL;
> > + }
> > +
> > + if (server_ip && *server_ip)
> > + {
> > + grub_free (server_ip);
> grub_free (*server_ip)
OK.
>
> > + *server_ip = NULL;
> > + }
> > +
> > + if (boot_file && *boot_file)
> > + {
> > + grub_free (boot_file);
> grub_free (*boot_file)
OK.
>
> > + *boot_file = NULL;
> > + }
> > + }
> > +
> > + return;
> > +}
> > +
> > +
> > +static grub_err_t
> > +grub_net_configure_by_dhcpv6_adv (const struct grub_net_dhcpv6_packet
> > *v6_adv,
> > + 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_err_t err;
> > + grub_uint16_t len;
> > + grub_uint64_t elapsed;
> > + char err_msg[64];
> > +
> > + if (v6_adv->message_type != DHCPv6_ADVERTISE)
>
> We come here after checking message type, or am I wrong?
OK. We either have to remove the outer check or this,
>
> > + {
> > + grub_error (GRUB_ERR_IO, N_("DHCPv6 info not found"));
> > + return grub_errno;
> > + }
> > +
> > + opt_client = find_dhcpv6_option (v6_adv, OPTION_CLIENTID);
> > + opt_server = find_dhcpv6_option (v6_adv, OPTION_SERVERID);
> > + opt_iana = find_dhcpv6_option (v6_adv, OPTION_IA_NA);
> > +
> > + err_msg[0] = '\0';
> > + if (!opt_client)
> > + grub_strcpy (err_msg, "client id");
> > +
> > + if (!opt_server)
> > + {
> > + if (grub_strlen (err_msg))
> > + grub_strcpy (err_msg + grub_strlen (err_msg), ", server id");
> > + else
> > + grub_strcpy (err_msg, "server id");
> > + }
> > +
> > + if (!opt_iana)
> > + {
> > + if (grub_strlen (err_msg))
> > + grub_strcpy (err_msg + grub_strlen (err_msg), ", iana");
> > + else
> > + grub_strcpy (err_msg, "iana");
> > + }
> > +
> > + if (grub_strlen (err_msg))
> > + {
> > + grub_strcpy (err_msg + grub_strlen (err_msg), " missing");
> > + grub_error (GRUB_ERR_IO, N_(err_msg));
>
> This means it will not be extracted by xgettext and we need 7 different
> strings added manually. This needs some change if you really want it to
> be translated. May be use grub_error_push with "Mandatory DHCPv6 option
> %s missing" or similar?
OK.
>
> > + return grub_errno;
> > + }
> > +
> > + inf = session->ifaces;
> > +
> > + 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 (512);
>
> Symbolic constant would be nice. Where this size comes from?
It's quite liberal : any size that's sufficiently large to hold the
incoming populated netbuff data. :)
>
> > +
> > + if (!nb)
> > + {
> > + grub_netbuff_free (nb);
>
> nb is NULL at this point, not?
Duh, really stupid code. Will fix it.
>
> > + return grub_errno;
> > + }
> > +
> > + err = grub_netbuff_reserve (nb, 512);
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > +
> > + len = grub_cpu_to_be16(opt_client->len);
> > + err = grub_netbuff_push (nb, len + 4);
> len + sizeof (*opt_client)
OK.
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > + grub_memcpy (nb->data, opt_client, len + 4);
> ditto
OK.
>
> > +
> > + len = grub_cpu_to_be16(opt_server->len);
> > + err = grub_netbuff_push (nb, len + 4);
> ditto
OK.
>
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > + grub_memcpy (nb->data, opt_server, len + 4);
> ditto
OK.
>
> > +
> > + len = grub_cpu_to_be16(opt_iana->len);
> > + err = grub_netbuff_push (nb, len + 4);
> ditto
OK.
>
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > + grub_memcpy (nb->data, opt_iana, len + 4);
> ditto
OK.
>
> > +
> > + err = grub_netbuff_push (nb, 8);
> sizeof (struct grub_dhcpv6_option) + 2 * sizeof (grub_uint16_t)
> probably makes it more clear.
OK.
>
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > +
> > + popt = (struct grub_dhcpv6_option*) nb->data;
> > + popt->code = grub_cpu_to_be16_compile_time (OPTION_ORO);
> > + popt->len = grub_cpu_to_be16_compile_time (4);
>
> and here 2 * sizeof (grub_uint16_t) as well.
OK.
>
> > + grub_set_unaligned16 (popt->data, grub_cpu_to_be16_compile_time
> > (OPTION_BOOTFILE_URL));
> > + grub_set_unaligned16 (popt->data + 2, grub_cpu_to_be16_compile_time
> > (OPTION_DNS_SERVERS));
> > +
> > + err = grub_netbuff_push (nb, 6);
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > + popt = (struct grub_dhcpv6_option*) nb->data;
> > + popt->code = grub_cpu_to_be16_compile_time (OPTION_ELAPSED_TIME);
> > + popt->len = grub_cpu_to_be16_compile_time (2);
> > +
>
> similar
>
> > + // the time is expressed in hundredths of a second
> Please, let's stick to C comments
OK.
>
> > + 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, 4);
> also better sizeof to avoid magic constants
OK.
>
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > +
> > + v6 = (struct grub_net_dhcpv6_packet *) nb->data;
> > + v6->message_type = 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);
> > +
> > + if (err)
>
> Just initialize it from start to GRUB_ERR_NONE?
OK.
>
> > + return err;
> > +
> > + return GRUB_ERR_NONE;
> > +}
> > +
> > +
> > +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 ..).
>
> > + {
> > + grub_error (GRUB_ERR_IO, N_("DHCPv6 info not found"));
> > + return NULL;
> > + }
> > +
> > + your_ip = find_dhcpv6_address(v6);
> > +
> > + if (!your_ip)
> > + {
> > + grub_error (GRUB_ERR_IO, N_("DHCPv6 address not found"));
> > + return NULL;
> > + }
> > +
> > + get_dhcpv6_dns_address (v6, &dns, &num_dns);
> > +
> > + if (dns && num_dns)
> > + {
> > + int i;
> > +
> > + for (i = 0; i < num_dns; ++i)
> > + grub_net_add_dns_server (dns + i);
> > +
> > + grub_free (dns);
> > + }
> > + else
> > + {
> > + if (grub_errno)
> > + grub_print_error ();
> > + }
> > +
>
> braces not needed.
OK.
>
> > + find_dhcpv6_bootfile_url (v6, &proto, &server_ip, &boot_file);
> > +
> > + if (grub_errno)
> > + grub_print_error ();
> > +
> > + 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);
> > +
> > + grub_env_set_net_property (name, "boot_file", boot_file,
> > + grub_strlen (boot_file));
> > +
>
> boot_file can be NULL, right?
Yes. I will fix it.
>
> > + 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)
> > + return NULL;
> > + }
> > +
> > + 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
> > + return NULL;
> > + }
> > +
> > + return inf;
>
> This smells like memory leak in multiple places above. proto,
> server_ip, etc
Yes. I will fix them.
>
> > +}
> > +
> > void
> > grub_net_process_dhcp (struct grub_net_buff *nb,
> > struct grub_net_card *card)
> > @@ -288,6 +936,67 @@ 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;
> > +
> > + v6 = (const struct grub_net_dhcpv6_packet *) nb->data;
> > +
> > + opt_iana = find_dhcpv6_option (v6, OPTION_IA_NA);
> > + if (!opt_iana)
> > + 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 == DHCPv6_ADVERTISE)
> > + {
> > + grub_net_configure_by_dhcpv6_adv (
> > + (const struct grub_net_dhcpv6_packet*) nb->data, session);
> > + }
> > + else if (v6->message_type == DHCPv6_REPLY)
> > + {
> > + char *name;
> > + struct grub_net_network_level_interface *inf;
> > +
> > + inf = session->ifaces;
> > + name = grub_xasprintf ("%s:dhcp", inf->card->name);
> > + if (!name)
> > + return;
> > +
> > + grub_net_configure_by_dhcpv6_reply (name, inf->card,
> > + 0, (const struct grub_net_dhcpv6_packet *) nb->data,
> > + (nb->tail - nb->data), 0, 0, 0);
> > +
> > + if (!grub_errno)
> > + {
> > + grub_dhcpv6_session_remove (session);
> > + grub_free (session);
> > + }
> > +
> > + grub_free (name);
> > + }
> > +
> > + if (grub_errno)
> > + grub_print_error ();
> > +
> > + return;
> > +}
> > +
> > static char
> > hexdigit (grub_uint8_t val)
> > {
> > @@ -564,7 +1273,177 @@ 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;
> > + grub_size_t ncards = 0;
> > + 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;
> > + ncards++;
> > + }
> > +
>
> where ncards is used?
It's not used, simply uncleaned code copy-paste from origianl net_bootp
command.
>
> > + FOR_NET_CARDS (card)
> > + {
> > + struct grub_net_network_level_interface *ifaces;
> > +
>
> Why plural? It was array in original code, but not here, right?
Yes. You bet it. I will fix it.
>
> > + if (argc > 0 && grub_strcmp (card->name, args[0]) != 0)
> > + continue;
> > +
> > + ifaces = grub_net_ipv6_get_link_local (card, &card->default_address);
> > + if (!ifaces)
> > + {
> > + grub_free (ifaces);
>
> You still insist on freeing NULL pointers :)
(Sigh). I'm speech-less to my own stupid code. :(
It will be fixed next version.
>
> > + return grub_errno;
> > + }
> > +
>
> Allocated sessions are leaked.
OK.
>
> > + session = grub_zalloc (sizeof (*session));
> > + session->ifaces = ifaces;
> > + session->iaid = j;
> > + grub_dhcpv6_session_add (session);
> > + j++;
> > + }
> > +
> > + for (interval = 200; interval < 10000; interval *= 2)
> > + {
> > + int done = 1;
> > +
> > + FOR_DHCPV6_SESSIONS (session)
> > + {
> Something strange with indentation.
OK.
>
> > + 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->ifaces,
> > + &multicast, &ll_multicast);
> > + if (err)
> > + return grub_errno;
>
> Allocated sessions?
>
> > + nb = grub_netbuff_alloc (512);
> > + if (!nb)
> > + {
> > + grub_netbuff_free (nb);
>
> free(NULL)
>
> > + return grub_errno;
> > + }
> > +
>
> memory leak
OK.
>
> > + err = grub_netbuff_reserve (nb, 512);
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > +
>
> memory leak
OK.
>
> > + err = grub_netbuff_push (nb, 6);
>
> I wonder if some good macro around sizeof could be used here to avoid
> hardcoding magic numbers.
>
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > +
>
> memory leak
OK.
>
> > + opt = (struct grub_dhcpv6_option *)nb->data;
> > + opt->code = grub_cpu_to_be16_compile_time (OPTION_ELAPSED_TIME);
> > + opt->len = grub_cpu_to_be16_compile_time (2);
> > + grub_set_unaligned16 (opt->data, 0);
> > +
> > + err = grub_netbuff_push (nb, sizeof(*duid) + 4);
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > +
>
> memory leak
OK.
>
> > + opt = (struct grub_dhcpv6_option *)nb->data;
> > + opt->code = grub_cpu_to_be16_compile_time (OPTION_CLIENTID);
> > //option_client_id
>
> C++ comments
OK.
>
> > + 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->ifaces->hwaddress.mac,
> > + sizeof (session->ifaces->hwaddress.mac));
> > +
> > + err = grub_netbuff_push (nb, sizeof (*ia_na) + 4);
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > +
>
> memory leak
OK.
>
> > + opt = (struct grub_dhcpv6_option *)nb->data;
> > + opt->code = grub_cpu_to_be16_compile_time (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, 4);
> > + if (err)
> > + {
> > + grub_netbuff_free (nb);
> > + return err;
> > + }
> > +
>
> memory leak
OK.
>
> > + 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->ifaces->address, &multicast);
> > +
> > + err = grub_net_send_ip_packet (session->ifaces, &multicast,
> > + &ll_multicast, nb, GRUB_NET_IP_UDP);
> > + done = 0;
> > + grub_netbuff_free (nb);
> > +
> > + if (err)
> > + return err;
>
> memory leak
OK.
>
> > + }
> > + if (!done)
> > + grub_net_poll_cards (interval, 0);
> > + }
> > +
> > + FOR_DHCPV6_SESSIONS (session)
> > + {
> > + err = grub_error (GRUB_ERR_FILE_NOT_FOUND,
> > + N_("couldn't autoconfigure %s"),
> > + session->ifaces->card->name);
>
> Unless I misunderstand something only last error is printed anyway. May
> be it should push and then print them outside of loop?
OK.
>
> > + grub_dhcpv6_session_remove (session);
> > + grub_free (session);
> > + }
> > +
> > +
> > + return err;
> > +}
> > +
> > +static grub_command_t cmd_getdhcp, cmd_bootp, cmd_bootp6;
> >
> > void
> > grub_bootp_init (void)
> > @@ -575,6 +1454,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 +1464,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..8bb56be 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 && grub_be_to_cpu16 (udph->dst) == 546)
>
> udph->dst == grub_cpu_to_be16_compile_time
OK.
>
> > + {
> > + 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/efi/api.h b/include/grub/efi/api.h
> > index e5dd543..ff77750 100644
> > --- a/include/grub/efi/api.h
> > +++ b/include/grub/efi/api.h
>
> This does not really belong to this patch.
>
> > @@ -1340,14 +1340,68 @@ 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 EFI_PXE_BASE_CODE_MAX_IPCNT 8
> GRUB_EFI_* please
OK.
>
> > +typedef struct {
> > + grub_uint8_t filters;
> > + grub_uint8_t ip_cnt;
> > + grub_uint16_t reserved;
> > + grub_efi_pxe_ip_address_t ip_list[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 EFI_PXE_BASE_CODE_MAX_ARP_ENTRIES 8
> > +#define EFI_PXE_BASE_CODE_MAX_ROUTE_ENTRIES 8
> > +
>
> Same
OK.
>
> > 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[48];
> > + 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[EFI_PXE_BASE_CODE_MAX_ARP_ENTRIES];
> > + grub_uint32_t route_table_entries;
> > + grub_efi_pxe_route_entry_t
> > route_table[EFI_PXE_BASE_CODE_MAX_ROUTE_ENTRIES];
> > } grub_efi_pxe_mode_t;
> >
> > typedef struct grub_efi_pxe
> > 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);
>
[PATCH 2/3] UEFI IPv6 PXE support, Michael Chang, 2015/04/15
[PATCH 3/3] Use UEFI MAC device as default configured by net_bootp6, Michael Chang, 2015/04/15
Re: [RFC] Support DHCPv6 and UEFI IPv6 PXE, Andrei Borzenkov, 2015/04/17