Hello Simon, On Thu, 6 Feb 2025 13:51:54 +0000, Simon Horman <ho...@kernel.org> wrote:
> 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. Definitely! > > 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. Yes, the patch set evolved a little bit over the 'just fix some little strange behavior'..., splitting it up will work for me for sure..., will do on next patch set iteration... Regards, Peter > > 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.
