[net-next,v5,0/8] Some pktgen fixes/improvments (part II)

Message

Peter Seiderer Feb. 13, 2025, 11:19 a.m. UTC

While 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...

Patch set splited into part I

- 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()

And part II (this one):

- net: pktgen: use defines for the various dec/hex number parsing digits lengths
- net: pktgen: fix mix of int/long
- 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: fix mpls reset parsing
- net: pktgen: remove all superfluous index assignements
- selftest: net: add proc_net_pktgen

Regards,
Peter

Changes v4 -> v5:
 - split up patchset into part i/ii (suggested by Simon Horman)
 - add rev-by Simon Horman
 - net: pktgen: align some variable  declarations to the most common pattern
   -> net: pktgen: fix mix of int/long
   - instead of align to most common pattern (int) adjust all usages to
      size_t for i and max and ssize_t for len and adjust function signatures
      of hex32_arg(), count_trail_chars(), num_arg() and strn_len() accordingly
   - respect reverse xmas tree order for local variable declarations (where
        possible without too much code churn)
   - update subject line and patch description
 - dropped net: pktgen: hex32_arg/num_arg error out in case no characters are
   available
   - keep empty hex/num arg is implicit assumed as zero value
 - dropped net: pktgen: num_arg error out in case no valid character is parsed
   - keep empty hex/num arg is implicit assumed as zero value
 - Change patch description ('Fixes:' -> 'Addresses the following:',
   suggested by Simon Horman)
 - net: pktgen: remove all superfluous index assignements
   - new patch (suggested by Simon Horman)
 - selftest: net: add proc_net_pktgen
   - addapt to dropped patch 'net: pktgen: hex32_arg/num_arg error out in case
     no characters are available', empty hex/num arg is now implicit assumed as
     zero value (instead of failure)

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/[e-mail]/
[2] https://lore.kernel.org/netdev/[e-mail]/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76201b5979768500bca362871db66d77cb4c225e


Peter Seiderer (8):
  net: pktgen: fix mix of int/long
  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: fix mpls reset parsing
  net: pktgen: remove all superfluous index assignements
  selftest: net: add proc_net_pktgen

 net/core/pktgen.c                             | 288 ++++----
 tools/testing/selftests/net/Makefile          |   1 +
 tools/testing/selftests/net/config            |   1 +
 tools/testing/selftests/net/proc_net_pktgen.c | 646 ++++++++++++++++++
 4 files changed, 806 insertions(+), 130 deletions(-)
 create mode 100644 tools/testing/selftests/net/proc_net_pktgen.c

Comments

Jakub Kicinski Feb. 13, 2025, 3:42 p.m. UTC | #1

On Thu, 13 Feb 2025 12:19:12 +0100 Peter Seiderer wrote:
> Patch set splited into part I

> And part II (this one):

Automation doesn't know how to track dependencies so this won't apply
for the testing. I'll mark it as RFC, you'll have to repost..