On Fri, Mar 08, 2019 at 11:51:27AM +0000, Andre Przywara wrote:
> On Fri, 8 Mar 2019 12:34:18 +0100
> Daniel Kiper <daniel.ki...@oracle.com> wrote:
>
> > On Thu, Mar 07, 2019 at 03:14:09PM +0000, Andre Przywara wrote:
> > > From: Andrei Borzenkov <arvidj...@gmail.com>
> > >
> > > DHCP specifies a special dummy option OVERLOAD, to allow DHCP options to
> > > spill over into the (legacy) BOOTFILE and SNAME fields.
> > >
> > > Parse and handle this option properly.
> > >
> > > Signed-off-by: Andre Przywara <andre.przyw...@arm.com>
> >
> > In general Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com> but one
> > nitpick below...
> >
> > > ---
> > > grub-core/net/bootp.c | 59 ++++++++++++++++++++++++++++++++++++++++++-
> > > include/grub/net.h | 1 +
> > > 2 files changed, 59 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> > > index 8d6763689..860d9c80d 100644
> > > --- a/grub-core/net/bootp.c
> > > +++ b/grub-core/net/bootp.c
> > > @@ -25,11 +25,19 @@
> > > #include <grub/net/udp.h>
> > > #include <grub/datetime.h>
> > >
> > > +enum
> > > +{
> > > + GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
> > > + GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
> > > +};
> > > +
> > > static const void *
> > > find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t
> > > size,
> > > grub_uint8_t opt_code, grub_uint8_t *opt_len)
> > > {
> > > const grub_uint8_t *ptr;
> > > + grub_uint8_t overload = 0;
> > > + int end = 0;
> > > grub_size_t i;
> > >
> > > if (opt_len)
> > > @@ -54,6 +62,7 @@ find_dhcp_option (const struct grub_net_bootp_packet
> > > *bp, grub_size_t size,
> > > size -= sizeof (*bp);
> > > i = sizeof (grub_uint32_t);
> > >
> > > +again:
> > > while (i < size)
> > > {
> > > grub_uint8_t tagtype;
> > > @@ -67,7 +76,10 @@ find_dhcp_option (const struct grub_net_bootp_packet
> > > *bp, grub_size_t size,
> > >
> > > /* End tag. */
> > > if (tagtype == GRUB_NET_BOOTP_END)
> > > - break;
> > > + {
> > > + end = 1;
> > > + break;
> > > + }
> > >
> > > if (i >= size)
> > > return NULL;
> > > @@ -84,9 +96,54 @@ find_dhcp_option (const struct grub_net_bootp_packet
> > > *bp, grub_size_t size,
> > > return &ptr[i];
> > > }
> > >
> > > + if (tagtype == GRUB_NET_DHCP_OVERLOAD && taglength == 1)
> > > + overload = ptr[i];
> > > +
> > > i += taglength;
> > > }
> > >
> > > + if (!end)
> > > + return NULL;
> > > +
> > > + /* RFC2131, 4.1, 23ff:
> > > + * If the options in a DHCP message extend into the 'sname' and 'file'
> > > + * fields, the 'option overload' option MUST appear in the 'options'
> > > + * field, with value 1, 2 or 3, as specified in RFC 1533. If the
> > > + * 'option overload' option is present in the 'options' field, the
> > > + * options in the 'options' field MUST be terminated by an 'end'
> > > option,
> > > + * and MAY contain one or more 'pad' options to fill the options field.
> > > + * The options in the 'sname' and 'file' fields (if in use as indicated
> > > + * by the 'options overload' option) MUST begin with the first octet of
> > > + * the field, MUST be terminated by an 'end' option, and MUST be
> > > + * followed by 'pad' options to fill the remainder of the field. Any
> > > + * individual option in the 'options', 'sname' and 'file' fields MUST
> > > be
> > > + * entirely contained in that field. The options in the 'options'
> > > field
> > > + * MUST be interpreted first, so that any 'option overload' options may
> > > + * be interpreted. The 'file' field MUST be interpreted next (if the
> > > + * 'option overload' option indicates that the 'file' field contains
> > > + * DHCP options), followed by the 'sname' field.
> > > + *
> > > + * FIXME: We do not explicitly check for trailing 'pad' options here.
> > > + */
> > > + end = 0;
> >
> > It seems to me that this line is not needed... I can drop it before
> > committing the patch if you are OK with it.
>
> I was eyeballing this line as well, but I think it *is* needed:
> It resets end to 0, before possibly jumping back to the again label, inside
> the if conditions below.
> Does that make sense?
Ahhh... Right, I have missed that. So,
Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com>
Daniel
> Cheers,
> Andre.
>
> >
> > > + if (overload & GRUB_DHCP_OPT_OVERLOAD_FILE)
> > > + {
> > > + overload &= ~GRUB_DHCP_OPT_OVERLOAD_FILE;
> > > + ptr = (grub_uint8_t *) &bp->boot_file[0];
> > > + size = sizeof (bp->boot_file);
> > > + i = 0;
> > > + goto again;
> > > + }
> > > +
> > > + if (overload & GRUB_DHCP_OPT_OVERLOAD_SNAME)
> > > + {
> > > + overload &= ~GRUB_DHCP_OPT_OVERLOAD_SNAME;
> > > + ptr = (grub_uint8_t *) &bp->server_name[0];
> > > + size = sizeof (bp->server_name);
> > > + i = 0;
> > > + goto again;
> > > + }
> > > +
> > > return NULL;
> > > }
> >
> > Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
