BPF List
 help / color / mirror / Atom feed
* [PATCH bpf-next v1 0/3] Fix missing process_iter_arg type check
@ 2024-11-27 23:01 Kumar Kartikeya Dwivedi
  2024-11-27 23:01 ` [PATCH bpf-next v1 1/3] bpf: Ensure reg is PTR_TO_STACK in process_iter_arg Kumar Kartikeya Dwivedi
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-11-27 23:01 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
	Martin KaFai Lau, Eduard Zingerman, Tao Lyu, Mathias Payer,
	Meng Xu, Sanidhya Kashyap

I am taking over Tao's earlier patch set that can be found at [0], after
an offline discussion. The bug reported in that thread is that
process_iter_arg missed a reg->type == PTR_TO_STACK check. Fix this by
adding it in, and also address comments from Andrii on the earlier
attempt. Include more selftests to ensure the error is caught.

  [0]: https://lore.kernel.org/bpf/20241107214736.347630-1-tao.lyu@epfl.ch

Kumar Kartikeya Dwivedi (2):
  bpf: Zero index arg error string for dynptr and iter
  selftests/bpf: Add tests for iter arg check

Tao Lyu (1):
  bpf: Ensure reg is PTR_TO_STACK in process_iter_arg

 kernel/bpf/verifier.c                         | 17 +++++++-----
 .../testing/selftests/bpf/progs/dynptr_fail.c | 22 ++++++++--------
 tools/testing/selftests/bpf/progs/iters.c     | 26 +++++++++++++++++++
 .../selftests/bpf/progs/iters_state_safety.c  | 14 +++++-----
 .../selftests/bpf/progs/iters_testmod_seq.c   |  4 +--
 .../bpf/progs/test_kfunc_dynptr_param.c       |  2 +-
 .../selftests/bpf/progs/verifier_bits_iter.c  |  8 +++---
 7 files changed, 62 insertions(+), 31 deletions(-)


base-commit: c8d02b547363880d996f80c38cc8b997c7b90725
-- 
2.43.5


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH bpf-next v1 1/3] bpf: Ensure reg is PTR_TO_STACK in process_iter_arg
  2024-11-27 23:01 [PATCH bpf-next v1 0/3] Fix missing process_iter_arg type check Kumar Kartikeya Dwivedi
@ 2024-11-27 23:01 ` Kumar Kartikeya Dwivedi
  2024-11-27 23:01 ` [PATCH bpf-next v1 2/3] bpf: Zero index arg error string for dynptr and iter Kumar Kartikeya Dwivedi
  2024-11-27 23:01 ` [PATCH bpf-next v1 3/3] selftests/bpf: Add tests for iter arg check Kumar Kartikeya Dwivedi
  2 siblings, 0 replies; 6+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-11-27 23:01 UTC (permalink / raw)
  To: bpf
  Cc: Andrii Nakryiko, Tao Lyu, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Eduard Zingerman, Mathias Payer, Meng Xu,
	Sanidhya Kashyap

From: Tao Lyu <tao.lyu@epfl.ch>

Currently, KF_ARG_PTR_TO_ITER handling missed checking the reg->type and
ensuring it is PTR_TO_STACK. Instead of enforcing this in the caller of
process_iter_arg, move the check into it instead so that all callers
will gain the check by default. This is similar to process_dynptr_func.

An existing selftest in verifier_bits_iter.c fails due to this change,
but it's because it was passing a NULL pointer into iter_next helper and
getting an error further down the checks, but probably meant to pass an
uninitialized iterator on the stack (as is done in the subsequent test
below it). We will gain coverage for non-PTR_TO_STACK arguments in later
patches hence just change the declaration to zero-ed stack object.

