[PATCH v2 bpf-next 0/2] bpf: Fix an issue in bpf_iter_task

[PATCH v2 bpf-next 0/2] bpf: Fix an issue in bpf_iter_task

[Yafang Shao]

The uninitialized bpf_iter_task variable poses a risk of triggering a
kernel panic. To fix this potential issue, it's imperative to ensure proper
initialization of the variable. This problem surfaced during the
implementation phase of the bits iterator [0]. 

[0]. https://lwn.net/ml/bpf/CALOAHbDJWHOB+viBz6SUqdeF+Nkxmh4gLZo5Ad_keQXjBWHAsQ@mail.gmail.com 

v1->v2:
- Correct the fixes tag (Chuyi)  

Yafang Shao (2):
  bpf: Fix an issue due to uninitialized bpf_iter_task
  selftests/bpf: Add negtive test cases for task iter

 kernel/bpf/task_iter.c                         |  2 ++
 tools/testing/selftests/bpf/prog_tests/iters.c |  1 +
 tools/testing/selftests/bpf/progs/iters_task.c | 12 +++++++++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

-- 
2.39.1

[PATCH v2 bpf-next 1/2] bpf: Fix an issue due to uninitialized bpf_iter_task

[Yafang Shao]

Failure to initialize it->pos, coupled with the presence of an invalid
value in the flags variable, can lead to it->pos referencing an invalid
task, potentially resulting in a kernel panic. To mitigate this risk, it's
crucial to ensure proper initialization of it->pos to NULL.

