Re: [PATCH v6 6/6] dhcp: Add RFC 3396 option splitting for concatenation-requiring options

[Laurent Vivier <lvivier@redhat.com>]

On 8/24/26 15:44, Anshu Kumari wrote:
> Implement option splitting per RFC 3396 for options that may exceed
> 255 bytes.  A new concat_req[] lookup table marks options requiring
> concatenation (currently option 81, Client FQDN per RFC 4702).
> 
> The opts[].s buffer is resized from 255 to 497 bytes
> (OPT_CONCAT_MAX) to hold the maximum data that can be split
> across the options field, file field, and sname field.
> 
> When a concatenation-requiring option does not fit as a single
> option in any field, fill() calls fill_split() to split it across
> fields in RFC 3396 order: options field first, then file, then
> sname.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=192
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> ---
> v6:
> - Merged v5 patches 6/7 and 7/7 into a single patch.
> - Replaced DHCP_OPT_STR_CONCAT enum value and is_concat_opt()
>    helper with a concat_req[] boolean lookup table.
> - Used MIN() macro instead of ternary for chunk size.
> - Fixed space calculation to account for 2-byte code+length
>    overhead per chunk.
> 
> v5:
>    - New patch: implement option splitting per RFC 3396 for options exceeding 255 bytes
>    - Add DHCP_OPT_STR_CONCAT type, is_concat_opt(), fill_split() helpers
>    - Resize opts[].s from 255 to OPT_CONCAT_MAX (497) bytes
>    - Add /* fallthrough */ between DHCP_OPT_STR and DHCP_OPT_STR_CONCAT case
> ---
>   dhcp.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index 43ce133..eda04d1 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -35,6 +35,11 @@
>   #include "dhcp.h"
>   #include "parse.h"
>   
> +/* RFC 3396: maximum option data that can be split across options field,
> + * file field, and sname field (minus code+length overhead per portion).
> + */
> +#define OPT_CONCAT_MAX	497
> +
>   /**
>    * enum opt_state - DHCP option state
>    * @OPT_UNSET:		Option not configured
> @@ -59,7 +64,7 @@ enum opt_state {
>   struct opt {
>   	int sent;
>   	int slen;
> -	uint8_t s[255];
> +	uint8_t s[OPT_CONCAT_MAX];
>   	int clen;
>   	uint8_t c[255];
>   	enum opt_state state;
> @@ -215,6 +220,13 @@ static const enum dhcp_opt_type dhcp_opt_types[] = {
>   	[252] = DHCP_OPT_STR,		/* WPAD URL */
>   };
>   
> +/**
> + * concat_req - Options requiring RFC 3396 concatenation, indexed by code
> + */
> +static const bool concat_req[256] = {
> +	[81] = true,	/* Client FQDN (RFC 4702, Section 2) */
> +};
> +
>   /**
>    * dhcp_opt_parse() - Parse a DHCP option value
>    * @code:	DHCP option code
> @@ -309,6 +321,9 @@ static int dhcp_opt_parse(uint8_t code, const char *str,
>   	case DHCP_OPT_STR:
>   		slen = strlen(str);
>   
> +		if (!concat_req[code] && slen > 255)
> +			return -1;
> +
>   		if (slen >= buf_len)
>   			return -1;
>   
> @@ -442,6 +457,40 @@ enum dhcp_overload {
>   	DHCP_OVERLOAD_SNAME = 2,
>   };
>   
> +/**
> + * fill_split() - Write a split portion of an option into a buffer
> + * @buf:	Buffer to write into
> + * @size:	Usable size of @buf
> + * @o:		Option number (code)
> + * @offset:	Current offset within @buf, updated on write
> + * @data:	Pointer to remaining option data to write
> + * @remaining:	Bytes of option data still to write
> + *
> + * Return: number of data bytes written (excluding code+length header)
> + */
> +static size_t fill_split(uint8_t *buf, size_t size, int o, int *offset,
> +			 const uint8_t *data, size_t remaining)
> +{
> +	size_t avail, chunk;
> +
> +	if (*offset + 2 >= (int)size)
> +		return 0;
> +
> +	avail = size - *offset - 2;
> +	chunk = MIN(remaining, avail);
> +	if (!chunk)
> +		return 0;
> +
> +	buf[*offset] = o;
> +	buf[*offset + 1] = chunk;

buf[] is uint8_t, but chunk can be > 255:
avail can be OPT_MAX - 3 - 2 (302)
remainining is, on first call opts[81].slen (that is opts[81].slen = strlen(c->fqdn) +
   5, that can be PASST_MAXDNAME - 1 + 5 = 258).

So to avoid this overflow (or other in future options) you should use chunk = 
MIN(MIN(remaining, avail), 255)
> +	*offset += 2;
> +
> +	memcpy(buf + *offset, data, chunk);
> +	*offset += chunk;
> +
> +	return chunk;
> +}
> +
>   /**
>    * fill() - Fill options in message, with overload into file/sname if needed
>    * @m:			Message to fill
> @@ -495,6 +544,54 @@ static int fill(struct msg *m, enum dhcp_overload *overload, bool has_bootfile)
>   		}
>   	}
>   
> +	/* RFC 3396: split concatenation-requiring options that didn't fit
> +	 * as a single option.  Split order: options, file, sname.
> +	 */
> +	for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
> +		size_t file_cap, sname_cap, total, written;
> +
> +		if (opts[o].state == OPT_UNSET || opts[o].sent ||
> +		    !concat_req[o])
> +			continue;
> +
> +		sname_cap = sizeof(m->sname) - 1 > (size_t)sname_off ?
> +			    sizeof(m->sname) - 1 - sname_off : 0;
> +
> +		if (has_bootfile || sizeof(m->file) - 1 <= (size_t)file_off)
> +			file_cap = 0;
> +		else
> +			file_cap = sizeof(m->file) - 1 - file_off;
> +
> +		total = (size > (size_t)offset ? size - offset - 2 : 0)

here also 'size - offset - 2' must be capped by 255 otherwise total available space is not 
correctly evaluated.

> +			+ (file_cap > 2 ? file_cap - 2 : 0)
> +			+ (sname_cap > 2 ? sname_cap - 2 : 0);
> +
> +		if (total < (size_t)opts[o].slen) {
> +			debug("DHCP: skipping option %i (no space to split)",
> +			      o);
> +			continue;
> +		}
> +
> +		written = 0;
> +		written += fill_split(m->o, size, o, &offset,
> +				      opts[o].s, opts[o].slen);
> +		if (written < (size_t)opts[o].slen && !has_bootfile)
> +			written += fill_split(m->file,
> +					      sizeof(m->file) - 1, o,
> +					      &file_off,
> +					      opts[o].s + written,
> +					      opts[o].slen - written);
> +		if (written < (size_t)opts[o].slen)
> +			written += fill_split(m->sname,
> +					      sizeof(m->sname) - 1, o,
> +					      &sname_off,
> +					      opts[o].s + written,
> +					      opts[o].slen - written);
> +
> +		if (written >= (size_t)opts[o].slen)
> +			opts[o].sent = 1;
> +	}
> +
>   	/* Report any options that could not be sent */
>   	for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
>   		if (opts[o].state != OPT_UNSET && !opts[o].sent)