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

[Laurent Vivier <lvivier@redhat.com>]

On 5/26/26 14:31, 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.

This patch not only adds the overload, it also copies the options from the command line 
(custom_opts) to the actual opts.

> 
> Link: https://bugs.passt.top/show_bug.cgi?id=192
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> ---
> v2:
>    - Added #define DHCP_OVERLOAD_FILE and #define DHCP_OVERLOAD_SNAME constants
>    - Added comment documenting space reservation: /* Reserve 3 bytes for option 52 */
>    - Fixed DNS search length: sizeof(m->o) only, not combined with file+sname
>    - Removed dhcp_boot references — reply.file copy now reads from opts[67]
>    - Used DHCP_OVERLOAD_FILE constant in reply.file guard
> ---
>   dhcp.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 75 insertions(+), 9 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index e126063..a49a05a 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -306,14 +306,59 @@ static bool fill_one(uint8_t *buf, size_t size, int o, int *offset)
>   	return false;
>   }
>   
> +#define DHCP_OVERLOAD_FILE	1
> +#define DHCP_OVERLOAD_SNAME	2
> +
> +/**
> + * 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,
> + *         DHCP_OVERLOAD_FILE for file, DHCP_OVERLOAD_SNAME for sname,
> + *         or both OR'd together
> + */
> +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 |= DHCP_OVERLOAD_FILE;
> +	}
> +
> +	if (sname_off) {
> +		m->sname[sname_off] = 255;
> +		overload |= DHCP_OVERLOAD_SNAME;
> +	}
> +
> +	return overload;
> +}
> +
>   /**
> - * fill() - Fill options in message
> + * 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)
>   {
> +	/* Reserve 3 bytes for option 52 (overload) if needed */
> +	size_t size = OPT_MAX - 3;
>   	int i, o, offset = 0;
>   
>   	for (o = 0; o < 255; o++)
> @@ -324,20 +369,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, size, 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, size, 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, size, o, &offset);
> +	}
> +
> +	*overload = fill_overflow(m);
> +
> +	if (*overload) {
> +		m->o[offset++] = 52;
> +		m->o[offset++] = 1;
> +		m->o[offset++] = *overload;
>   	}
>   
>   	m->o[offset++] = 255;
> @@ -462,6 +512,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);
> @@ -613,7 +664,22 @@ 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() manages option 119 but we have also it in custom_opts (incorrectly 
set as it doesn't manage compression). I think we overwrite it with incorrect values.

Moreover I think this function is broken, because max_len is computed using opts[].slen, 
but opts[].slen is updated below (and max_len should be OPT_MAX - 3, not sizeof(m->o))

>   
> -	dlen = offsetof(struct msg, o) + fill(&reply);
> +	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);
> +
> +	/* Copy boot file name into the file field for legacy PXE clients,
> +	 * unless the file field is already used for option overload.
> +	 */
> +	if (!(overload & DHCP_OVERLOAD_FILE) &&
> +	    opts[67].slen > 0 && (size_t)opts[67].slen < sizeof(reply.file))
> +		memcpy(&reply.file, opts[67].s, opts[67].slen + 1);

No need for "&" in "&reply.size".
the "+ 1" is wrong as reply.file has been cleared by a memset() (whereas opts[67].s[slen] 
can be not NUL).

>   
>   	if (m->flags & FLAG_BROADCAST)
>   		dst = in4addr_broadcast;

Thanks,
Laurent