Re: [PATCH net-next v4 13/17] net: pktgen: fix access outside of user given buffer in pktgen_if_write()

[Simon Horman]

On Wed, Feb 05, 2025 at 02:11:49PM +0100, Peter Seiderer wrote:
> Honour the user given buffer size for the hex32_arg(), num_arg(),
> strn_len(), get_imix_entries() and get_labels() calls (otherwise they will
> access memory outside of the user given buffer).
> 
> Signed-off-by: Peter Seiderer <ps.report@xxxxxxx>
> ---
> Changes v3 -> v4:
>   - replace C99 comment (suggested by Paolo Abeni)
>   - drop available characters check in strn_len() (suggested by Paolo Abeni)
>   - factored out patch 'net: pktgen: align some variable declarations to the
>     most common pattern' (suggested by Paolo Abeni)
>   - factored out patch 'net: pktgen: remove extra tmp variable (re-use len
>     instead)' (suggested by Paolo Abeni)
>   - factored out patch 'net: pktgen: remove some superfluous variable
>     initializing' (suggested by Paolo Abeni)
>   - factored out patch 'net: pktgen: fix mpls maximum labels list parsing'
>     (suggested by Paolo Abeni)
>   - factored out 'net: pktgen: hex32_arg/num_arg error out in case no
>     characters are available' (suggested by Paolo Abeni)
>   - factored out 'net: pktgen: num_arg error out in case no valid character
>     is parsed' (suggested by Paolo Abeni)
> 
> Changes v2 -> v3:
>   - no changes
> 
> Changes v1 -> v2:
>   - additional fix get_imix_entries() and get_labels()

Reviewed-by: Simon Horman <horms@xxxxxxxxxx>

> ---
>  net/core/pktgen.c | 176 ++++++++++++++++++++++++++++++----------------

...

> @@ -1015,7 +1025,8 @@ static ssize_t pktgen_if_write(struct file *file,
>  	}
>  
>  	if (!strcmp(name, "min_pkt_size")) {
> -		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
> +		max = min(DEC_10_DIGITS, count - i);
> +		len = num_arg(&user_buffer[i], max, &value);
>  		if (len < 0)
>  			return len;
>  

As an aside:

The code immediately following the hunk above is as follows.
And this block it is representative of many (all?) of the code
modified by the hunks that make up the remainder of this patch.

My observation is that i is incremented but never used again -
the function subsequently returns at the end of the if condition.

So perhaps it would be a nice, as a follow-up, to clean this up
be removing the increment of i from this and similar blocks.


		if (len < 0)
			return len;

		i += len;
		if (value < 14 + 20 + 8)
			value = 14 + 20 + 8;
		if (value != pkt_dev->min_pkt_size) {
			pkt_dev->min_pkt_size = value;
			pkt_dev->cur_pkt_size = value;
		}
		sprintf(pg_result, "OK: min_pkt_size=%d",
			pkt_dev->min_pkt_size);
		return count;
	}

...