Re: [PATCH 1/3] dhcp: Use -1 as "missing option" length instead of 0

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 25 Nov 2024 12:08:35 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Nov 25, 2024 at 01:04:21AM +0100, Stefano Brivio wrote:
> > We want to add support for option 80 (Rapid Commit, RFC 4039), whose
> > length is 0.
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  dhcp.c | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/dhcp.c b/dhcp.c
> > index a06f143..2fe4a4d 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -36,9 +36,9 @@
> >  /**
> >   * struct opt - DHCP option
> >   * @sent:	Convenience flag, set while filling replies
> > - * @slen:	Length of option defined for server
> > + * @slen:	Length of option defined for server, -1 if not going to be sent
> >   * @s:		Option payload from server
> > - * @clen:	Length of option received from client
> > + * @clen:	Length of option received from client, -1 if not received
> >   * @c:		Option payload from client
> >   */
> >  struct opt {
> > @@ -154,17 +154,17 @@ static int fill(struct msg *m)
> >  	 * option 53 at the beginning of the list.
> >  	 * Put it there explicitly, unless requested via option 55.
> >  	 */
> > -	if (!memchr(opts[55].c, 53, opts[55].clen))
> > +	if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen))
> >  		fill_one(m, 53, &offset);
> >  
> >  	for (i = 0; i < opts[55].clen; i++) {
> >  		o = opts[55].c[i];
> > -		if (opts[o].slen)
> > +		if (opts[o].slen != -1)
> >  			fill_one(m, o, &offset);
> >  	}
> >  
> >  	for (o = 0; o < 255; o++) {
> > -		if (opts[o].slen && !opts[o].sent)
> > +		if (opts[o].slen != -1 && !opts[o].sent)
> >  			fill_one(m, o, &offset);
> >  	}
> >  
> > @@ -264,6 +264,9 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
> >  						 ".\xc0");
> >  		}
> >  	}
> > +
> > +	if (!opts[119].slen)
> > +		opts[119].slen = -1;
> >  }
> >  
> >  /**
> > @@ -313,6 +316,13 @@ int dhcp(const struct ctx *c, const struct pool *p)
> >  
> >  	offset += offsetof(struct msg, o);
> >  
> > +	for (i = 0; i < ARRAY_SIZE(opts); i++) {
> > +		if (!opts[i].slen)
> > +			opts[i].slen = -1;
> > +
> > +		opts[i].clen = -1;
> > +	}  
> 
> Could this move to dhcp_init()?  I think there you wouldn't need test
> and could unconditionally initialize all the lengths to -1 before
> initializing the options we actually use.

No, because dhcp_init() is run only once, and 'opts' at this point
represents the status from the previous run, so:

- we need to unconditionally reset all the 'clen' attributes which were
  set in the previous run

- we need to reset the 'slen' attributes for zero-length options (it's
  just option 80 at the moment) because we need to re-evaluate their
  inclusion. Sure, I could also clean things up at the end of any run,
  but this is more practical and robust

> >  	while (opt_off + 2 < opt_len) {
> >  		const uint8_t *olen, *val;
> >  		uint8_t *type;
> > @@ -334,8 +344,9 @@ int dhcp(const struct ctx *c, const struct pool *p)
> >  	if (opts[53].c[0] == DHCPDISCOVER) {
> >  		info("DHCP: offer to discover");
> >  		opts[53].s[0] = DHCPOFFER;
> > -	} else if (opts[53].c[0] == DHCPREQUEST || !opts[53].clen) {
> > -		info("%s: ack to request", opts[53].clen ? "DHCP" : "BOOTP");
> > +	} else if (opts[53].c[0] == DHCPREQUEST || opts[53].clen <= 0) {
> > +		info("%s: ack to request",
> > +		     (opts[53].clen <= 0) ? "DHCP" : "BOOTP");  
> 
> Should this be <= 0, or < 0?  i.e. Wouldn't even an empty option 53
> indicate we're dealing with DHCP rather than BOOTP?

It should really be <= 0, preserving the existing behaviour, because if
option 53 is empty, we don't know what kind of DHCP message that is. We
know for sure that it's not a valid DHCP message, but it probably is a
valid BOOTP message (with a vendor extension).

This might look like speculation, but there are some half-DHCP
implementations from the 1990s which we can happily handle as BOOTP
clients, but not really as DHCP. After all the fun we had with wattcp32
and mTCP I would say it's not unlikely.

> >  		opts[53].s[0] = DHCPACK;
> >  	} else {
> >  		return -1;
> > @@ -374,6 +385,8 @@ int dhcp(const struct ctx *c, const struct pool *p)
> >  		((struct in_addr *)opts[6].s)[i] = c->ip4.dns[i];
> >  		opts[6].slen += sizeof(uint32_t);
> >  	}
> > +	if (!opts[6].slen)
> > +		opts[6].slen = -1;
> >  
> >  	if (!c->no_dhcp_dns_search)
> >  		opt_set_dns_search(c, sizeof(m->o));  

-- 
Stefano