Re: [PATCH 5/6] dhcp: Add option overload

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5275 bytes --]

On Mon, May 18, 2026 at 06:50:01PM +0530, Anshu Kumari wrote:
> A user can enter lots of options in command-line which may not fit in
> existing buffer, So when the options field is full, overflow remaining
> DHCP options into the file and sname fields per RFC 2132 option 52.
> 
> Also, when the file field is not used for overload, copy the boot
> file URL there directly for legacy PXE clients.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=192
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> ---
>  dhcp.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 78 insertions(+), 10 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index a966c34..fde5d57 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -386,13 +386,53 @@ static bool fill_one(uint8_t *buf, size_t cap, int o, int *offset)
>  }
>  
>  /**
> - * fill() - Fill options in message
> + * fill_overflow() - Fill remaining options into file and sname fields
> + * @m:		Message whose file/sname fields may be used for overflow
> + *
> + * Return: option 52 overload value: 0 if no overflow, 1 for file,
> + *         2 for sname, 3 for both
> + */
> +static int fill_overflow(struct msg *m)
> +{
> +	int file_off = 0, sname_off = 0, overload = 0;
> +	int o;
> +
> +	for (o = 0; o < 255; o++) {
> +		if (opts[o].slen == -1 || opts[o].sent)
> +			continue;
> +		fill_one(m->file, sizeof(m->file) - 1, o, &file_off);
> +	}
> +
> +	for (o = 0; o < 255; o++) {
> +		if (opts[o].slen == -1 || opts[o].sent)
> +			continue;
> +		if (fill_one(m->sname, sizeof(m->sname) - 1, o, &sname_off))
> +			debug("DHCP: skipping option %i (overload full)", o);
> +	}
> +
> +	if (file_off) {
> +		m->file[file_off] = 255;
> +		overload |= 1;

Some #defined constants for the overload bits would probably be a good idea.

> +	}
> +
> +	if (sname_off) {
> +		m->sname[sname_off] = 255;
> +		overload |= 2;
> +	}
> +
> +	return overload;
> +}
> +
> +/**
> + * fill() - Fill options in message, with overload into file/sname if needed
>   * @m:		Message to fill
> + * @overload:	Set to option 52 value (0 if none, 1/2/3 per RFC 2132)
>   *
>   * Return: current size of options field
>   */
> -static int fill(struct msg *m)
> +static int fill(struct msg *m, int *overload)
>  {
> +	size_t cap = OPT_MAX - 3;
>  	int i, o, offset = 0;
>  
>  	for (o = 0; o < 255; o++)
> @@ -403,20 +443,25 @@ 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))
> -		if (fill_one(m->o, OPT_MAX, 53, &offset))
> -			 debug("DHCP: skipping option 53");
> +		fill_one(m->o, cap, 53, &offset);
>  
>  	for (i = 0; i < opts[55].clen; i++) {
>  		o = opts[55].c[i];
>  		if (opts[o].slen != -1)
> -			if (fill_one(m->o, OPT_MAX, o, &offset))
> -				debug("DHCP: skipping option %i", o);
> +			fill_one(m->o, cap, o, &offset);
>  	}
>  
>  	for (o = 0; o < 255; o++) {
>  		if (opts[o].slen != -1 && !opts[o].sent)
> -			if (fill_one(m->o, OPT_MAX, o, &offset))
> -				debug("DHCP: skipping option %i", o);
> +			fill_one(m->o, cap, o, &offset);
> +	}
> +
> +	*overload = fill_overflow(m);
> +
> +	if (*overload) {
> +		m->o[offset++] = 52;
> +		m->o[offset++] = 1;
> +		m->o[offset++] = *overload;

If we reach this path then we've near-filled the normal option area.
What guarantees we'll have space for option 52 itself?

>  	}
>  
>  	m->o[offset++] = 255;
> @@ -541,6 +586,7 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  	struct msg const *m;
>  	struct msg reply;
>  	unsigned int i;
> +	int overload;
>  
>  	eh = IOV_REMOVE_HEADER(data, eh_storage);
>  	iph = IOV_PEEK_HEADER(data, iph_storage);
> @@ -690,9 +736,31 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  	}
>  
>  	if (!c->no_dhcp_dns_search)
> -		opt_set_dns_search(c, sizeof(m->o));
> +		opt_set_dns_search(c, sizeof(m->o) + sizeof(m->file)
> +					       + sizeof(m->sname));

Does passing the combined length here actually make sense?  IIUC each
single option still needs to fit within one of the buffer areas.

> +
> +	if (c->dhcp_boot[0]) {
> +		size_t boot_len = strlen(c->dhcp_boot);
> +
> +		if (boot_len <= sizeof(opts[67].s)) {
> +			opts[67].slen = boot_len;
> +			memcpy(opts[67].s, c->dhcp_boot, boot_len);
> +		}
> +	}
> +
> +	for (i = 0; i < (unsigned int)c->custom_opts_count; i++) {
> +		uint8_t code = c->custom_opts[i].code;
> +
> +		opts[code].slen = c->custom_opts[i].len;
> +		memcpy(opts[code].s, c->custom_opts[i].val,
> +		       c->custom_opts[i].len);
> +	}
> +
> +	dlen = offsetof(struct msg, o) + fill(&reply, &overload);
>  
> -	dlen = offsetof(struct msg, o) + fill(&reply);
> +	if (!(overload & 1) &&
> +	    c->dhcp_boot[0] && strlen(c->dhcp_boot) < sizeof(reply.file))
> +		memcpy(&reply.file, c->dhcp_boot, strlen(c->dhcp_boot) + 1);
>  
>  	if (m->flags & FLAG_BROADCAST)
>  		dst = in4addr_broadcast;
> -- 
> 2.54.0
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]