Re: [PATCH v2 2/4] dhcp: Make option parsing more robust, explicitly handle options 0 and 255

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

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

On Thu, Jul 16, 2026 at 09:22:20AM +0200, Stefano Brivio wrote:
> The initial option-scanning loop in dhcp(), so far, ignored options 0
> (Pad Option, RFC 2132, Section 3.1) and 255 (End Option, RFC 2132,
> Section 3.2).
> 
> As a result:
> 
> - if we ever encountered option 0 in the middle of option fields
>   (never seen in practice), we would potentially terminate the loop
>   too early, before scanning remaining options
> 
> - a malformed message with an option 255 followed by a length byte
>   would (reliably) cause us to terminate as we would exceed the
>   allocated size for the 'opts' array, which is detected as buffer
>   overflow by the FORTIFY_SOURCE mechanism
> 
> The latter was reported as potential vulnerability by AISLE, but it's
> not actually a vulnerability as we always terminate without carrying
> on further handling, and in our security model the guest is able to
> sabotage its own connectivity in any case (for example, a malformed
> frame from the hypervisor would cause us to reset the connection, or
> entirely flooding the flow table would cause inbound connectivity to
> stop working, etc.).
> 
> The reported behaviour, however, is indeed a defect, as it affects
> the functional robustness to a hypothetical issue in a DHCP client,
> and that's something we definitely want to fix.
> 
> Make the option parsing loop more robust by:
> 
> - resizing 'opts' from 255 to 256 elements: there's no particular
>   reason to try to save a tiny bit of memory (which shouldn't even
>   be allocated in practice) instead of being defensive about it
> 
> - explicitly handle options 0 (skip one byte, continue) and 255 (stop
>   processing options) in the option-scanning loop
> 
> - scanning the last two bytes of options as well and using
>   iov_tail_size(data) directly as loop condition, instead of a rather
>   inconsistent usage of opt_len
> 
> This bug was found and an initial version of the patch was written by
> the AISLE AI security scanning tool (https://aisle.com/platform).
> 
> Reported-by: AISLE
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> v2:
>   - Handle one-byte options before IOV_REMOVE_HEADER() for the length byte
>   - Use iov_tail_size(data) as loop condition instead of mixing things up
>     with opt_len
> 
>  dhcp.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index 1ff8cba..c3c7422 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -49,7 +49,7 @@ struct opt {
>  	uint8_t c[255];
>  };
>  
> -static struct opt opts[255];
> +static struct opt opts[256];
>  
>  #define DHCPDISCOVER	1
>  #define DHCPOFFER	2
> @@ -363,25 +363,29 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  	for (i = 0; i < ARRAY_SIZE(opts); i++)
>  		opts[i].clen = -1;
>  
> -	opt_len = iov_tail_size(data);
> -	while (opt_len >= 2) {
> +	while ((opt_len = iov_tail_size(data))) {
>  		uint8_t olen_storage, type_storage;
>  		const uint8_t *olen;
>  		uint8_t *type;
>  
> -		type = IOV_REMOVE_HEADER(data, type_storage);
> -		olen = IOV_REMOVE_HEADER(data, olen_storage);
> -		if (!type || !olen)
> +		if (!(type = IOV_REMOVE_HEADER(data, type_storage)))
>  			return -1;
>  
> -		opt_len = iov_tail_size(data);
> -		if (opt_len < *olen)
> +		if (*type == 255)
> +			break;
> +
> +		if (*type == 0) /* Pad Option (RFC 2132, 3.1): one byte */
> +			continue;
> +
> +		if (!(olen = IOV_REMOVE_HEADER(data, olen_storage)))
> +			return -1;
> +
> +		if (opt_len - 2 < *olen)
>  			return -1;
>  
>  		iov_to_buf(&data->iov[0], data->cnt, data->off, &opts[*type].c, *olen);
>  		opts[*type].clen = *olen;
>  		iov_drop_header(data, *olen);
> -		opt_len -= *olen;
>  	}
>  
>  	opts[80].slen = -1;
> -- 
> 2.43.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 --]