Re: [PATCH 1/3] Added net_bootp6 command

[Andrei Borzenkov]

В Mon, 20 Apr 2015 11:09:10 +0800
Michael Chang <address@hidden> пишет:

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