[PATCH bpf v2 0/2] Fix missing process_iter_arg type check

[PATCH bpf v2 0/2] Fix missing process_iter_arg type check

[Kumar Kartikeya Dwivedi]

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

Changelog:
----------
v1 -> v2:
v1: https://lore.kernel.org/bpf/20241127230147.4158201-1-memxor@gmail.com

 * Rebase on bpf/master (Andrii)

Kumar Kartikeya Dwivedi (1):
  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                         |  5 ++++
 tools/testing/selftests/bpf/progs/iters.c     | 26 +++++++++++++++++++
 .../selftests/bpf/progs/verifier_bits_iter.c  |  4 +--
 3 files changed, 33 insertions(+), 2 deletions(-)


base-commit: 537a2525eaf76ea9b0dca62b994500d8670b39d5
--
2.43.5

[PATCH bpf v2 1/2] bpf: Ensure reg is PTR_TO_STACK in process_iter_arg

[Kumar Kartikeya Dwivedi]

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

[PATCH bpf v2 2/2] selftests/bpf: Add tests for iter arg check

[Kumar Kartikeya Dwivedi]

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

Re: [PATCH bpf v2 0/2] Fix missing process_iter_arg type check

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Mon,  2 Dec 2024 16:02:36 -0800 you wrote:
> 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
> 
> [...]

Here is the summary with links:
  - [bpf,v2,1/2] bpf: Ensure reg is PTR_TO_STACK in process_iter_arg
    https://git.kernel.org/bpf/bpf/c/12659d28615d
  - [bpf,v2,2/2] selftests/bpf: Add tests for iter arg check
    https://git.kernel.org/bpf/bpf/c/7f71197001e3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html