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... [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 (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
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 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 --- 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 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 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 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 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 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;
Use defines for the various dec/hex number parsing digits lengths (hex32_arg/num_arg calls).
Signed-off-by: Peter Seiderer ps.report@gmx.net --- Changes v3 -> v4 - new patch (suggested by Simon Horman) --- net/core/pktgen.c | 80 ++++++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 36 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 55064713223e..4f201a2db2dc 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -179,6 +179,15 @@ #define MAX_IMIX_ENTRIES 20 #define IMIX_PRECISION 100 /* Precision of IMIX distribution */
+#define DEC_1_DIGITS 1 +#define DEC_4_DIGITS 4 +#define DEC_5_DIGITS 5 +#define DEC_9_DIGITS 9 +#define DEC_10_DIGITS 10 + +#define HEX_2_DIGITS 2 +#define HEX_8_DIGITS 8 + #define func_enter() pr_debug("entering %s\n", __func__);
#define PKT_FLAGS \ @@ -844,7 +853,6 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen) static ssize_t get_imix_entries(const char __user *buffer, struct pktgen_dev *pkt_dev) { - const int max_digits = 10; int i = 0; long len; char c; @@ -858,7 +866,7 @@ static ssize_t get_imix_entries(const char __user *buffer, if (pkt_dev->n_imix_entries >= MAX_IMIX_ENTRIES) return -E2BIG;
- len = num_arg(&buffer[i], max_digits, &size); + len = num_arg(&buffer[i], DEC_10_DIGITS, &size); if (len < 0) return len; i += len; @@ -872,7 +880,7 @@ static ssize_t get_imix_entries(const char __user *buffer, if (size < 14 + 20 + 8) size = 14 + 20 + 8;
- len = num_arg(&buffer[i], max_digits, &weight); + len = num_arg(&buffer[i], DEC_10_DIGITS, &weight); if (len < 0) return len; if (weight <= 0) @@ -902,7 +910,7 @@ static ssize_t get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) pkt_dev->nr_labels = 0; do { __u32 tmp; - len = hex32_arg(&buffer[i], 8, &tmp); + len = hex32_arg(&buffer[i], HEX_8_DIGITS, &tmp); if (len <= 0) return len; pkt_dev->labels[n] = htonl(tmp); @@ -1008,7 +1016,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "min_pkt_size")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1025,7 +1033,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "max_pkt_size")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1044,7 +1052,7 @@ static ssize_t pktgen_if_write(struct file *file, /* Shortcut for min = max */
if (!strcmp(name, "pkt_size")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1075,7 +1083,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "debug")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1086,7 +1094,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "frags")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1096,7 +1104,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "delay")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1111,7 +1119,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "rate")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1126,7 +1134,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "ratep")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1141,7 +1149,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "udp_src_min")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1154,7 +1162,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "udp_dst_min")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1167,7 +1175,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "udp_src_max")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1180,7 +1188,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "udp_dst_max")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1193,7 +1201,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "clone_skb")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len; /* clone_skb is not supported for netif_receive xmit_mode and @@ -1214,7 +1222,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "count")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1225,7 +1233,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "src_mac_count")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1239,7 +1247,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "dst_mac_count")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1253,7 +1261,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "burst")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1272,7 +1280,7 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "node")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1592,7 +1600,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "flows")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1606,7 +1614,7 @@ static ssize_t pktgen_if_write(struct file *file, } #ifdef CONFIG_XFRM if (!strcmp(name, "spi")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1617,7 +1625,7 @@ static ssize_t pktgen_if_write(struct file *file, } #endif if (!strcmp(name, "flowlen")) { - len = num_arg(&user_buffer[i], 10, &value); + len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); if (len < 0) return len;
@@ -1628,7 +1636,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "queue_map_min")) { - len = num_arg(&user_buffer[i], 5, &value); + len = num_arg(&user_buffer[i], DEC_5_DIGITS, &value); if (len < 0) return len;
@@ -1639,7 +1647,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "queue_map_max")) { - len = num_arg(&user_buffer[i], 5, &value); + len = num_arg(&user_buffer[i], DEC_5_DIGITS, &value); if (len < 0) return len;
@@ -1673,7 +1681,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "vlan_id")) { - len = num_arg(&user_buffer[i], 4, &value); + len = num_arg(&user_buffer[i], DEC_4_DIGITS, &value); if (len < 0) return len;
@@ -1700,7 +1708,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "vlan_p")) { - len = num_arg(&user_buffer[i], 1, &value); + len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value); if (len < 0) return len;
@@ -1715,7 +1723,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "vlan_cfi")) { - len = num_arg(&user_buffer[i], 1, &value); + len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value); if (len < 0) return len;
@@ -1730,7 +1738,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "svlan_id")) { - len = num_arg(&user_buffer[i], 4, &value); + len = num_arg(&user_buffer[i], DEC_4_DIGITS, &value); if (len < 0) return len;
@@ -1757,7 +1765,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "svlan_p")) { - len = num_arg(&user_buffer[i], 1, &value); + len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value); if (len < 0) return len;
@@ -1772,7 +1780,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "svlan_cfi")) { - len = num_arg(&user_buffer[i], 1, &value); + len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value); if (len < 0) return len;
@@ -1788,7 +1796,7 @@ static ssize_t pktgen_if_write(struct file *file,
if (!strcmp(name, "tos")) { __u32 tmp_value = 0; - len = hex32_arg(&user_buffer[i], 2, &tmp_value); + len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value); if (len < 0) return len;
@@ -1804,7 +1812,7 @@ static ssize_t pktgen_if_write(struct file *file,
if (!strcmp(name, "traffic_class")) { __u32 tmp_value = 0; - len = hex32_arg(&user_buffer[i], 2, &tmp_value); + len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value); if (len < 0) return len;
@@ -1819,7 +1827,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "skb_priority")) { - len = num_arg(&user_buffer[i], 9, &value); + len = num_arg(&user_buffer[i], DEC_9_DIGITS, &value); if (len < 0) return len;
On Wed, Feb 05, 2025 at 02:11:44PM +0100, Peter Seiderer wrote:
Use defines for the various dec/hex number parsing digits lengths (hex32_arg/num_arg calls).
Signed-off-by: Peter Seiderer ps.report@gmx.net
Changes v3 -> v4
- new patch (suggested by Simon Horman)
Reviewed-by: Simon Horman horms@kernel.org
Align some variable declarations (in get_imix_entries and get_labels) to the most common pattern (int instead of ssize_t/long) and adjust function return value accordingly.
Signed-off-by: Peter Seiderer ps.report@gmx.net --- Changes v3 -> v4 - new patch (factored out of patch 'net: pktgen: fix access outside of user given buffer in pktgen_if_write()') --- net/core/pktgen.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 4f201a2db2dc..279910367ad4 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -850,12 +850,11 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen) * where each entry consists of size and weight delimited by commas. * "size1,weight_1 size2,weight_2 ... size_n,weight_n" for example. */ -static ssize_t get_imix_entries(const char __user *buffer, - struct pktgen_dev *pkt_dev) +static int get_imix_entries(const char __user *buffer, + struct pktgen_dev *pkt_dev) { - int i = 0; - long len; char c; + int i = 0, len;
pkt_dev->n_imix_entries = 0;
@@ -900,12 +899,11 @@ static ssize_t get_imix_entries(const char __user *buffer, return i; }
-static ssize_t get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) +static int get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) { - unsigned int n = 0; char c; - ssize_t i = 0; - int len; + int i = 0, len; + unsigned int n = 0;
pkt_dev->nr_labels = 0; do {
On Wed, Feb 05, 2025 at 02:11:45PM +0100, Peter Seiderer wrote:
Align some variable declarations (in get_imix_entries and get_labels) to the most common pattern (int instead of ssize_t/long) and adjust function return value accordingly.
Signed-off-by: Peter Seiderer ps.report@gmx.net
Hi Peter,
These comments are is true in general of this patchset, but particularly so in the case of this patch:
* I think a more succinct subject would be nice. * I think the patch description should provide some reason _why_ the change is being made.
Also, specifically relating to this patch, I wonder if it's scope ought to be extended. For example, the two callers of num_arg(), get_imix_entries() and pktgen_if_write() assign the return value of num_arg() to len, which is now an int in both functions. But num_args() returns a long.
Changes v3 -> v4
- new patch (factored out of patch 'net: pktgen: fix access outside of user given buffer in pktgen_if_write()')
net/core/pktgen.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 4f201a2db2dc..279910367ad4 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -850,12 +850,11 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen)
- where each entry consists of size and weight delimited by commas.
- "size1,weight_1 size2,weight_2 ... size_n,weight_n" for example.
*/ -static ssize_t get_imix_entries(const char __user *buffer,
struct pktgen_dev *pkt_dev)+static int get_imix_entries(const char __user *buffer,
struct pktgen_dev *pkt_dev){
- int i = 0;
- long len; char c;
- int i = 0, len;
Given it can be achieved with exactly the same lines changed, just in a different order, please arrange the local variable declarations in reverse xmas tree order - longest line to shortest.
Likewise for the other hunk of this patch. And I believe there are also other cases in this patchset where this comment applied.
The following tool can be useful: https://github.com/ecree-solarflare/xmastree
...
Hello Simon,
On Thu, 6 Feb 2025 13:25:38 +0000, Simon Horman horms@kernel.org wrote:
On Wed, Feb 05, 2025 at 02:11:45PM +0100, Peter Seiderer wrote:
Align some variable declarations (in get_imix_entries and get_labels) to the most common pattern (int instead of ssize_t/long) and adjust function return value accordingly.
Signed-off-by: Peter Seiderer ps.report@gmx.net
Hi Peter,
These comments are is true in general of this patchset, but particularly so in the case of this patch:
- I think a more succinct subject would be nice.
- I think the patch description should provide some reason _why_ the change is being made.
Yep, will improve...
Also, specifically relating to this patch, I wonder if it's scope ought to be extended. For example, the two callers of num_arg(), get_imix_entries() and pktgen_if_write() assign the return value of num_arg() to len, which is now an int in both functions. But num_args() returns a long.
Aim was to get rid of the int/long mixture in the code (which works flawless because no one writes to proc with more than a few bytes AND count is limited to INT_MAX - PAGE_SIZE in vfs_write (see [1], [2])...
I believe the clean way is to use
size_t i, max; ssize_t len;
consequently through out the code and adjust the function signatures accordingly...., will re-spin...
Changes v3 -> v4
- new patch (factored out of patch 'net: pktgen: fix access outside of user given buffer in pktgen_if_write()')
net/core/pktgen.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 4f201a2db2dc..279910367ad4 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -850,12 +850,11 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen)
- where each entry consists of size and weight delimited by commas.
- "size1,weight_1 size2,weight_2 ... size_n,weight_n" for example.
*/ -static ssize_t get_imix_entries(const char __user *buffer,
struct pktgen_dev *pkt_dev)+static int get_imix_entries(const char __user *buffer,
struct pktgen_dev *pkt_dev){
- int i = 0;
- long len; char c;
- int i = 0, len;
Given it can be achieved with exactly the same lines changed, just in a different order, please arrange the local variable declarations in reverse xmas tree order - longest line to shortest.
Likewise for the other hunk of this patch. And I believe there are also other cases in this patchset where this comment applied.
The following tool can be useful: https://github.com/ecree-solarflare/xmastree
O.k. will take a look at it...
Thanks for review!
Regards, Peter
[1] https://elixir.bootlin.com/linux/v6.13.1/source/fs/read_write.c#L673 [2] https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/fs.h#L2704
On Tue, Feb 11, 2025 at 10:29:59AM +0100, Peter Seiderer wrote:
Hello Simon,
On Thu, 6 Feb 2025 13:25:38 +0000, Simon Horman horms@kernel.org wrote:
On Wed, Feb 05, 2025 at 02:11:45PM +0100, Peter Seiderer wrote:
Align some variable declarations (in get_imix_entries and get_labels) to the most common pattern (int instead of ssize_t/long) and adjust function return value accordingly.
Signed-off-by: Peter Seiderer ps.report@gmx.net
Hi Peter,
These comments are is true in general of this patchset, but particularly so in the case of this patch:
- I think a more succinct subject would be nice.
- I think the patch description should provide some reason _why_ the change is being made.
Yep, will improve...
Also, specifically relating to this patch, I wonder if it's scope ought to be extended. For example, the two callers of num_arg(), get_imix_entries() and pktgen_if_write() assign the return value of num_arg() to len, which is now an int in both functions. But num_args() returns a long.
Aim was to get rid of the int/long mixture in the code (which works flawless because no one writes to proc with more than a few bytes AND count is limited to INT_MAX - PAGE_SIZE in vfs_write (see [1], [2])...
I believe the clean way is to use
size_t i, max; ssize_t len;
consequently through out the code and adjust the function signatures accordingly...., will re-spin...
Thanks Peter,
I for one am all for things being consistent.
...
Remove extra tmp variable in pktgen_if_write (re-use len instead).
Signed-off-by: Peter Seiderer ps.report@gmx.net --- Changes v3 -> v4 - new patch (factored out of patch 'net: pktgen: fix access outside of user given buffer in pktgen_if_write()') --- net/core/pktgen.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 279910367ad4..91b06473c925 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -966,7 +966,6 @@ static ssize_t pktgen_if_write(struct file *file, char name[16], valstr[32]; unsigned long value = 0; char *pg_result = NULL; - int tmp = 0; char buf[128];
pg_result = &(pkt_dev->result[0]); @@ -977,12 +976,12 @@ static ssize_t pktgen_if_write(struct file *file, }
max = count; - tmp = count_trail_chars(user_buffer, max); - if (tmp < 0) { + len = count_trail_chars(user_buffer, max); + if (len < 0) { pr_warn("illegal format\n"); - return tmp; + return len; } - i = tmp; + i = len;
/* Read variable name */
On Wed, Feb 05, 2025 at 02:11:46PM +0100, Peter Seiderer wrote:
Remove extra tmp variable in pktgen_if_write (re-use len instead).
Signed-off-by: Peter Seiderer ps.report@gmx.net
Reviewed-by: Simon Horman horms@kernel.org
Remove some superfluous variable initializing before hex32_arg call (as the same init is done here already).
Signed-off-by: Peter Seiderer ps.report@gmx.net --- Changes v3 -> v4 - new patch (factored out of patch 'net: pktgen: fix access outside of user given buffer in pktgen_if_write()') --- net/core/pktgen.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 91b06473c925..84fd88e48275 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -1792,7 +1792,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "tos")) { - __u32 tmp_value = 0; + __u32 tmp_value; len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value); if (len < 0) return len; @@ -1808,7 +1808,7 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "traffic_class")) { - __u32 tmp_value = 0; + __u32 tmp_value; len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value); if (len < 0) return len;
On Wed, Feb 05, 2025 at 02:11:47PM +0100, Peter Seiderer wrote:
Remove some superfluous variable initializing before hex32_arg call (as the same init is done here already).
Signed-off-by: Peter Seiderer ps.report@gmx.net
Reviewed-by: Simon Horman horms@kernel.org
Fix mpls maximum labels list parsing up to MAX_MPLS_LABELS/16 entries (instead of up to MAX_MPLS_LABELS - 1).
Fixes:
$ echo "mpls 00000f00,00000f01,00000f02,00000f03,00000f04,00000f05,00000f06,00000f07,00000f08,00000f09,00000f0a,00000f0b,00000f0c,00000f0d,00000f0e,00000f0f" > /proc/net/pktgen/lo@0 -bash: echo: write error: Argument list too long
Signed-off-by: Peter Seiderer ps.report@gmx.net --- Changes v3 -> v4 - new patch (factored out of patch 'net: pktgen: fix access outside of user given buffer in pktgen_if_write()') --- net/core/pktgen.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 84fd88e48275..0fd15f21119b 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -908,6 +908,10 @@ static int get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) pkt_dev->nr_labels = 0; do { __u32 tmp; + + if (n >= MAX_MPLS_LABELS) + return -E2BIG; + len = hex32_arg(&buffer[i], HEX_8_DIGITS, &tmp); if (len <= 0) return len; @@ -919,8 +923,6 @@ static int get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) return -EFAULT; i++; n++; - if (n >= MAX_MPLS_LABELS) - return -E2BIG; } while (c == ',');
pkt_dev->nr_labels = n;
On Wed, Feb 05, 2025 at 02:11:48PM +0100, Peter Seiderer wrote:
Fix mpls maximum labels list parsing up to MAX_MPLS_LABELS/16 entries (instead of up to MAX_MPLS_LABELS - 1).
Fixes:
"Fixes: " has a special meaning, it is recognised as a tag by tooling, and implies a bug fix. Please consider some other way of describing this, e.g.
Addresses the following:
$ echo "mpls 00000f00,00000f01,00000f02,00000f03,00000f04,00000f05,00000f06,00000f07,00000f08,00000f09,00000f0a,00000f0b,00000f0c,00000f0d,00000f0e,00000f0f" > /proc/net/pktgen/lo@0 -bash: echo: write error: Argument list too long
Signed-off-by: Peter Seiderer ps.report@gmx.net
Changes v3 -> v4
- new patch (factored out of patch 'net: pktgen: fix access outside of user given buffer in pktgen_if_write()')
net/core/pktgen.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 84fd88e48275..0fd15f21119b 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -908,6 +908,10 @@ static int get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) pkt_dev->nr_labels = 0; do { __u32 tmp;
if (n >= MAX_MPLS_LABELS)return -E2BIG;- len = hex32_arg(&buffer[i], HEX_8_DIGITS, &tmp); if (len <= 0) return len;
@@ -919,8 +923,6 @@ static int get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) return -EFAULT; i++; n++;
if (n >= MAX_MPLS_LABELS)return -E2BIG;pkt_dev->nr_labels = n; -- 2.48.1
Honour the user given buffer size for the hex32_arg(), num_arg(), strn_len(), get_imix_entries() and get_labels() calls (otherwise they will access memory outside of the user given buffer).
Signed-off-by: Peter Seiderer ps.report@gmx.net --- Changes v3 -> v4: - 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: - no changes
Changes v1 -> v2: - additional fix get_imix_entries() and get_labels() --- net/core/pktgen.c | 176 ++++++++++++++++++++++++++++++---------------- 1 file changed, 117 insertions(+), 59 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 0fd15f21119b..3c42ecf17ba2 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -851,10 +851,11 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen) * "size1,weight_1 size2,weight_2 ... size_n,weight_n" for example. */ static int get_imix_entries(const char __user *buffer, + unsigned int maxlen, struct pktgen_dev *pkt_dev) { char c; - int i = 0, len; + int i = 0, max, len;
pkt_dev->n_imix_entries = 0;
@@ -865,10 +866,13 @@ static int get_imix_entries(const char __user *buffer, if (pkt_dev->n_imix_entries >= MAX_IMIX_ENTRIES) return -E2BIG;
- len = num_arg(&buffer[i], DEC_10_DIGITS, &size); + max = min(DEC_10_DIGITS, maxlen - i); + len = num_arg(&buffer[i], max, &size); if (len < 0) return len; i += len; + if (i >= maxlen) + return -EINVAL; if (get_user(c, &buffer[i])) return -EFAULT; /* Check for comma between size_i and weight_i */ @@ -879,7 +883,8 @@ static int get_imix_entries(const char __user *buffer, if (size < 14 + 20 + 8) size = 14 + 20 + 8;
- len = num_arg(&buffer[i], DEC_10_DIGITS, &weight); + max = min(DEC_10_DIGITS, maxlen - i); + len = num_arg(&buffer[i], max, &weight); if (len < 0) return len; if (weight <= 0) @@ -889,21 +894,23 @@ static int get_imix_entries(const char __user *buffer, pkt_dev->imix_entries[pkt_dev->n_imix_entries].weight = weight;
i += len; + pkt_dev->n_imix_entries++; + + if (i >= maxlen) + break; if (get_user(c, &buffer[i])) return -EFAULT; - i++; - pkt_dev->n_imix_entries++; } while (c == ' ');
return i; }
-static int get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) +static int get_labels(const char __user *buffer, int maxlen, struct pktgen_dev *pkt_dev) { char c; - int i = 0, len; unsigned int n = 0; + int i = 0, max, len;
pkt_dev->nr_labels = 0; do { @@ -912,17 +919,20 @@ static int get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) if (n >= MAX_MPLS_LABELS) return -E2BIG;
- len = hex32_arg(&buffer[i], HEX_8_DIGITS, &tmp); + max = min(HEX_8_DIGITS, maxlen - i); + len = hex32_arg(&buffer[i], max, &tmp); if (len <= 0) return len; pkt_dev->labels[n] = htonl(tmp); if (pkt_dev->labels[n] & MPLS_STACK_BOTTOM) pkt_dev->flags |= F_MPLS_RND; i += len; + n++; + if (i >= maxlen) + break; if (get_user(c, &buffer[i])) return -EFAULT; i++; - n++; } while (c == ',');
pkt_dev->nr_labels = n; @@ -986,8 +996,8 @@ static ssize_t pktgen_if_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;
@@ -1015,7 +1025,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "min_pkt_size")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1032,7 +1043,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "max_pkt_size")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1051,7 +1063,8 @@ static ssize_t pktgen_if_write(struct file *file, /* Shortcut for min = max */
if (!strcmp(name, "pkt_size")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1071,7 +1084,8 @@ static ssize_t pktgen_if_write(struct file *file, if (pkt_dev->clone_skb > 0) return -EINVAL;
- len = get_imix_entries(&user_buffer[i], pkt_dev); + max = count - i; + len = get_imix_entries(&user_buffer[i], max, pkt_dev); if (len < 0) return len;
@@ -1082,7 +1096,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "debug")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1093,7 +1108,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "frags")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1103,7 +1119,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "delay")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1118,7 +1135,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "rate")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1133,7 +1151,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "ratep")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1148,7 +1167,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "udp_src_min")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1161,7 +1181,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "udp_dst_min")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1174,7 +1195,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "udp_src_max")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1187,7 +1209,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "udp_dst_max")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1200,7 +1223,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "clone_skb")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len; /* clone_skb is not supported for netif_receive xmit_mode and @@ -1221,7 +1245,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "count")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1232,7 +1257,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "src_mac_count")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1246,7 +1272,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "dst_mac_count")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1260,7 +1287,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "burst")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1279,7 +1307,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "node")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1300,11 +1329,12 @@ static ssize_t pktgen_if_write(struct file *file, if (!strcmp(name, "xmit_mode")) { 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) return len;
+ memset(f, 0, sizeof(f)); if (copy_from_user(f, &user_buffer[i], len)) return -EFAULT; i += len; @@ -1340,11 +1370,12 @@ static ssize_t pktgen_if_write(struct file *file, char f[32]; char *end;
- 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) return len;
+ memset(f, 0, 32); if (copy_from_user(f, &user_buffer[i], len)) return -EFAULT; i += len; @@ -1389,7 +1420,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "dst_min") || !strcmp(name, "dst")) { - len = strn_len(&user_buffer[i], sizeof(pkt_dev->dst_min) - 1); + max = min(sizeof(pkt_dev->dst_min) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) return len;
@@ -1409,7 +1441,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "dst_max")) { - len = strn_len(&user_buffer[i], sizeof(pkt_dev->dst_max) - 1); + max = min(sizeof(pkt_dev->dst_max) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) return len;
@@ -1429,7 +1462,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "dst6")) { - len = strn_len(&user_buffer[i], sizeof(buf) - 1); + max = min(sizeof(buf) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) return len;
@@ -1452,7 +1486,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "dst6_min")) { - len = strn_len(&user_buffer[i], sizeof(buf) - 1); + max = min(sizeof(buf) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) return len;
@@ -1474,7 +1509,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "dst6_max")) { - len = strn_len(&user_buffer[i], sizeof(buf) - 1); + max = min(sizeof(buf) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) return len;
@@ -1495,7 +1531,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "src6")) { - len = strn_len(&user_buffer[i], sizeof(buf) - 1); + max = min(sizeof(buf) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) return len;
@@ -1518,7 +1555,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "src_min")) { - len = strn_len(&user_buffer[i], sizeof(pkt_dev->src_min) - 1); + max = min(sizeof(pkt_dev->src_min) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) return len;
@@ -1538,7 +1576,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "src_max")) { - len = strn_len(&user_buffer[i], sizeof(pkt_dev->src_max) - 1); + max = min(sizeof(pkt_dev->src_max) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) return len;
@@ -1558,7 +1597,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "dst_mac")) { - len = strn_len(&user_buffer[i], sizeof(valstr) - 1); + max = min(sizeof(valstr) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) return len;
@@ -1575,7 +1615,8 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "src_mac")) { - len = strn_len(&user_buffer[i], sizeof(valstr) - 1); + max = min(sizeof(valstr) - 1, count - i); + len = strn_len(&user_buffer[i], max); if (len < 0) return len;
@@ -1599,7 +1640,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "flows")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1613,7 +1655,8 @@ static ssize_t pktgen_if_write(struct file *file, } #ifdef CONFIG_XFRM if (!strcmp(name, "spi")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1624,7 +1667,8 @@ static ssize_t pktgen_if_write(struct file *file, } #endif if (!strcmp(name, "flowlen")) { - len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value); + max = min(DEC_10_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1635,7 +1679,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "queue_map_min")) { - len = num_arg(&user_buffer[i], DEC_5_DIGITS, &value); + max = min(DEC_5_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1646,7 +1691,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "queue_map_max")) { - len = num_arg(&user_buffer[i], DEC_5_DIGITS, &value); + max = min(DEC_5_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1659,7 +1705,8 @@ static ssize_t pktgen_if_write(struct file *file, if (!strcmp(name, "mpls")) { unsigned int n, cnt;
- len = get_labels(&user_buffer[i], pkt_dev); + max = count - i; + len = get_labels(&user_buffer[i], max, pkt_dev); if (len < 0) return len; i += len; @@ -1680,7 +1727,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "vlan_id")) { - len = num_arg(&user_buffer[i], DEC_4_DIGITS, &value); + max = min(DEC_4_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1707,7 +1755,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "vlan_p")) { - len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value); + max = min(DEC_1_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1722,7 +1771,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "vlan_cfi")) { - len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value); + max = min(DEC_1_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1737,7 +1787,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "svlan_id")) { - len = num_arg(&user_buffer[i], DEC_4_DIGITS, &value); + max = min(DEC_4_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1764,7 +1815,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "svlan_p")) { - len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value); + max = min(DEC_1_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1779,7 +1831,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "svlan_cfi")) { - len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value); + max = min(DEC_1_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
@@ -1795,7 +1848,9 @@ static ssize_t pktgen_if_write(struct file *file,
if (!strcmp(name, "tos")) { __u32 tmp_value; - len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value); + + max = min(HEX_2_DIGITS, count - i); + len = hex32_arg(&user_buffer[i], max, &tmp_value); if (len < 0) return len;
@@ -1811,7 +1866,9 @@ static ssize_t pktgen_if_write(struct file *file,
if (!strcmp(name, "traffic_class")) { __u32 tmp_value; - len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value); + + max = min(HEX_2_DIGITS, count - i); + len = hex32_arg(&user_buffer[i], max, &tmp_value); if (len < 0) return len;
@@ -1826,7 +1883,8 @@ static ssize_t pktgen_if_write(struct file *file, }
if (!strcmp(name, "skb_priority")) { - len = num_arg(&user_buffer[i], DEC_9_DIGITS, &value); + max = min(DEC_9_DIGITS, count - i); + len = num_arg(&user_buffer[i], max, &value); if (len < 0) return len;
On Wed, Feb 05, 2025 at 02:11:49PM +0100, Peter Seiderer wrote:
Honour the user given buffer size for the hex32_arg(), num_arg(), strn_len(), get_imix_entries() and get_labels() calls (otherwise they will access memory outside of the user given buffer).
Signed-off-by: Peter Seiderer ps.report@gmx.net
Changes v3 -> v4:
- 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:
- no changes
Changes v1 -> v2:
- additional fix get_imix_entries() and get_labels()
Reviewed-by: Simon Horman horms@kernel.org
net/core/pktgen.c | 176 ++++++++++++++++++++++++++++++----------------
...
@@ -1015,7 +1025,8 @@ static ssize_t pktgen_if_write(struct file *file, } if (!strcmp(name, "min_pkt_size")) {
len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
max = min(DEC_10_DIGITS, count - i);len = num_arg(&user_buffer[i], max, &value);
As an aside:
The code immediately following the hunk above is as follows. And this block it is representative of many (all?) of the code modified by the hunks that make up the remainder of this patch.
My observation is that i is incremented but never used again - the function subsequently returns at the end of the if condition.
So perhaps it would be a nice, as a follow-up, to clean this up be removing the increment of i from this and similar blocks.
if (len < 0) return len;
i += len; if (value < 14 + 20 + 8) value = 14 + 20 + 8; if (value != pkt_dev->min_pkt_size) { pkt_dev->min_pkt_size = value; pkt_dev->cur_pkt_size = value; } sprintf(pg_result, "OK: min_pkt_size=%d", pkt_dev->min_pkt_size); return count; }
...
In hex32_arg() and num_arg() error out in case no characters are available (maxlen = 0).
Signed-off-by: Peter Seiderer ps.report@gmx.net --- Changes v3 -> v4 - new patch (factored out of patch 'net: pktgen: fix access outside of user given buffer in pktgen_if_write()') --- net/core/pktgen.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 3c42ecf17ba2..cb3b732fd0a3 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -761,6 +761,9 @@ static int hex32_arg(const char __user *user_buffer, unsigned long maxlen, int i = 0; *num = 0;
+ if (!maxlen) + return -EINVAL; + for (; i < maxlen; i++) { int value; char c; @@ -808,6 +811,9 @@ static long num_arg(const char __user *user_buffer, unsigned long maxlen, int i; *num = 0;
+ if (!maxlen) + return -EINVAL; + for (i = 0; i < maxlen; i++) { char c; if (get_user(c, &user_buffer[i]))
On Wed, Feb 05, 2025 at 02:11:50PM +0100, Peter Seiderer wrote:
In hex32_arg() and num_arg() error out in case no characters are available (maxlen = 0).
Signed-off-by: Peter Seiderer ps.report@gmx.net
Hi Peter,
This patch and the following could benefit from:
* A more succinct subject * An explanation of why the change is being made in the patch description
...
In num_arg() error out in case no valid character is parsed.
Signed-off-by: Peter Seiderer ps.report@gmx.net --- Changes v3 -> v4 - new patch (factored out of patch 'net: pktgen: fix access outside of user given buffer in pktgen_if_write()') --- net/core/pktgen.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index cb3b732fd0a3..a46eb20edf6c 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -821,6 +821,9 @@ static long num_arg(const char __user *user_buffer, unsigned long maxlen, if ((c >= '0') && (c <= '9')) { *num *= 10; *num += c - '0'; + } else if (i == 0) { + /* no valid character parsed, error out */ + return -EINVAL; } else break; }
Fix mpls list reset parsing to work as describe in Documentation/networking/pktgen.rst:
pgset "mpls 0" turn off mpls (or any invalid argument works too!)
- before the patch
$ echo "mpls 00000001,00000002" > /proc/net/pktgen/lo@0 $ grep mpls /proc/net/pktgen/lo@0 mpls: 00000001, 00000002 Result: OK: mpls=00000001,00000002
$ echo "mpls 00000001,00000002" > /proc/net/pktgen/lo@0 $ echo "mpls 0" > /proc/net/pktgen/lo@0 $ grep mpls /proc/net/pktgen/lo@0 mpls: 00000000 Result: OK: mpls=00000000
$ echo "mpls 00000001,00000002" > /proc/net/pktgen/lo@0 $ echo "mpls invalid" > /proc/net/pktgen/lo@0 $ grep mpls /proc/net/pktgen/lo@0 Result: OK: mpls=
- after the patch
$ echo "mpls 00000001,00000002" > /proc/net/pktgen/lo@0 $ grep mpls /proc/net/pktgen/lo@0 mpls: 00000001, 00000002 Result: OK: mpls=00000001,00000002
$ echo "mpls 00000001,00000002" > /proc/net/pktgen/lo@0 $ echo "mpls 0" > /proc/net/pktgen/lo@0 $ grep mpls /proc/net/pktgen/lo@0 Result: OK: mpls=
$ echo "mpls 00000001,00000002" > /proc/net/pktgen/lo@0 $ echo "mpls invalid" > /proc/net/pktgen/lo@0 $ grep mpls /proc/net/pktgen/lo@0 Result: OK: mpls=
Signed-off-by: Peter Seiderer ps.report@gmx.net Reviewed-by: Simon Horman horms@kernel.org --- Changes v3 -> v4 - add rev-by Simon Horman
Changes v2 -> v3: - new patch --- net/core/pktgen.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index a46eb20edf6c..434531a3e3c0 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -930,8 +930,13 @@ static int get_labels(const char __user *buffer, int maxlen, struct pktgen_dev *
max = min(HEX_8_DIGITS, maxlen - i); len = hex32_arg(&buffer[i], max, &tmp); - if (len <= 0) + if (len < 0) return len; + + // return empty list in case of invalid input and/or zero value + if (len == 0 || tmp == 0) + return maxlen; + pkt_dev->labels[n] = htonl(tmp); if (pkt_dev->labels[n] & MPLS_STACK_BOTTOM) pkt_dev->flags |= F_MPLS_RND;
Add some test for /proc/net/pktgen/... interface.
- enable 'CONFIG_NET_PKTGEN=m' in tools/testing/selftests/net/config
Signed-off-by: Peter Seiderer ps.report@gmx.net --- Changes v3 -> v4: - no changes
Changes v2 -> v3: - 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: - 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 --- tools/testing/selftests/net/Makefile | 1 + tools/testing/selftests/net/config | 1 + tools/testing/selftests/net/proc_net_pktgen.c | 650 ++++++++++++++++++ 3 files changed, 652 insertions(+) create mode 100644 tools/testing/selftests/net/proc_net_pktgen.c
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile index 73ee88d6b043..095708cd8345 100644 --- a/tools/testing/selftests/net/Makefile +++ b/tools/testing/selftests/net/Makefile @@ -100,6 +100,7 @@ TEST_PROGS += vlan_bridge_binding.sh TEST_PROGS += bpf_offload.py TEST_PROGS += ipv6_route_update_soft_lockup.sh TEST_PROGS += busy_poll_test.sh +TEST_GEN_PROGS += proc_net_pktgen
# YNL files, must be before "include ..lib.mk" YNL_GEN_FILES := busy_poller netlink-dumps diff --git a/tools/testing/selftests/net/config b/tools/testing/selftests/net/config index 5b9baf708950..9fe1b3464fbc 100644 --- a/tools/testing/selftests/net/config +++ b/tools/testing/selftests/net/config @@ -107,3 +107,4 @@ CONFIG_XFRM_INTERFACE=m CONFIG_XFRM_USER=m CONFIG_IP_NF_MATCH_RPFILTER=m CONFIG_IP6_NF_MATCH_RPFILTER=m +CONFIG_NET_PKTGEN=m diff --git a/tools/testing/selftests/net/proc_net_pktgen.c b/tools/testing/selftests/net/proc_net_pktgen.c new file mode 100644 index 000000000000..81bc9e9e412f --- /dev/null +++ b/tools/testing/selftests/net/proc_net_pktgen.c @@ -0,0 +1,650 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * proc_net_pktgen: kselftest for /proc/net/pktgen interface + * + * Copyright (c) 2025 Peter Seiderer ps.report@gmx.net + * + */ +#include <errno.h> +#include <fcntl.h> +#include <stdlib.h> +#include <unistd.h> + +#include "../kselftest_harness.h" + +static const char ctrl_cmd_stop[] = "stop"; +static const char ctrl_cmd_start[] = "start"; +static const char ctrl_cmd_reset[] = "reset"; + +static const char wrong_ctrl_cmd[] = "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"; + +static const char thr_cmd_add_loopback_0[] = "add_device lo@0"; +static const char thr_cmd_rm_loopback_0[] = "rem_device_all"; + +static const char wrong_thr_cmd[] = "forsureawrongcommand"; +static const char legacy_thr_cmd[] = "max_before_softirq"; + +static const char wrong_dev_cmd[] = "forsurewrongcommand"; +static const char dev_cmd_min_pkt_size_0[] = "min_pkt_size"; +static const char dev_cmd_min_pkt_size_1[] = "min_pkt_size "; +static const char dev_cmd_min_pkt_size_2[] = "min_pkt_size 0"; +static const char dev_cmd_min_pkt_size_3[] = "min_pkt_size 1"; +static const char dev_cmd_min_pkt_size_4[] = "min_pkt_size 100"; +static const char dev_cmd_min_pkt_size_5[] = "min_pkt_size=1001"; +static const char dev_cmd_min_pkt_size_6[] = "min_pkt_size =2002"; +static const char dev_cmd_min_pkt_size_7[] = "min_pkt_size= 3003"; +static const char dev_cmd_min_pkt_size_8[] = "min_pkt_size = 4004"; +static const char dev_cmd_max_pkt_size_0[] = "max_pkt_size 200"; +static const char dev_cmd_pkt_size_0[] = "pkt_size 300"; +static const char dev_cmd_imix_weights_0[] = "imix_weights 0,7 576,4 1500,1"; +static const char dev_cmd_imix_weights_1[] = "imix_weights 101,1 102,2 103,3 104,4 105,5 106,6 107,7 108,8 109,9 110,10 111,11 112,12 113,13 114,14 115,15 116,16 117,17 118,18 119,19 120,20"; +static const char dev_cmd_imix_weights_2[] = "imix_weights 100,1 102,2 103,3 104,4 105,5 106,6 107,7 108,8 109,9 110,10 111,11 112,12 113,13 114,14 115,15 116,16 117,17 118,18 119,19 120,20 121,21"; +static const char dev_cmd_debug_0[] = "debug 1"; +static const char dev_cmd_debug_1[] = "debug 0"; +static const char dev_cmd_frags_0[] = "frags 100"; +static const char dev_cmd_delay_0[] = "delay 100"; +static const char dev_cmd_delay_1[] = "delay 2147483647"; +static const char dev_cmd_rate_0[] = "rate 0"; +static const char dev_cmd_rate_1[] = "rate 100"; +static const char dev_cmd_ratep_0[] = "ratep 0"; +static const char dev_cmd_ratep_1[] = "ratep 200"; +static const char dev_cmd_udp_src_min_0[] = "udp_src_min 1"; +static const char dev_cmd_udp_dst_min_0[] = "udp_dst_min 2"; +static const char dev_cmd_udp_src_max_0[] = "udp_src_max 3"; +static const char dev_cmd_udp_dst_max_0[] = "udp_dst_max 4"; +static const char dev_cmd_clone_skb_0[] = "clone_skb 1"; +static const char dev_cmd_clone_skb_1[] = "clone_skb 0"; +static const char dev_cmd_count_0[] = "count 100"; +static const char dev_cmd_src_mac_count_0[] = "src_mac_count 100"; +static const char dev_cmd_dst_mac_count_0[] = "dst_mac_count 100"; +static const char dev_cmd_burst_0[] = "burst 0"; +static const char dev_cmd_node_0[] = "node 100"; +static const char dev_cmd_xmit_mode_0[] = "xmit_mode start_xmit"; +static const char dev_cmd_xmit_mode_1[] = "xmit_mode netif_receive"; +static const char dev_cmd_xmit_mode_2[] = "xmit_mode queue_xmit"; +static const char dev_cmd_xmit_mode_3[] = "xmit_mode nonsense"; +static const char dev_cmd_flag_0[] = "flag UDPCSUM"; +static const char dev_cmd_flag_1[] = "flag !UDPCSUM"; +static const char dev_cmd_flag_2[] = "flag nonsense"; +static const char dev_cmd_dst_min_0[] = "dst_min 101.102.103.104"; +static const char dev_cmd_dst_0[] = "dst 101.102.103.104"; +static const char dev_cmd_dst_max_0[] = "dst_max 201.202.203.204"; +static const char dev_cmd_dst6_0[] = "dst6 2001:db38:1234:0000:0000:0000:0000:0000"; +static const char dev_cmd_dst6_min_0[] = "dst6_min 2001:db8:1234:0000:0000:0000:0000:0000"; +static const char dev_cmd_dst6_max_0[] = "dst6_max 2001:db8:1234:0000:0000:0000:0000:0000"; +static const char dev_cmd_src6_0[] = "src6 2001:db38:1234:0000:0000:0000:0000:0000"; +static const char dev_cmd_src_min_0[] = "src_min 101.102.103.104"; +static const char dev_cmd_src_max_0[] = "src_max 201.202.203.204"; +static const char dev_cmd_dst_mac_0[] = "dst_mac 01:02:03:04:05:06"; +static const char dev_cmd_src_mac_0[] = "src_mac 11:12:13:14:15:16"; +static const char dev_cmd_clear_counters_0[] = "clear_counters"; +static const char dev_cmd_flows_0[] = "flows 100"; +static const char dev_cmd_spi_0[] = "spi 100"; +static const char dev_cmd_flowlen_0[] = "flowlen 100"; +static const char dev_cmd_queue_map_min_0[] = "queue_map_min 1"; +static const char dev_cmd_queue_map_max_0[] = "queue_map_max 2"; +static const char dev_cmd_mpls_0[] = "mpls 00000001"; +static const char dev_cmd_mpls_1[] = "mpls 00000001,000000f2"; +static const char dev_cmd_mpls_2[] = "mpls 00000f00,00000f01,00000f02,00000f03,00000f04,00000f05,00000f06,00000f07,00000f08,00000f09,00000f0a,00000f0b,00000f0c,00000f0d,00000f0e,00000f0f"; +static const char dev_cmd_mpls_3[] = "mpls 00000f00,00000f01,00000f02,00000f03,00000f04,00000f05,00000f06,00000f07,00000f08,00000f09,00000f0a,00000f0b,00000f0c,00000f0d,00000f0e,00000f0f,00000f10"; +static const char dev_cmd_vlan_id_0[] = "vlan_id 1"; +static const char dev_cmd_vlan_p_0[] = "vlan_p 1"; +static const char dev_cmd_vlan_cfi_0[] = "vlan_cfi 1"; +static const char dev_cmd_vlan_id_1[] = "vlan_id 4096"; +static const char dev_cmd_svlan_id_0[] = "svlan_id 1"; +static const char dev_cmd_svlan_p_0[] = "svlan_p 1"; +static const char dev_cmd_svlan_cfi_0[] = "svlan_cfi 1"; +static const char dev_cmd_svlan_id_1[] = "svlan_id 4096"; +static const char dev_cmd_tos_0[] = "tos 0"; +static const char dev_cmd_tos_1[] = "tos 0f"; +static const char dev_cmd_tos_2[] = "tos 0ff"; +static const char dev_cmd_traffic_class_0[] = "traffic_class f0"; +static const char dev_cmd_skb_priority_0[] = "skb_priority 999"; + +FIXTURE(proc_net_pktgen) { + int ctrl_fd; + int thr_fd; + int dev_fd; +}; + +FIXTURE_SETUP(proc_net_pktgen) { + int r; + ssize_t len; + + r = system("modprobe pktgen"); + ASSERT_EQ(r, 0) TH_LOG("CONFIG_NET_PKTGEN not enabled, module pktgen not loaded?"); + + self->ctrl_fd = open("/proc/net/pktgen/pgctrl", O_RDWR); + ASSERT_GE(self->ctrl_fd, 0) TH_LOG("CONFIG_NET_PKTGEN not enabled, module pktgen not loaded?"); + + self->thr_fd = open("/proc/net/pktgen/kpktgend_0", O_RDWR); + ASSERT_GE(self->thr_fd, 0) TH_LOG("CONFIG_NET_PKTGEN not enabled, module pktgen not loaded?"); + + len = write(self->thr_fd, thr_cmd_add_loopback_0, sizeof(thr_cmd_add_loopback_0)); + ASSERT_EQ(len, sizeof(thr_cmd_add_loopback_0)) TH_LOG("device lo@0 already registered?"); + + self->dev_fd = open("/proc/net/pktgen/lo@0", O_RDWR); + ASSERT_GE(self->dev_fd, 0) TH_LOG("device entry for lo@0 missing?"); +} + +FIXTURE_TEARDOWN(proc_net_pktgen) { + int ret; + ssize_t len; + + ret = close(self->dev_fd); + EXPECT_EQ(ret, 0); + + len = write(self->thr_fd, thr_cmd_rm_loopback_0, sizeof(thr_cmd_rm_loopback_0)); + EXPECT_EQ(len, sizeof(thr_cmd_rm_loopback_0)); + + ret = close(self->thr_fd); + EXPECT_EQ(ret, 0); + + ret = close(self->ctrl_fd); + EXPECT_EQ(ret, 0); +} + +TEST_F(proc_net_pktgen, wrong_ctrl_cmd) { + for (int i = 0; i <= sizeof(wrong_ctrl_cmd); i++) { + ssize_t len; + + len = write(self->ctrl_fd, wrong_ctrl_cmd, i); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, EINVAL); + } +} + +TEST_F(proc_net_pktgen, ctrl_cmd) { + ssize_t len; + + len = write(self->ctrl_fd, ctrl_cmd_stop, sizeof(ctrl_cmd_stop)); + EXPECT_EQ(len, sizeof(ctrl_cmd_stop)); + + len = write(self->ctrl_fd, ctrl_cmd_stop, sizeof(ctrl_cmd_stop) - 1); + EXPECT_EQ(len, sizeof(ctrl_cmd_stop) - 1); + + len = write(self->ctrl_fd, ctrl_cmd_start, sizeof(ctrl_cmd_start)); + EXPECT_EQ(len, sizeof(ctrl_cmd_start)); + + len = write(self->ctrl_fd, ctrl_cmd_start, sizeof(ctrl_cmd_start) - 1); + EXPECT_EQ(len, sizeof(ctrl_cmd_start) - 1); + + len = write(self->ctrl_fd, ctrl_cmd_reset, sizeof(ctrl_cmd_reset)); + EXPECT_EQ(len, sizeof(ctrl_cmd_reset)); + + len = write(self->ctrl_fd, ctrl_cmd_reset, sizeof(ctrl_cmd_reset) - 1); + EXPECT_EQ(len, sizeof(ctrl_cmd_reset) - 1); +} + +TEST_F(proc_net_pktgen, wrong_thr_cmd) { + for (int i = 0; i <= sizeof(wrong_thr_cmd); i++) { + ssize_t len; + + len = write(self->thr_fd, wrong_thr_cmd, i); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, EINVAL); + } +} + +TEST_F(proc_net_pktgen, legacy_thr_cmd) { + for (int i = 0; i <= sizeof(legacy_thr_cmd); i++) { + ssize_t len; + + len = write(self->thr_fd, legacy_thr_cmd, i); + if (i < (sizeof(legacy_thr_cmd) - 1)) { + // incomplete command string + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, EINVAL); + } else { + // complete command string without/with trailing '\0' + EXPECT_EQ(len, i); + } + } +} + +TEST_F(proc_net_pktgen, wrong_dev_cmd) { + for (int i = 0; i <= sizeof(wrong_dev_cmd); i++) { + ssize_t len; + + len = write(self->dev_fd, wrong_dev_cmd, i); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, EINVAL); + } +} + +TEST_F(proc_net_pktgen, dev_cmd_min_pkt_size) { + ssize_t len; + + // with trailing '\0' + len = write(self->dev_fd, dev_cmd_min_pkt_size_0, sizeof(dev_cmd_min_pkt_size_0)); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, EINVAL); + + // without trailing '\0' + len = write(self->dev_fd, dev_cmd_min_pkt_size_0, sizeof(dev_cmd_min_pkt_size_0) - 1); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, EINVAL); + + // with trailing '\0' + len = write(self->dev_fd, dev_cmd_min_pkt_size_1, sizeof(dev_cmd_min_pkt_size_1)); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, EINVAL); + + // without trailing '\0' + len = write(self->dev_fd, dev_cmd_min_pkt_size_1, sizeof(dev_cmd_min_pkt_size_1) - 1); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, EINVAL); + + // with trailing '\0' + len = write(self->dev_fd, dev_cmd_min_pkt_size_2, sizeof(dev_cmd_min_pkt_size_2)); + EXPECT_EQ(len, sizeof(dev_cmd_min_pkt_size_2)); + + // without trailing '\0' + len = write(self->dev_fd, dev_cmd_min_pkt_size_2, sizeof(dev_cmd_min_pkt_size_2) - 1); + EXPECT_EQ(len, sizeof(dev_cmd_min_pkt_size_2) - 1); + + len = write(self->dev_fd, dev_cmd_min_pkt_size_3, sizeof(dev_cmd_min_pkt_size_3)); + EXPECT_EQ(len, sizeof(dev_cmd_min_pkt_size_3)); + + len = write(self->dev_fd, dev_cmd_min_pkt_size_4, sizeof(dev_cmd_min_pkt_size_4)); + EXPECT_EQ(len, sizeof(dev_cmd_min_pkt_size_4)); + + len = write(self->dev_fd, dev_cmd_min_pkt_size_5, sizeof(dev_cmd_min_pkt_size_5)); + EXPECT_EQ(len, sizeof(dev_cmd_min_pkt_size_5)); + + len = write(self->dev_fd, dev_cmd_min_pkt_size_6, sizeof(dev_cmd_min_pkt_size_6)); + EXPECT_EQ(len, sizeof(dev_cmd_min_pkt_size_6)); + + len = write(self->dev_fd, dev_cmd_min_pkt_size_7, sizeof(dev_cmd_min_pkt_size_7)); + EXPECT_EQ(len, sizeof(dev_cmd_min_pkt_size_7)); + + len = write(self->dev_fd, dev_cmd_min_pkt_size_8, sizeof(dev_cmd_min_pkt_size_8)); + EXPECT_EQ(len, sizeof(dev_cmd_min_pkt_size_8)); +} + +TEST_F(proc_net_pktgen, dev_cmd_max_pkt_size) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_max_pkt_size_0, sizeof(dev_cmd_max_pkt_size_0)); + EXPECT_EQ(len, sizeof(dev_cmd_max_pkt_size_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_pkt_size) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_pkt_size_0, sizeof(dev_cmd_pkt_size_0)); + EXPECT_EQ(len, sizeof(dev_cmd_pkt_size_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_imix_weights) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_imix_weights_0, sizeof(dev_cmd_imix_weights_0)); + EXPECT_EQ(len, sizeof(dev_cmd_imix_weights_0)); + + len = write(self->dev_fd, dev_cmd_imix_weights_1, sizeof(dev_cmd_imix_weights_1)); + EXPECT_EQ(len, sizeof(dev_cmd_imix_weights_1)); + + len = write(self->dev_fd, dev_cmd_imix_weights_2, sizeof(dev_cmd_imix_weights_2)); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, E2BIG); +} + +TEST_F(proc_net_pktgen, dev_cmd_debug) { + ssize_t len; + + // debug on + len = write(self->dev_fd, dev_cmd_debug_0, sizeof(dev_cmd_debug_0)); + EXPECT_EQ(len, sizeof(dev_cmd_debug_0)); + + // debug off + len = write(self->dev_fd, dev_cmd_debug_1, sizeof(dev_cmd_debug_1)); + EXPECT_EQ(len, sizeof(dev_cmd_debug_1)); +} + +TEST_F(proc_net_pktgen, dev_cmd_frags) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_frags_0, sizeof(dev_cmd_frags_0)); + EXPECT_EQ(len, sizeof(dev_cmd_frags_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_delay) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_delay_0, sizeof(dev_cmd_delay_0)); + EXPECT_EQ(len, sizeof(dev_cmd_delay_0)); + + len = write(self->dev_fd, dev_cmd_delay_1, sizeof(dev_cmd_delay_1)); + EXPECT_EQ(len, sizeof(dev_cmd_delay_1)); +} + +TEST_F(proc_net_pktgen, dev_cmd_rate) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_rate_0, sizeof(dev_cmd_rate_0)); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, EINVAL); + + len = write(self->dev_fd, dev_cmd_rate_1, sizeof(dev_cmd_rate_1)); + EXPECT_EQ(len, sizeof(dev_cmd_rate_1)); +} + +TEST_F(proc_net_pktgen, dev_cmd_ratep) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_ratep_0, sizeof(dev_cmd_ratep_0)); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, EINVAL); + + len = write(self->dev_fd, dev_cmd_ratep_1, sizeof(dev_cmd_ratep_1)); + EXPECT_EQ(len, sizeof(dev_cmd_ratep_1)); +} + +TEST_F(proc_net_pktgen, dev_cmd_udp_src_min) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_udp_src_min_0, sizeof(dev_cmd_udp_src_min_0)); + EXPECT_EQ(len, sizeof(dev_cmd_udp_src_min_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_udp_dst_min) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_udp_dst_min_0, sizeof(dev_cmd_udp_dst_min_0)); + EXPECT_EQ(len, sizeof(dev_cmd_udp_dst_min_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_udp_src_max) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_udp_src_max_0, sizeof(dev_cmd_udp_src_max_0)); + EXPECT_EQ(len, sizeof(dev_cmd_udp_src_max_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_udp_dst_max) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_udp_dst_max_0, sizeof(dev_cmd_udp_dst_max_0)); + EXPECT_EQ(len, sizeof(dev_cmd_udp_dst_max_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_clone_skb) { + ssize_t len; + + // clone_skb on (gives EOPNOTSUPP on lo device) + len = write(self->dev_fd, dev_cmd_clone_skb_0, sizeof(dev_cmd_clone_skb_0)); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, EOPNOTSUPP); + + // clone_skb off + len = write(self->dev_fd, dev_cmd_clone_skb_1, sizeof(dev_cmd_clone_skb_1)); + EXPECT_EQ(len, sizeof(dev_cmd_clone_skb_1)); +} + +TEST_F(proc_net_pktgen, dev_cmd_count) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_count_0, sizeof(dev_cmd_count_0)); + EXPECT_EQ(len, sizeof(dev_cmd_count_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_src_mac_count) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_src_mac_count_0, sizeof(dev_cmd_src_mac_count_0)); + EXPECT_EQ(len, sizeof(dev_cmd_src_mac_count_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_dst_mac_count) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_dst_mac_count_0, sizeof(dev_cmd_dst_mac_count_0)); + EXPECT_EQ(len, sizeof(dev_cmd_dst_mac_count_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_burst) { + ssize_t len; + + // burst off + len = write(self->dev_fd, dev_cmd_burst_0, sizeof(dev_cmd_burst_0)); + EXPECT_EQ(len, sizeof(dev_cmd_burst_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_node) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_node_0, sizeof(dev_cmd_node_0)); + EXPECT_EQ(len, sizeof(dev_cmd_node_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_xmit_mode) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_xmit_mode_0, sizeof(dev_cmd_xmit_mode_0)); + EXPECT_EQ(len, sizeof(dev_cmd_xmit_mode_0)); + + len = write(self->dev_fd, dev_cmd_xmit_mode_1, sizeof(dev_cmd_xmit_mode_1)); + EXPECT_EQ(len, sizeof(dev_cmd_xmit_mode_1)); + + len = write(self->dev_fd, dev_cmd_xmit_mode_2, sizeof(dev_cmd_xmit_mode_2)); + EXPECT_EQ(len, sizeof(dev_cmd_xmit_mode_2)); + + len = write(self->dev_fd, dev_cmd_xmit_mode_3, sizeof(dev_cmd_xmit_mode_3)); + EXPECT_EQ(len, sizeof(dev_cmd_xmit_mode_3)); +} + +TEST_F(proc_net_pktgen, dev_cmd_flag) { + ssize_t len; + + // flag UDPCSUM on + len = write(self->dev_fd, dev_cmd_flag_0, sizeof(dev_cmd_flag_0)); + EXPECT_EQ(len, sizeof(dev_cmd_flag_0)); + + // flag UDPCSUM off + len = write(self->dev_fd, dev_cmd_flag_1, sizeof(dev_cmd_flag_1)); + EXPECT_EQ(len, sizeof(dev_cmd_flag_1)); + + // flag invalid + len = write(self->dev_fd, dev_cmd_flag_2, sizeof(dev_cmd_flag_2)); + EXPECT_EQ(len, sizeof(dev_cmd_flag_2)); +} + +TEST_F(proc_net_pktgen, dev_cmd_dst_min) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_dst_min_0, sizeof(dev_cmd_dst_min_0)); + EXPECT_EQ(len, sizeof(dev_cmd_dst_min_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_dst) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_dst_0, sizeof(dev_cmd_dst_0)); + EXPECT_EQ(len, sizeof(dev_cmd_dst_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_dst_max) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_dst_max_0, sizeof(dev_cmd_dst_max_0)); + EXPECT_EQ(len, sizeof(dev_cmd_dst_max_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_dst6) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_dst6_0, sizeof(dev_cmd_dst6_0)); + EXPECT_EQ(len, sizeof(dev_cmd_dst6_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_dst6_min) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_dst6_min_0, sizeof(dev_cmd_dst6_min_0)); + EXPECT_EQ(len, sizeof(dev_cmd_dst6_min_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_dst6_max) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_dst6_max_0, sizeof(dev_cmd_dst6_max_0)); + EXPECT_EQ(len, sizeof(dev_cmd_dst6_max_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_src6) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_src6_0, sizeof(dev_cmd_src6_0)); + EXPECT_EQ(len, sizeof(dev_cmd_src6_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_src_min) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_src_min_0, sizeof(dev_cmd_src_min_0)); + EXPECT_EQ(len, sizeof(dev_cmd_src_min_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_src_max) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_src_max_0, sizeof(dev_cmd_src_max_0)); + EXPECT_EQ(len, sizeof(dev_cmd_src_max_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_dst_mac) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_dst_mac_0, sizeof(dev_cmd_dst_mac_0)); + EXPECT_EQ(len, sizeof(dev_cmd_dst_mac_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_src_mac) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_src_mac_0, sizeof(dev_cmd_src_mac_0)); + EXPECT_EQ(len, sizeof(dev_cmd_src_mac_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_clear_counters) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_clear_counters_0, sizeof(dev_cmd_clear_counters_0)); + EXPECT_EQ(len, sizeof(dev_cmd_clear_counters_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_flows) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_flows_0, sizeof(dev_cmd_flows_0)); + EXPECT_EQ(len, sizeof(dev_cmd_flows_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_spi) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_spi_0, sizeof(dev_cmd_spi_0)); + EXPECT_EQ(len, sizeof(dev_cmd_spi_0)) TH_LOG("CONFIG_XFRM not enabled?"); +} + +TEST_F(proc_net_pktgen, dev_cmd_flowlen) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_flowlen_0, sizeof(dev_cmd_flowlen_0)); + EXPECT_EQ(len, sizeof(dev_cmd_flowlen_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_queue_map_min) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_queue_map_min_0, sizeof(dev_cmd_queue_map_min_0)); + EXPECT_EQ(len, sizeof(dev_cmd_queue_map_min_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_queue_map_max) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_queue_map_max_0, sizeof(dev_cmd_queue_map_max_0)); + EXPECT_EQ(len, sizeof(dev_cmd_queue_map_max_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_mpls) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_mpls_0, sizeof(dev_cmd_mpls_0)); + EXPECT_EQ(len, sizeof(dev_cmd_mpls_0)); + + len = write(self->dev_fd, dev_cmd_mpls_1, sizeof(dev_cmd_mpls_1)); + EXPECT_EQ(len, sizeof(dev_cmd_mpls_1)); + + len = write(self->dev_fd, dev_cmd_mpls_2, sizeof(dev_cmd_mpls_2)); + EXPECT_EQ(len, sizeof(dev_cmd_mpls_2)); + + len = write(self->dev_fd, dev_cmd_mpls_3, sizeof(dev_cmd_mpls_3)); + EXPECT_EQ(len, -1); + EXPECT_EQ(errno, E2BIG); +} + +TEST_F(proc_net_pktgen, dev_cmd_vlan_id) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_vlan_id_0, sizeof(dev_cmd_vlan_id_0)); + EXPECT_EQ(len, sizeof(dev_cmd_vlan_id_0)); + + len = write(self->dev_fd, dev_cmd_vlan_p_0, sizeof(dev_cmd_vlan_p_0)); + EXPECT_EQ(len, sizeof(dev_cmd_vlan_p_0)); + + len = write(self->dev_fd, dev_cmd_vlan_cfi_0, sizeof(dev_cmd_vlan_cfi_0)); + EXPECT_EQ(len, sizeof(dev_cmd_vlan_cfi_0)); + + len = write(self->dev_fd, dev_cmd_vlan_id_1, sizeof(dev_cmd_vlan_id_1)); + EXPECT_EQ(len, sizeof(dev_cmd_vlan_id_1)); +} + +TEST_F(proc_net_pktgen, dev_cmd_svlan_id) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_svlan_id_0, sizeof(dev_cmd_svlan_id_0)); + EXPECT_EQ(len, sizeof(dev_cmd_svlan_id_0)); + + len = write(self->dev_fd, dev_cmd_svlan_p_0, sizeof(dev_cmd_svlan_p_0)); + EXPECT_EQ(len, sizeof(dev_cmd_svlan_p_0)); + + len = write(self->dev_fd, dev_cmd_svlan_cfi_0, sizeof(dev_cmd_svlan_cfi_0)); + EXPECT_EQ(len, sizeof(dev_cmd_svlan_cfi_0)); + + len = write(self->dev_fd, dev_cmd_svlan_id_1, sizeof(dev_cmd_svlan_id_1)); + EXPECT_EQ(len, sizeof(dev_cmd_svlan_id_1)); +} + + +TEST_F(proc_net_pktgen, dev_cmd_tos) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_tos_0, sizeof(dev_cmd_tos_0)); + EXPECT_EQ(len, sizeof(dev_cmd_tos_0)); + + len = write(self->dev_fd, dev_cmd_tos_1, sizeof(dev_cmd_tos_1)); + EXPECT_EQ(len, sizeof(dev_cmd_tos_1)); + + len = write(self->dev_fd, dev_cmd_tos_2, sizeof(dev_cmd_tos_2)); + EXPECT_EQ(len, sizeof(dev_cmd_tos_2)); +} + + +TEST_F(proc_net_pktgen, dev_cmd_traffic_class) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_traffic_class_0, sizeof(dev_cmd_traffic_class_0)); + EXPECT_EQ(len, sizeof(dev_cmd_traffic_class_0)); +} + +TEST_F(proc_net_pktgen, dev_cmd_skb_priority) { + ssize_t len; + + len = write(self->dev_fd, dev_cmd_skb_priority_0, sizeof(dev_cmd_skb_priority_0)); + EXPECT_EQ(len, sizeof(dev_cmd_skb_priority_0)); +} + +TEST_HARNESS_MAIN
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\@0Params: 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\@0Params: 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\@0Params: 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\@0Params: 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\@0Params: 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\@0Params: 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.
linux-kselftest-mirror@lists.linaro.org
-
Peter Seiderer -
Simon Horman