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 (this one)
- 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 (will follow):
- 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 v5 -> v6: - add rev-by Simon Horman - drop patch 'net: pktgen: use defines for the various dec/hex number parsing digits lengths'
Changes v4 -> v5: - split up patchset into part i/ii (suggested by Simon Horman)
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... [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/?i...
Peter Seiderer (7): 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/core/pktgen.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-)
Replace ENOTSUPP with EOPNOTSUPP, fixes checkpatch hint
WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
and e.g.
$ echo "clone_skb 1" > /proc/net/pktgen/lo@0 -bash: echo: write error: Unknown error 524
Signed-off-by: Peter Seiderer ps.report@gmx.net Reviewed-by: Simon Horman horms@kernel.org --- Changes v5 -> v6 - no changes
Changes v4 -> v5 - split up patchset into part i/ii (suggested by Simon Horman)
Changes v3 -> v4 - add rev-by Simon Horman
Changes v2 -> v3 - no changes
Changes v1 -> v2 - no changes --- net/core/pktgen.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 82b6a2c3c141..496aa16773e7 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -1198,7 +1198,7 @@ static ssize_t pktgen_if_write(struct file *file, if ((value > 0) && ((pkt_dev->xmit_mode == M_NETIF_RECEIVE) || !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))) - return -ENOTSUPP; + return -EOPNOTSUPP; if (value > 0 && (pkt_dev->n_imix_entries > 0 || !(pkt_dev->flags & F_SHARED))) return -EINVAL; @@ -1258,7 +1258,7 @@ static ssize_t pktgen_if_write(struct file *file, ((pkt_dev->xmit_mode == M_QUEUE_XMIT) || ((pkt_dev->xmit_mode == M_START_XMIT) && (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))))) - return -ENOTSUPP; + return -EOPNOTSUPP;
if (value > 1 && !(pkt_dev->flags & F_SHARED)) return -EINVAL; @@ -1303,7 +1303,7 @@ static ssize_t pktgen_if_write(struct file *file, } else if (strcmp(f, "netif_receive") == 0) { /* clone_skb set earlier, not supported in this mode */ if (pkt_dev->clone_skb > 0) - return -ENOTSUPP; + return -EOPNOTSUPP;
pkt_dev->xmit_mode = M_NETIF_RECEIVE;
Enable more flexible parameters syntax, allowing 'param=value' in addition to the already supported 'param value' pattern (additional this gives the skipping '=' in count_trail_chars() a purpose).
Tested with:
$ echo "min_pkt_size 999" > /proc/net/pktgen/lo@0 $ echo "min_pkt_size=999" > /proc/net/pktgen/lo@0 $ echo "min_pkt_size =999" > /proc/net/pktgen/lo@0 $ echo "min_pkt_size= 999" > /proc/net/pktgen/lo@0 $ echo "min_pkt_size = 999" > /proc/net/pktgen/lo@0
Signed-off-by: Peter Seiderer ps.report@gmx.net Reviewed-by: Simon Horman horms@kernel.org --- Changes v5 -> v6 - add rev-by Simon Horman
Changes v4 -> v5 - split up patchset into part i/ii (suggested by Simon Horman)
Changes v3 -> v4: - rephrase commit message (suggested by Paolo Abeni)
Changes v2 -> v3: - no changes
Changes v1 -> v2: - no changes --- net/core/pktgen.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 496aa16773e7..4f8ec6c9bed4 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -823,6 +823,7 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen) case '\r': case '\t': case ' ': + case '=': goto done_str; default: break;
Fix hex32_arg parsing for short reads (here 7 hex digits instead of the expected 8), shift result only on successful input parsing.
- before the patch
$ echo "mpls 0000123" > /proc/net/pktgen/lo@0 $ grep mpls /proc/net/pktgen/lo@0 mpls: 00001230 Result: OK: mpls=00001230
- with patch applied
$ echo "mpls 0000123" > /proc/net/pktgen/lo@0 $ grep mpls /proc/net/pktgen/lo@0 mpls: 00000123 Result: OK: mpls=00000123
Signed-off-by: Peter Seiderer ps.report@gmx.net Reviewed-by: Simon Horman horms@kernel.org --- Changes v5 -> v6 - no changes
Changes v4 -> v5 - split up patchset into part i/ii (suggested by Simon Horman)
Changes v3 -> v4 - add rev-by Simon Horman
Changes v2 -> v3: - no changes
Changes v1 -> v2: - new patch --- net/core/pktgen.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 4f8ec6c9bed4..28dbbf70e142 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -753,14 +753,15 @@ static int hex32_arg(const char __user *user_buffer, unsigned long maxlen, for (; i < maxlen; i++) { int value; char c; - *num <<= 4; if (get_user(c, &user_buffer[i])) return -EFAULT; value = hex_to_bin(c); - if (value >= 0) + if (value >= 0) { + *num <<= 4; *num |= value; - else + } else { break; + } } return i; }
Given an invalid 'rate' command e.g. 'rate 0' the return value is '1', leading to the following misleading output:
- the good case
$ echo "rate 100" > /proc/net/pktgen/lo@0 $ grep "Result:" /proc/net/pktgen/lo@0 Result: OK: rate=100
- the bad case (before the patch)
$ echo "rate 0" > /proc/net/pktgen/lo@0" -bash: echo: write error: Invalid argument $ grep "Result:" /proc/net/pktgen/lo@0 Result: No such parameter "ate"
- with patch applied
$ echo "rate 0" > /proc/net/pktgen/lo@0 -bash: echo: write error: Invalid argument $ grep "Result:" /proc/net/pktgen/lo@0 Result: Idle
Signed-off-by: Peter Seiderer ps.report@gmx.net Reviewed-by: Simon Horman horms@kernel.org --- Changes v5 -> v6 - no changes
Changes v4 -> v5 - split up patchset into part i/ii (suggested by Simon Horman)
Changes v3 -> v4 - add rev-by Simon Horman
Changes v2 -> v3: - no changes
Changes v1 -> v2: - new patch --- net/core/pktgen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 28dbbf70e142..75c7511bf492 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -1115,7 +1115,7 @@ static ssize_t pktgen_if_write(struct file *file,
i += len; if (!value) - return len; + return -EINVAL; pkt_dev->delay = pkt_dev->min_pkt_size*8*NSEC_PER_USEC/value; if (debug) pr_info("Delay set at: %llu ns\n", pkt_dev->delay);
Given an invalid 'ratep' command e.g. 'ratep 0' the return value is '1', leading to the following misleading output:
- the good case
$ echo "ratep 100" > /proc/net/pktgen/lo@0 $ grep "Result:" /proc/net/pktgen/lo@0 Result: OK: ratep=100
- the bad case (before the patch)
$ echo "ratep 0" > /proc/net/pktgen/lo@0" -bash: echo: write error: Invalid argument $ grep "Result:" /proc/net/pktgen/lo@0 Result: No such parameter "atep"
- with patch applied
$ echo "ratep 0" > /proc/net/pktgen/lo@0 -bash: echo: write error: Invalid argument $ grep "Result:" /proc/net/pktgen/lo@0 Result: Idle
Signed-off-by: Peter Seiderer ps.report@gmx.net Reviewed-by: Simon Horman horms@kernel.org --- Changes v5 -> v6 - no changes
Changes v4 -> v5 - split up patchset into part i/ii (suggested by Simon Horman)
Changes v3 -> v4 - add rev-by Simon Horman
Changes v2 -> v3: - no changes
Changes v1 -> v2: - new patch --- net/core/pktgen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 75c7511bf492..c8a5b4d17407 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -1130,7 +1130,7 @@ static ssize_t pktgen_if_write(struct file *file,
i += len; if (!value) - return len; + return -EINVAL; pkt_dev->delay = NSEC_PER_SEC/value; if (debug) pr_info("Delay set at: %llu ns\n", pkt_dev->delay);
Enable command writing without trailing '\n':
- the good case
$ echo "reset" > /proc/net/pktgen/pgctrl
- the bad case (before the patch)
$ echo -n "reset" > /proc/net/pktgen/pgctrl -bash: echo: write error: Invalid argument
- with patch applied
$ echo -n "reset" > /proc/net/pktgen/pgctrl
Signed-off-by: Peter Seiderer ps.report@gmx.net Reviewed-by: Simon Horman horms@kernel.org --- Changes v5 -> v6 - no changes
Changes v4 -> v5 - split up patchset into part i/ii (suggested by Simon Horman)
Changes v3 -> v4 - add rev-by Simon Horman
Changes v2 -> v3: - new patch --- net/core/pktgen.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index c8a5b4d17407..f6e35ba035c7 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -517,21 +517,23 @@ static ssize_t pgctrl_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { char data[128]; + size_t max; struct pktgen_net *pn = net_generic(current->nsproxy->net_ns, pg_net_id);
if (!capable(CAP_NET_ADMIN)) return -EPERM;
- if (count == 0) + if (count < 1) return -EINVAL;
- if (count > sizeof(data)) - count = sizeof(data); - - if (copy_from_user(data, buf, count)) + max = min(count, sizeof(data) - 1); + if (copy_from_user(data, buf, max)) return -EFAULT;
- data[count - 1] = 0; /* Strip trailing '\n' and terminate string */ + if (data[max - 1] == '\n') + data[max - 1] = 0; /* strip trailing '\n', terminate string */ + else + data[max] = 0; /* terminate string */
if (!strcmp(data, "stop")) pktgen_stop_all_threads(pn);
Honour the user given buffer size for the strn_len() calls (otherwise strn_len() will access memory outside of the user given buffer).
Signed-off-by: Peter Seiderer ps.report@gmx.net Reviewed-by: Simon Horman horms@kernel.org --- Changes v5 -> v6 - no changes
Changes v4 -> v5 - split up patchset into part i/ii (suggested by Simon Horman)
Changes v3 -> v4 - add rev-by Simon Horman
Changes v2 -> v3: - no changes
Changes v1 -> v2: - no changes --- net/core/pktgen.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index f6e35ba035c7..55064713223e 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -1900,8 +1900,8 @@ static ssize_t pktgen_thread_write(struct file *file, i = len;
/* Read variable name */ - - len = strn_len(&user_buffer[i], sizeof(name) - 1); + max = min(sizeof(name) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) return len;
@@ -1931,7 +1931,8 @@ static ssize_t pktgen_thread_write(struct file *file, if (!strcmp(name, "add_device")) { char f[32]; memset(f, 0, 32); - len = strn_len(&user_buffer[i], sizeof(f) - 1); + max = min(sizeof(f) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) { ret = len; goto out;
Hello:
This series was applied to netdev/net-next.git (main) by Jakub Kicinski kuba@kernel.org:
On Wed, 19 Feb 2025 09:45:20 +0100 you wrote:
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):
[...]
Here is the summary with links: - [net-next,v6,1/7] net: pktgen: replace ENOTSUPP with EOPNOTSUPP https://git.kernel.org/netdev/net-next/c/802fb6db9fdc - [net-next,v6,2/7] net: pktgen: enable 'param=value' parsing https://git.kernel.org/netdev/net-next/c/80604d19b5fc - [net-next,v6,3/7] net: pktgen: fix hex32_arg parsing for short reads https://git.kernel.org/netdev/net-next/c/b38504346a24 - [net-next,v6,4/7] net: pktgen: fix 'rate 0' error handling (return -EINVAL) https://git.kernel.org/netdev/net-next/c/3ba38c25a8c0 - [net-next,v6,5/7] net: pktgen: fix 'ratep 0' error handling (return -EINVAL) https://git.kernel.org/netdev/net-next/c/1c3bc2c325f8 - [net-next,v6,6/7] net: pktgen: fix ctrl interface command parsing https://git.kernel.org/netdev/net-next/c/1e5e511373fe - [net-next,v6,7/7] net: pktgen: fix access outside of user given buffer in pktgen_thread_write() https://git.kernel.org/netdev/net-next/c/425e64440ad0
You are awesome, thank you!
linux-kselftest-mirror@lists.linaro.org
-
patchwork-bot+netdevbpf＠kernel.org -
Peter Seiderer