Fixes: 06accc8779c1 ("bpf: add support for open-coded iterator loops")
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Tao Lyu <tao.lyu@epfl.ch>
[ Kartikeya: move check into process_iter_arg, rewrite commit log ]
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/verifier.c                                  | 5 +++++
 tools/testing/selftests/bpf/progs/verifier_bits_iter.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1c4ebb326785..358a3566bb60 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8189,6 +8189,11 @@ static int process_iter_arg(struct bpf_verifier_env *env, int regno, int insn_id
 	const struct btf_type *t;
 	int spi, err, i, nr_slots, btf_id;
 
+	if (reg->type != PTR_TO_STACK) {
+		verbose(env, "arg#%d expected pointer to an iterator on stack\n", regno - 1);
+		return -EINVAL;
+	}
+
 	/* For iter_{new,next,destroy} functions, btf_check_iter_kfuncs()
 	 * ensures struct convention, so we wouldn't need to do any BTF
 	 * validation here. But given iter state can be passed as a parameter
diff --git a/tools/testing/selftests/bpf/progs/verifier_bits_iter.c b/tools/testing/selftests/bpf/progs/verifier_bits_iter.c
index 7c881bca9af5..a7a6ae6c162f 100644
--- a/tools/testing/selftests/bpf/progs/verifier_bits_iter.c
+++ b/tools/testing/selftests/bpf/progs/verifier_bits_iter.c
@@ -35,9 +35,9 @@ __description("uninitialized iter in ->next()")
 __failure __msg("expected an initialized iter_bits as arg #1")
 int BPF_PROG(next_uninit, struct bpf_iter_meta *meta, struct cgroup *cgrp)
 {
-	struct bpf_iter_bits *it = NULL;
+	struct bpf_iter_bits it = {};
 
-	bpf_iter_bits_next(it);
+	bpf_iter_bits_next(&it);
 	return 0;
 }
 
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH bpf-next v1 2/3] bpf: Zero index arg error string for dynptr and iter
  2024-11-27 23:01 [PATCH bpf-next v1 0/3] Fix missing process_iter_arg type check Kumar Kartikeya Dwivedi
  2024-11-27 23:01 ` [PATCH bpf-next v1 1/3] bpf: Ensure reg is PTR_TO_STACK in process_iter_arg Kumar Kartikeya Dwivedi
@ 2024-11-27 23:01 ` Kumar Kartikeya Dwivedi
  2024-11-27 23:06   ` Kumar Kartikeya Dwivedi
  2024-11-27 23:01 ` [PATCH bpf-next v1 3/3] selftests/bpf: Add tests for iter arg check Kumar Kartikeya Dwivedi
  2 siblings, 1 reply; 6+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-11-27 23:01 UTC (permalink / raw)
  To: bpf
  Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Eduard Zingerman, Tao Lyu, Mathias Payer,
	Meng Xu, Sanidhya Kashyap

Andrii spotted that process_dynptr_func's rejection of incorrect
argument register type will print an error string where argument numbers
are not zero-indexed, unlike elsewhere in the verifier.  Fix this by
subtracting 1 from regno. The same scenario exists for iterator
messages. Fix selftest error strings that match on the exact argument
number while we're at it to ensure clean bisection.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/verifier.c                         | 12 +++++-----
 .../testing/selftests/bpf/progs/dynptr_fail.c | 22 +++++++++----------
 .../selftests/bpf/progs/iters_state_safety.c  | 14 ++++++------
 .../selftests/bpf/progs/iters_testmod_seq.c   |  4 ++--
 .../bpf/progs/test_kfunc_dynptr_param.c       |  2 +-
 .../selftests/bpf/progs/verifier_bits_iter.c  |  4 ++--
 6 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 358a3566bb60..2fd35465d650 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8071,7 +8071,7 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn
 	if (reg->type != PTR_TO_STACK && reg->type != CONST_PTR_TO_DYNPTR) {
 		verbose(env,
 			"arg#%d expected pointer to stack or const struct bpf_dynptr\n",
-			regno);
+			regno - 1);
 		return -EINVAL;
 	}
 
@@ -8125,7 +8125,7 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn
 		if (!is_dynptr_reg_valid_init(env, reg)) {
 			verbose(env,
 				"Expected an initialized dynptr as arg #%d\n",
-				regno);
+				regno - 1);
 			return -EINVAL;
 		}
 
@@ -8133,7 +8133,7 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn
 		if (!is_dynptr_type_expected(env, reg, arg_type & ~MEM_RDONLY)) {
 			verbose(env,
 				"Expected a dynptr of type %s as arg #%d\n",
-				dynptr_type_str(arg_to_dynptr_type(arg_type)), regno);
+				dynptr_type_str(arg_to_dynptr_type(arg_type)), regno - 1);
 			return -EINVAL;
 		}
 
@@ -8202,7 +8202,7 @@ static int process_iter_arg(struct bpf_verifier_env *env, int regno, int insn_id
 	 */
 	btf_id = btf_check_iter_arg(meta->btf, meta->func_proto, regno - 1);
 	if (btf_id < 0) {
-		verbose(env, "expected valid iter pointer as arg #%d\n", regno);
+		verbose(env, "expected valid iter pointer as arg #%d\n", regno - 1);
 		return -EINVAL;
 	}
 	t = btf_type_by_id(meta->btf, btf_id);
@@ -8212,7 +8212,7 @@ static int process_iter_arg(struct bpf_verifier_env *env, int regno, int insn_id
 		/* bpf_iter_<type>_new() expects pointer to uninit iter state */
 		if (!is_iter_reg_valid_uninit(env, reg, nr_slots)) {
 			verbose(env, "expected uninitialized iter_%s as arg #%d\n",
-				iter_type_str(meta->btf, btf_id), regno);
+				iter_type_str(meta->btf, btf_id), regno - 1);
 			return -EINVAL;
 		}
 
@@ -8236,7 +8236,7 @@ static int process_iter_arg(struct bpf_verifier_env *env, int regno, int insn_id
 			break;
 		case -EINVAL:
 			verbose(env, "expected an initialized iter_%s as arg #%d\n",
-				iter_type_str(meta->btf, btf_id), regno);
+				iter_type_str(meta->btf, btf_id), regno - 1);
 			return err;
 		case -EPROTO:
 			verbose(env, "expected an RCU CS when using %s\n", meta->func_name);
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index 8f36c9de7591..dfd817d0348c 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -149,7 +149,7 @@ int ringbuf_release_uninit_dynptr(void *ctx)
 
 /* A dynptr can't be used after it has been invalidated */
 SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #3")
+__failure __msg("Expected an initialized dynptr as arg #2")
 int use_after_invalid(void *ctx)
 {
 	struct bpf_dynptr ptr;
@@ -428,7 +428,7 @@ int invalid_helper2(void *ctx)
 
 /* A bpf_dynptr is invalidated if it's been written into */
 SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #1")
+__failure __msg("Expected an initialized dynptr as arg #0")
 int invalid_write1(void *ctx)
 {
 	struct bpf_dynptr ptr;
@@ -1407,7 +1407,7 @@ int invalid_slice_rdwr_rdonly(struct __sk_buff *skb)
 
 /* bpf_dynptr_adjust can only be called on initialized dynptrs */
 SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #1")
+__failure __msg("Expected an initialized dynptr as arg #0")
 int dynptr_adjust_invalid(void *ctx)
 {
 	struct bpf_dynptr ptr = {};
@@ -1420,7 +1420,7 @@ int dynptr_adjust_invalid(void *ctx)
 
 /* bpf_dynptr_is_null can only be called on initialized dynptrs */
 SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #1")
+__failure __msg("Expected an initialized dynptr as arg #0")
 int dynptr_is_null_invalid(void *ctx)
 {
 	struct bpf_dynptr ptr = {};
@@ -1433,7 +1433,7 @@ int dynptr_is_null_invalid(void *ctx)
 
 /* bpf_dynptr_is_rdonly can only be called on initialized dynptrs */
 SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #1")
+__failure __msg("Expected an initialized dynptr as arg #0")
 int dynptr_is_rdonly_invalid(void *ctx)
 {
 	struct bpf_dynptr ptr = {};
@@ -1446,7 +1446,7 @@ int dynptr_is_rdonly_invalid(void *ctx)
 
 /* bpf_dynptr_size can only be called on initialized dynptrs */
 SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #1")
+__failure __msg("Expected an initialized dynptr as arg #0")
 int dynptr_size_invalid(void *ctx)
 {
 	struct bpf_dynptr ptr = {};
@@ -1459,7 +1459,7 @@ int dynptr_size_invalid(void *ctx)
 
 /* Only initialized dynptrs can be cloned */
 SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #1")
+__failure __msg("Expected an initialized dynptr as arg #0")
 int clone_invalid1(void *ctx)
 {
 	struct bpf_dynptr ptr1 = {};
@@ -1493,7 +1493,7 @@ int clone_invalid2(struct xdp_md *xdp)
 
 /* Invalidating a dynptr should invalidate its clones */
 SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #3")
+__failure __msg("Expected an initialized dynptr as arg #2")
 int clone_invalidate1(void *ctx)
 {
 	struct bpf_dynptr clone;
@@ -1514,7 +1514,7 @@ int clone_invalidate1(void *ctx)
 
 /* Invalidating a dynptr should invalidate its parent */
 SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #3")
+__failure __msg("Expected an initialized dynptr as arg #2")
 int clone_invalidate2(void *ctx)
 {
 	struct bpf_dynptr ptr;
@@ -1535,7 +1535,7 @@ int clone_invalidate2(void *ctx)
 
 /* Invalidating a dynptr should invalidate its siblings */
 SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #3")
+__failure __msg("Expected an initialized dynptr as arg #2")
 int clone_invalidate3(void *ctx)
 {
 	struct bpf_dynptr ptr;
@@ -1723,7 +1723,7 @@ __noinline long global_call_bpf_dynptr(const struct bpf_dynptr *dynptr)
 }
 
 SEC("?raw_tp")
-__failure __msg("arg#1 expected pointer to stack or const struct bpf_dynptr")
+__failure __msg("arg#0 expected pointer to stack or const struct bpf_dynptr")
 int test_dynptr_reg_type(void *ctx)
 {
 	struct task_struct *current = NULL;
diff --git a/tools/testing/selftests/bpf/progs/iters_state_safety.c b/tools/testing/selftests/bpf/progs/iters_state_safety.c
index d47e59aba6de..f41257eadbb2 100644
--- a/tools/testing/selftests/bpf/progs/iters_state_safety.c
+++ b/tools/testing/selftests/bpf/progs/iters_state_safety.c
@@ -73,7 +73,7 @@ int create_and_forget_to_destroy_fail(void *ctx)
 }
 
 SEC("?raw_tp")
-__failure __msg("expected an initialized iter_num as arg #1")
+__failure __msg("expected an initialized iter_num as arg #0")
 int destroy_without_creating_fail(void *ctx)
 {
 	/* init with zeros to stop verifier complaining about uninit stack */
@@ -91,7 +91,7 @@ int destroy_without_creating_fail(void *ctx)
 }
 
 SEC("?raw_tp")
-__failure __msg("expected an initialized iter_num as arg #1")
+__failure __msg("expected an initialized iter_num as arg #0")
 int compromise_iter_w_direct_write_fail(void *ctx)
 {
 	struct bpf_iter_num iter;
@@ -143,7 +143,7 @@ int compromise_iter_w_direct_write_and_skip_destroy_fail(void *ctx)
 }
 
 SEC("?raw_tp")
-__failure __msg("expected an initialized iter_num as arg #1")
+__failure __msg("expected an initialized iter_num as arg #0")
 int compromise_iter_w_helper_write_fail(void *ctx)
 {
 	struct bpf_iter_num iter;
@@ -230,7 +230,7 @@ int valid_stack_reuse(void *ctx)
 }
 
 SEC("?raw_tp")
-__failure __msg("expected uninitialized iter_num as arg #1")
+__failure __msg("expected uninitialized iter_num as arg #0")
 int double_create_fail(void *ctx)
 {
 	struct bpf_iter_num iter;
@@ -258,7 +258,7 @@ int double_create_fail(void *ctx)
 }
 
 SEC("?raw_tp")
-__failure __msg("expected an initialized iter_num as arg #1")
+__failure __msg("expected an initialized iter_num as arg #0")
 int double_destroy_fail(void *ctx)
 {
 	struct bpf_iter_num iter;
@@ -284,7 +284,7 @@ int double_destroy_fail(void *ctx)
 }
 
 SEC("?raw_tp")
-__failure __msg("expected an initialized iter_num as arg #1")
+__failure __msg("expected an initialized iter_num as arg #0")
 int next_without_new_fail(void *ctx)
 {
 	struct bpf_iter_num iter;
@@ -305,7 +305,7 @@ int next_without_new_fail(void *ctx)
 }
 
 SEC("?raw_tp")
-__failure __msg("expected an initialized iter_num as arg #1")
+__failure __msg("expected an initialized iter_num as arg #0")
 int next_after_destroy_fail(void *ctx)
 {
 	struct bpf_iter_num iter;
diff --git a/tools/testing/selftests/bpf/progs/iters_testmod_seq.c b/tools/testing/selftests/bpf/progs/iters_testmod_seq.c
index 4a176e6aede8..6543d5b6e0a9 100644
--- a/tools/testing/selftests/bpf/progs/iters_testmod_seq.c
+++ b/tools/testing/selftests/bpf/progs/iters_testmod_seq.c
@@ -79,7 +79,7 @@ int testmod_seq_truncated(const void *ctx)
 
 SEC("?raw_tp")
 __failure
-__msg("expected an initialized iter_testmod_seq as arg #2")
+__msg("expected an initialized iter_testmod_seq as arg #1")
 int testmod_seq_getter_before_bad(const void *ctx)
 {
 	struct bpf_iter_testmod_seq it;
@@ -89,7 +89,7 @@ int testmod_seq_getter_before_bad(const void *ctx)
 
 SEC("?raw_tp")
 __failure
-__msg("expected an initialized iter_testmod_seq as arg #2")
+__msg("expected an initialized iter_testmod_seq as arg #1")
 int testmod_seq_getter_after_bad(const void *ctx)
 {
 	struct bpf_iter_testmod_seq it;
diff --git a/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
index e68667aec6a6..cd4d752bd089 100644
--- a/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
+++ b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
@@ -45,7 +45,7 @@ int BPF_PROG(not_valid_dynptr, int cmd, union bpf_attr *attr, unsigned int size)
 }
 
 SEC("?lsm.s/bpf")
-__failure __msg("arg#1 expected pointer to stack or const struct bpf_dynptr")
+__failure __msg("arg#0 expected pointer to stack or const struct bpf_dynptr")
 int BPF_PROG(not_ptr_to_stack, int cmd, union bpf_attr *attr, unsigned int size)
 {
 	unsigned long val = 0;
diff --git a/tools/testing/selftests/bpf/progs/verifier_bits_iter.c b/tools/testing/selftests/bpf/progs/verifier_bits_iter.c
index a7a6ae6c162f..8bcddadfc4da 100644
--- a/tools/testing/selftests/bpf/progs/verifier_bits_iter.c
+++ b/tools/testing/selftests/bpf/progs/verifier_bits_iter.c
@@ -32,7 +32,7 @@ int BPF_PROG(no_destroy, struct bpf_iter_meta *meta, struct cgroup *cgrp)
 
 SEC("iter/cgroup")
 __description("uninitialized iter in ->next()")
-__failure __msg("expected an initialized iter_bits as arg #1")
+__failure __msg("expected an initialized iter_bits as arg #0")
 int BPF_PROG(next_uninit, struct bpf_iter_meta *meta, struct cgroup *cgrp)
 {
 	struct bpf_iter_bits it = {};
@@ -43,7 +43,7 @@ int BPF_PROG(next_uninit, struct bpf_iter_meta *meta, struct cgroup *cgrp)
 
 SEC("iter/cgroup")
 __description("uninitialized iter in ->destroy()")
-__failure __msg("expected an initialized iter_bits as arg #1")
+__failure __msg("expected an initialized iter_bits as arg #0")
 int BPF_PROG(destroy_uninit, struct bpf_iter_meta *meta, struct cgroup *cgrp)
 {
 	struct bpf_iter_bits it = {};
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH bpf-next v1 3/3] selftests/bpf: Add tests for iter arg check
  2024-11-27 23:01 [PATCH bpf-next v1 0/3] Fix missing process_iter_arg type check Kumar Kartikeya Dwivedi
  2024-11-27 23:01 ` [PATCH bpf-next v1 1/3] bpf: Ensure reg is PTR_TO_STACK in process_iter_arg Kumar Kartikeya Dwivedi
  2024-11-27 23:01 ` [PATCH bpf-next v1 2/3] bpf: Zero index arg error string for dynptr and iter Kumar Kartikeya Dwivedi
@ 2024-11-27 23:01 ` Kumar Kartikeya Dwivedi
  2 siblings, 0 replies; 6+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-11-27 23:01 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
	Martin KaFai Lau, Eduard Zingerman, Tao Lyu, Mathias Payer,
	Meng Xu, Sanidhya Kashyap

Add selftests to cover argument type check for iterator kfuncs, and
cover all three kinds (new, next, destroy). Without the fix in the
previous patch, the selftest would not cause a verifier error.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/testing/selftests/bpf/progs/iters.c | 26 +++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/iters.c b/tools/testing/selftests/bpf/progs/iters.c
index ef70b88bccb2..7c969c127573 100644
--- a/tools/testing/selftests/bpf/progs/iters.c
+++ b/tools/testing/selftests/bpf/progs/iters.c
@@ -1486,4 +1486,30 @@ int iter_subprog_check_stacksafe(const void *ctx)
 	return 0;
 }
 
+struct bpf_iter_num global_it;
+
+SEC("raw_tp")
+__failure __msg("arg#0 expected pointer to an iterator on stack")
+int iter_new_bad_arg(const void *ctx)
+{
+	bpf_iter_num_new(&global_it, 0, 1);
+	return 0;
+}
+
+SEC("raw_tp")
+__failure __msg("arg#0 expected pointer to an iterator on stack")
+int iter_next_bad_arg(const void *ctx)
+{
+	bpf_iter_num_next(&global_it);
+	return 0;
+}
+
+SEC("raw_tp")
+__failure __msg("arg#0 expected pointer to an iterator on stack")
+int iter_destroy_bad_arg(const void *ctx)
+{
+	bpf_iter_num_destroy(&global_it);
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next v1 2/3] bpf: Zero index arg error string for dynptr and iter
  2024-11-27 23:01 ` [PATCH bpf-next v1 2/3] bpf: Zero index arg error string for dynptr and iter Kumar Kartikeya Dwivedi
@ 2024-11-27 23:06   ` Kumar Kartikeya Dwivedi
  2024-11-29  0:20     ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 6+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-11-27 23:06 UTC (permalink / raw)
  To: bpf
  Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Eduard Zingerman, Tao Lyu, Mathias Payer,
	Meng Xu, Sanidhya Kashyap

On Thu, 28 Nov 2024 at 00:01, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> Andrii spotted that process_dynptr_func's rejection of incorrect
> argument register type will print an error string where argument numbers
> are not zero-indexed, unlike elsewhere in the verifier.  Fix this by
> subtracting 1 from regno. The same scenario exists for iterator
> messages. Fix selftest error strings that match on the exact argument
> number while we're at it to ensure clean bisection.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> --

When working on this, I noticed the same situation exists in IRQ
save/restore v4.

There are several options:
1. If this lands after IRQ series, I can respin and fix regno use in
process_irq_flag.
2. Maintainer landing IRQ series can s/regno/regno - 1/ in
process_irq_flag, selftests don't match on argument number.
3. I can respin IRQ series v5 with this addressed now.
4. If this lands before IRQ series, I can respin IRQ series and make the fix.
5. It can be a follow up for IRQ series.

Let me know whichever seems better.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next v1 2/3] bpf: Zero index arg error string for dynptr and iter
  2024-11-27 23:06   ` Kumar Kartikeya Dwivedi
@ 2024-11-29  0:20     ` Kumar Kartikeya Dwivedi
  0 siblings, 0 replies; 6+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-11-29  0:20 UTC (permalink / raw)
  To: bpf
  Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Eduard Zingerman, Tao Lyu, Mathias Payer,
	Meng Xu, Sanidhya Kashyap

On Thu, 28 Nov 2024 at 00:06, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Thu, 28 Nov 2024 at 00:01, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > Andrii spotted that process_dynptr_func's rejection of incorrect
> > argument register type will print an error string where argument numbers
> > are not zero-indexed, unlike elsewhere in the verifier.  Fix this by
> > subtracting 1 from regno. The same scenario exists for iterator
> > messages. Fix selftest error strings that match on the exact argument
> > number while we're at it to ensure clean bisection.
> >
> > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > --
>
> When working on this, I noticed the same situation exists in IRQ
> save/restore v4.
>
> There are several options:
> 1. If this lands after IRQ series, I can respin and fix regno use in
> process_irq_flag.
> 2. Maintainer landing IRQ series can s/regno/regno - 1/ in
> process_irq_flag, selftests don't match on argument number.
> 3. I can respin IRQ series v5 with this addressed now.
> 4. If this lands before IRQ series, I can respin IRQ series and make the fix.
> 5. It can be a follow up for IRQ series.
>
> Let me know whichever seems better.

Fixed in IRQ save/restore v5, so ignore.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-11-29  0:21 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-27 23:01 [PATCH bpf-next v1 0/3] Fix missing process_iter_arg type check Kumar Kartikeya Dwivedi
2024-11-27 23:01 ` [PATCH bpf-next v1 1/3] bpf: Ensure reg is PTR_TO_STACK in process_iter_arg Kumar Kartikeya Dwivedi
2024-11-27 23:01 ` [PATCH bpf-next v1 2/3] bpf: Zero index arg error string for dynptr and iter Kumar Kartikeya Dwivedi
2024-11-27 23:06   ` Kumar Kartikeya Dwivedi
2024-11-29  0:20     ` Kumar Kartikeya Dwivedi
2024-11-27 23:01 ` [PATCH bpf-next v1 3/3] selftests/bpf: Add tests for iter arg check Kumar Kartikeya Dwivedi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox