Re: [PATCH v8] dhcp, dhcpv6: Add hostname and client fqdn ops

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 14 Jan 2025 15:01:33 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:

> On Sat, Jan 11, 2025 at 12:53 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Fri, 10 Jan 2025 11:26:26 +0100
> > Enrique Llorente <ellorent@redhat.com> wrote:
> >  
> > > @@ -162,17 +180,20 @@ static int fill(struct msg *m)
> > >        * Put it there explicitly, unless requested via option 55.
> > >        */
> > >       if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen))
> > > -             fill_one(m, 53, &offset);
> > > +             offset = fill_one(m, 53, offset);  
> >
> > Now suppose that somebody adds a "long" option before this block, or...
> >  
> > >
> > >       for (i = 0; i < opts[55].clen; i++) {
> > >               o = opts[55].c[i];
> > >               if (opts[o].slen != -1)
> > > -                     fill_one(m, o, &offset);
> > > +                     offset = fill_one(m, o, offset);
> > >       }  
> >
> > before this one, and fill_one() returns -1. Then...
> >  
> > >       for (o = 0; o < 255; o++) {
> > > -             if (opts[o].slen != -1 && !opts[o].sent)
> > > -                     fill_one(m, o, &offset);
> > > +             if (opts[o].slen != -1 && !opts[o].sent) {
> > > +                     offset = fill_one(m, o, offset);  
> >
> > you use offset as -1 here, and boom. If not, see directly below).
> >  
> > > +                     if (offset == -1)  
> >
> > Or maybe the domain name is actually too long, and offset is -1 here...
> >  
> > > +                             debug("DHCP: skipping option %i", o);
> > > +             }
> > >       }
> > >
> > >       m->o[offset++] = 255;  
> >
> > ...and boom. This sets the last byte of "magic" (depending on the
> > architecture) to 0xff, making the whole message invalid.
> 
> well, I was expecting the only problematic options being at > 55
> (didn't check the standard)
> but I agree that we have to be consistent on handling the fill_in result.

Sure, that's the (reasonable) expectation, but note that you would have
the same problem even with a domain name that's too long for fill_one().

-- 
Stefano