Fixes: ac8148d957f5 ("bpf: bpf_iter_task_next: use next_task(kit->task) rather than next_task(kit->pos)")
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Acked-by: Yonghong Song <yonghong.song@linux.dev>
Cc: Chuyi Zhou <zhouchuyi@bytedance.com>
Cc: Oleg Nesterov <oleg@redhat.com>
---
 kernel/bpf/task_iter.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index e5c3500443c6..ec4e97c61eef 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -978,6 +978,8 @@ __bpf_kfunc int bpf_iter_task_new(struct bpf_iter_task *it,
 	BUILD_BUG_ON(__alignof__(struct bpf_iter_task_kern) !=
 					__alignof__(struct bpf_iter_task));
 
+	kit->pos = NULL;
+
 	switch (flags) {
 	case BPF_TASK_ITER_ALL_THREADS:
 	case BPF_TASK_ITER_ALL_PROCS:
-- 
2.39.1

Re: [PATCH v2 bpf-next 1/2] bpf: Fix an issue due to uninitialized bpf_iter_task

[Oleg Nesterov]

On 02/17, Yafang Shao wrote:
>
> Failure to initialize it->pos, coupled with the presence of an invalid
> value in the flags variable, can lead to it->pos referencing an invalid
> task, potentially resulting in a kernel panic. To mitigate this risk, it's
> crucial to ensure proper initialization of it->pos to NULL.
>
> Fixes: ac8148d957f5 ("bpf: bpf_iter_task_next: use next_task(kit->task) rather than next_task(kit->pos)")

Confused...

Does this mean that bpf_iter_task_next() (the only user of ->pos) can be
called even if bpf_iter_task_new() returns -EINVAL ?

Oleg.

> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
> Cc: Chuyi Zhou <zhouchuyi@bytedance.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/bpf/task_iter.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> index e5c3500443c6..ec4e97c61eef 100644
> --- a/kernel/bpf/task_iter.c
> +++ b/kernel/bpf/task_iter.c
> @@ -978,6 +978,8 @@ __bpf_kfunc int bpf_iter_task_new(struct bpf_iter_task *it,
>  	BUILD_BUG_ON(__alignof__(struct bpf_iter_task_kern) !=
>  					__alignof__(struct bpf_iter_task));
>  
> +	kit->pos = NULL;
> +
>  	switch (flags) {
>  	case BPF_TASK_ITER_ALL_THREADS:
>  	case BPF_TASK_ITER_ALL_PROCS:
> -- 
> 2.39.1
> 

Re: [PATCH v2 bpf-next 1/2] bpf: Fix an issue due to uninitialized bpf_iter_task

[Yafang Shao]

On Sat, Feb 17, 2024 at 8:05 PM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 02/17, Yafang Shao wrote:
> >
> > Failure to initialize it->pos, coupled with the presence of an invalid
> > value in the flags variable, can lead to it->pos referencing an invalid
> > task, potentially resulting in a kernel panic. To mitigate this risk, it's
> > crucial to ensure proper initialization of it->pos to NULL.
> >
> > Fixes: ac8148d957f5 ("bpf: bpf_iter_task_next: use next_task(kit->task) rather than next_task(kit->pos)")
>
> Confused...
>
> Does this mean that bpf_iter_task_next() (the only user of ->pos) can be
> called even if bpf_iter_task_new() returns -EINVAL ?

Right. The bpf_for_each() doesn't check the return value of bpf_iter_task_new
(), see also https://lore.kernel.org/bpf/20240208090906.56337-4-laoar.shao@gmail.com/

Even if we check the return value of bpf_iter_task_new() in
bpf_for_each(), we still need to fix it in the kernel.

>
> Oleg.
>
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Acked-by: Yonghong Song <yonghong.song@linux.dev>
> > Cc: Chuyi Zhou <zhouchuyi@bytedance.com>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > ---
> >  kernel/bpf/task_iter.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> > index e5c3500443c6..ec4e97c61eef 100644
> > --- a/kernel/bpf/task_iter.c
> > +++ b/kernel/bpf/task_iter.c
> > @@ -978,6 +978,8 @@ __bpf_kfunc int bpf_iter_task_new(struct bpf_iter_task *it,
> >       BUILD_BUG_ON(__alignof__(struct bpf_iter_task_kern) !=
> >                                       __alignof__(struct bpf_iter_task));
> >
> > +     kit->pos = NULL;
> > +
> >       switch (flags) {
> >       case BPF_TASK_ITER_ALL_THREADS:
> >       case BPF_TASK_ITER_ALL_PROCS:
> > --
> > 2.39.1
> >
>


-- 
Regards
Yafang

Re: [PATCH v2 bpf-next 1/2] bpf: Fix an issue due to uninitialized bpf_iter_task

[Oleg Nesterov]

On 02/17, Yafang Shao wrote:
>
> On Sat, Feb 17, 2024 at 8:05 PM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > Fixes: ac8148d957f5 ("bpf: bpf_iter_task_next: use next_task(kit->task) rather than next_task(kit->pos)")
> >
> > Confused...
> >
> > Does this mean that bpf_iter_task_next() (the only user of ->pos) can be
> > called even if bpf_iter_task_new() returns -EINVAL ?
>
> Right. The bpf_for_each() doesn't check the return value of bpf_iter_task_new
> (), see also https://lore.kernel.org/bpf/20240208090906.56337-4-laoar.shao@gmail.com/
>
> Even if we check the return value of bpf_iter_task_new() in
> bpf_for_each(), we still need to fix it in the kernel.

Hmm, OK. Somehow I naively thought there must be an in-kernel check that
would that prevent bpf_iter_task_next() if bpf_iter_task_new() failed.

Thanks for your explanations. FWIW,

Acked-by: Oleg Nesterov <oleg@redhat.com>

[PATCH v2 bpf-next 2/2] selftests/bpf: Add negtive test cases for task iter

[Yafang Shao]

Incorporate a test case to assess the handling of invalid flags or
task__nullable parameters passed to bpf_iter_task_new(). Prior to the
preceding commit, this scenario could potentially trigger a kernel panic.
However, with the previous commit, this test case is expected to function
correctly.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Chuyi Zhou <zhouchuyi@bytedance.com>
---
 tools/testing/selftests/bpf/prog_tests/iters.c |  1 +
 tools/testing/selftests/bpf/progs/iters_task.c | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/iters.c b/tools/testing/selftests/bpf/prog_tests/iters.c
index bf84d4a1d9ae..3c440370c1f0 100644
--- a/tools/testing/selftests/bpf/prog_tests/iters.c
+++ b/tools/testing/selftests/bpf/prog_tests/iters.c
@@ -193,6 +193,7 @@ static void subtest_task_iters(void)
 	ASSERT_EQ(skel->bss->procs_cnt, 1, "procs_cnt");
 	ASSERT_EQ(skel->bss->threads_cnt, thread_num + 1, "threads_cnt");
 	ASSERT_EQ(skel->bss->proc_threads_cnt, thread_num + 1, "proc_threads_cnt");
+	ASSERT_EQ(skel->bss->invalid_cnt, 0, "invalid_cnt");
 	pthread_mutex_unlock(&do_nothing_mutex);
 	for (int i = 0; i < thread_num; i++)
 		ASSERT_OK(pthread_join(thread_ids[i], &ret), "pthread_join");
diff --git a/tools/testing/selftests/bpf/progs/iters_task.c b/tools/testing/selftests/bpf/progs/iters_task.c
index c9b4055cd410..e4d53e40ff20 100644
--- a/tools/testing/selftests/bpf/progs/iters_task.c
+++ b/tools/testing/selftests/bpf/progs/iters_task.c
@@ -10,7 +10,7 @@
 char _license[] SEC("license") = "GPL";
 
 pid_t target_pid;
-int procs_cnt, threads_cnt, proc_threads_cnt;
+int procs_cnt, threads_cnt, proc_threads_cnt, invalid_cnt;
 
 void bpf_rcu_read_lock(void) __ksym;
 void bpf_rcu_read_unlock(void) __ksym;
@@ -26,6 +26,16 @@ int iter_task_for_each_sleep(void *ctx)
 	procs_cnt = threads_cnt = proc_threads_cnt = 0;
 
 	bpf_rcu_read_lock();
+	bpf_for_each(task, pos, NULL, ~0U) {
+		/* Below instructions shouldn't be executed for invalid flags */
+		invalid_cnt++;
+	}
+
+	bpf_for_each(task, pos, NULL, BPF_TASK_ITER_PROC_THREADS) {
+		/* Below instructions shouldn't be executed for invalid task__nullable */
+		invalid_cnt++;
+	}
+
 	bpf_for_each(task, pos, NULL, BPF_TASK_ITER_ALL_PROCS)
 		if (pos->pid == target_pid)
 			procs_cnt++;
-- 
2.39.1

Re: [PATCH v2 bpf-next 0/2] bpf: Fix an issue in bpf_iter_task

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Sat, 17 Feb 2024 19:41:50 +0800 you wrote:
> The uninitialized bpf_iter_task variable poses a risk of triggering a
> kernel panic. To fix this potential issue, it's imperative to ensure proper
> initialization of the variable. This problem surfaced during the
> implementation phase of the bits iterator [0].
> 
> [0]. https://lwn.net/ml/bpf/CALOAHbDJWHOB+viBz6SUqdeF+Nkxmh4gLZo5Ad_keQXjBWHAsQ@mail.gmail.com
> 
> [...]

Here is the summary with links:
  - [v2,bpf-next,1/2] bpf: Fix an issue due to uninitialized bpf_iter_task
    https://git.kernel.org/bpf/bpf/c/5f2ae606cb5a
  - [v2,bpf-next,2/2] selftests/bpf: Add negtive test cases for task iter
    https://git.kernel.org/bpf/bpf/c/5c138a8a4abe

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