Re: [PATCH 1/3] Added net_bootp6 command

[Andrei Borzenkov]

В Fri, 17 Apr 2015 13:04:27 +0800
Michael Chang <address@hidden> пишет:

> > 
> > > +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.