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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250206160103.GV554665@kernel.org>
Date: Thu, 6 Feb 2025 16:01:03 +0000
From: Simon Horman <horms@...nel.org>
To: Peter Seiderer <ps.report@....net>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-kselftest@...r.kernel.org,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH net-next v4 13/17] net: pktgen: fix access outside of
 user given buffer in pktgen_if_write()

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@....net>
> ---
> 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@...nel.org>

> ---
>  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;
	}

...