linux-kernel - Re: [PATCH net-next v4 00/17] Some pktgen fixes/improvments

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: <20250206135154.GB641369@kernel.org>
Date: Thu, 6 Feb 2025 13:51:54 +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 00/17] Some pktgen fixes/improvments

On Wed, Feb 05, 2025 at 02:11:36PM +0100, Peter Seiderer wrote:
> hile taking a look at '[PATCH net] pktgen: Avoid out-of-range in
> get_imix_entries' ([1]) and '[PATCH net v2] pktgen: Avoid out-of-bounds access
> in get_imix_entries' ([2], [3]) and doing some tests and code review I
> detected that the /proc/net/pktgen/... parsing logic does not honour the
> user given buffer bounds (resulting in out-of-bounds access).
> 
> This can be observed e.g. by the following simple test (sometimes the
> old/'longer' previous value is re-read from the buffer):
> 
>         $ echo add_device lo@0 > /proc/net/pktgen/kpktgend_0
> 
>         $ echo "min_pkt_size 12345" > /proc/net/pktgen/lo\@0 && grep min_pkt_size /proc/net/pktgen/lo\@0
> Params: count 1000  min_pkt_size: 12345  max_pkt_size: 0
> Result: OK: min_pkt_size=12345
> 
>         $ echo -n "min_pkt_size 123" > /proc/net/pktgen/lo\@0 && grep min_pkt_size /proc/net/pktgen/lo\@0
> Params: count 1000  min_pkt_size: 12345  max_pkt_size: 0
> Result: OK: min_pkt_size=12345
> 
>         $ echo "min_pkt_size 123" > /proc/net/pktgen/lo\@0 && grep min_pkt_size /proc/net/pktgen/lo\@0
> Params: count 1000  min_pkt_size: 123  max_pkt_size: 0
> Result: OK: min_pkt_size=123
> 
> So fix the out-of-bounds access (and some minor findings) and add a simple
> proc_net_pktgen selftest...
> 
> Regards,
> Peter
> 
> Changes v3 -> v4:
>  - add rev-by Simon Horman
>  - new patch 'net: pktgen: use defines for the various dec/hex number parsing
>    digits lengths' (suggested by Simon Horman)
>  - 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)

Hi Peter,

Thanks for splitting up the patchset some more, I for one find it much
easier to review them in this form.

That said, we are now over the preferred maximum of 15 patches in a series.
Perhaps the maintainers are ok with that, but I'd like to suggest breaking
the series in two: The first 7 patches seem to be somewhat stable, from a
review perspective, and could be posted as "part i"; And then the remaining
patches could be posted as "part ii" once "part i" has been accepted.

As for the selftests (the last patch of the series). A version,
trimmed down as appropriate, could be included in "part i", with a
follow-up in "part ii". Or the cover note for "part i" could state that the
selftests have been deferred to "part ii".

Perhaps the maintainers have other ideas, if so hopefully they will comment
here.