| Message ID | 20250205131153.476278-1-ps.report@gmx.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Some pktgen fixes/improvments | expand |
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.
Hello Simon, On Thu, 6 Feb 2025 13:51:54 +0000, Simon Horman <horms@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.
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) Changes v2 -> v3: - new patch: 'net: pktgen: fix ctrl interface command parsing' - new patch: 'net: pktgen: fix mpls reset parsing' - tools/testing/selftests/net/proc_net_pktgen.c: - fix typo in change description ('v1 -> v1' and tyop) - rename some vars to better match usage add_loopback_0 -> thr_cmd_add_loopback_0 rm_loopback_0 -> thr_cmd_rm_loopback_0 wrong_ctrl_cmd -> wrong_thr_cmd legacy_ctrl_cmd -> legacy_thr_cmd ctrl_fd -> thr_fd - add ctrl interface tests Changes v1 -> v2: - new patch: 'net: pktgen: fix hex32_arg parsing for short reads' - new patch: 'net: pktgen: fix 'rate 0' error handling (return -EINVAL)' - new patch: 'net: pktgen: fix 'ratep 0' error handling (return -EINVAL)' - net/core/pktgen.c: additional fix get_imix_entries() and get_labels() - tools/testing/selftests/net/proc_net_pktgen.c: - fix tyop not vs. nod (suggested by Jakub Kicinski) - fix misaligned line (suggested by Jakub Kicinski) - enable fomerly commented out CONFIG_XFRM dependent test (command spi), as CONFIG_XFRM is enabled via tools/testing/selftests/net/config CONFIG_XFRM_INTERFACE/CONFIG_XFRM_USER (suggestex by Jakub Kicinski) - add CONFIG_NET_PKTGEN=m to tools/testing/selftests/net/config (suggested by Jakub Kicinski) - add modprobe pktgen to FIXTURE_SETUP() (suggested by Jakub Kicinski) - fix some checkpatch warnings (Missing a blank line after declarations) - shrink line length by re-naming some variables (command -> cmd, device -> dev) - add 'rate 0' testcase - add 'ratep 0' testcase [1] https://lore.kernel.org/netdev/20241006221221.3744995-1-artem.chernyshev@red-soft.ru/ [2] https://lore.kernel.org/netdev/20250109083039.14004-1-pchelkin@ispras.ru/ [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76201b5979768500bca362871db66d77cb4c225e Peter Seiderer (17): net: pktgen: replace ENOTSUPP with EOPNOTSUPP net: pktgen: enable 'param=value' parsing net: pktgen: fix hex32_arg parsing for short reads net: pktgen: fix 'rate 0' error handling (return -EINVAL) net: pktgen: fix 'ratep 0' error handling (return -EINVAL) net: pktgen: fix ctrl interface command parsing net: pktgen: fix access outside of user given buffer in pktgen_thread_write() net: pktgen: use defines for the various dec/hex number parsing digits lengths net: pktgen: align some variable declarations to the most common pattern net: pktgen: remove extra tmp variable (re-use len instead) net: pktgen: remove some superfluous variable initializing net: pktgen: fix mpls maximum labels list parsing net: pktgen: fix access outside of user given buffer in pktgen_if_write() net: pktgen: hex32_arg/num_arg error out in case no characters are available net: pktgen: num_arg error out in case no valid character is parsed net: pktgen: fix mpls reset parsing selftest: net: add proc_net_pktgen net/core/pktgen.c | 268 +++++--- tools/testing/selftests/net/Makefile | 1 + tools/testing/selftests/net/config | 1 + tools/testing/selftests/net/proc_net_pktgen.c | 650 ++++++++++++++++++ 4 files changed, 828 insertions(+), 92 deletions(-) create mode 100644 tools/testing/selftests/net/proc_net_pktgen.c