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

[PATCH bpf-next 0/3] 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 

Yafang Shao (3):
  bpf: Fix an issue due to uninitialized bpf_iter_task
  selftests/bpf: Add negtive test cases for task iter
  libbpf: Check the return value of bpf_iter_<type>_new()

 kernel/bpf/task_iter.c                         |  2 ++
 tools/lib/bpf/bpf_helpers.h                    | 16 ++++++++++++----
 tools/testing/selftests/bpf/prog_tests/iters.c |  1 +
 tools/testing/selftests/bpf/progs/iters_task.c | 12 +++++++++++-
 4 files changed, 26 insertions(+), 5 deletions(-)

-- 
2.39.1

[PATCH bpf-next 1/3] 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 0. 

Fixes: c68a78ffe2cb ("bpf: Introduce task open coded iterator kfuncs")
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Chuyi Zhou <zhouchuyi@bytedance.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 bpf-next 1/3] bpf: Fix an issue due to uninitialized bpf_iter_task

[Chuyi Zhou]

Hello,

在 2024/2/8 17:09, 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 0.
> 
> Fixes: c68a78ffe2cb ("bpf: Introduce task open coded iterator kfuncs")
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Chuyi Zhou <zhouchuyi@bytedance.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:

LGTM.

Actually commit c68a78ffe2c ("bpf: Introduce task open coded iterator 
kfuncs") initialize it->pos to NULL. But it seems the following commit
ac8148d957f5043 ("bpf: bpf_iter_task_next: use next_task(kit->task) 
rather than next_task(kit->pos)") drops this initialization.

Thanks.

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

[Yafang Shao]

On Thu, Feb 8, 2024 at 5:41 PM Chuyi Zhou <zhouchuyi@bytedance.com> wrote:
>
> Hello,
>
> 在 2024/2/8 17:09, 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 0.
> >
> > Fixes: c68a78ffe2cb ("bpf: Introduce task open coded iterator kfuncs")
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Chuyi Zhou <zhouchuyi@bytedance.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:
>
> LGTM.
>
> Actually commit c68a78ffe2c ("bpf: Introduce task open coded iterator
> kfuncs") initialize it->pos to NULL. But it seems the following commit
> ac8148d957f5043 ("bpf: bpf_iter_task_next: use next_task(kit->task)
> rather than next_task(kit->pos)") drops this initialization.
>

Thanks for your quick response. Will update the fixes tag in the next version.

-- 
Regards
Yafang

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

[Yonghong Song]

On 2/8/24 1:41 AM, Chuyi Zhou wrote:
> Hello,
>
> 在 2024/2/8 17:09, 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 0.
>>
>> Fixes: c68a78ffe2cb ("bpf: Introduce task open coded iterator kfuncs")
>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>> Cc: Chuyi Zhou <zhouchuyi@bytedance.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:
>
> LGTM.
>
> Actually commit c68a78ffe2c ("bpf: Introduce task open coded iterator 
> kfuncs") initialize it->pos to NULL. But it seems the following commit
> ac8148d957f5043 ("bpf: bpf_iter_task_next: use next_task(kit->task) 
> rather than next_task(kit->pos)") drops this initialization.

Sorry, I missed this during reviewing commit ac8148d957f5043.
Your change LGTM.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

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

[Alexei Starovoitov]

On Thu, Feb 8, 2024 at 7:53 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 2/8/24 1:41 AM, Chuyi Zhou wrote:
> > Hello,
> >
> > 在 2024/2/8 17:09, 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 0.
> >>
> >> Fixes: c68a78ffe2cb ("bpf: Introduce task open coded iterator kfuncs")
> >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> >> Cc: Chuyi Zhou <zhouchuyi@bytedance.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:
> >
> > LGTM.
> >
> > Actually commit c68a78ffe2c ("bpf: Introduce task open coded iterator
> > kfuncs") initialize it->pos to NULL. But it seems the following commit
> > ac8148d957f5043 ("bpf: bpf_iter_task_next: use next_task(kit->task)
> > rather than next_task(kit->pos)") drops this initialization.
>
> Sorry, I missed this during reviewing commit ac8148d957f5043.
> Your change LGTM.

Ohh. Pls cc Oleg when you respin.

> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>
>

[PATCH bpf-next 2/3] 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

[PATCH bpf-next 3/3] libbpf: Check the return value of bpf_iter_<type>_new()

[Yafang Shao]

On success, bpf_iter_<type>_new() return 0. On failure, it returns ERR.
We'd better check the return value of it.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 tools/lib/bpf/bpf_helpers.h | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index 79eaa581be98..2cd2428e3bc6 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -133,6 +133,15 @@
 # define __bpf_unreachable()	__builtin_trap()
 #endif
 
+#ifndef __must_check
+#define __must_check __attribute__((warn_unused_result))
+#endif
+
+static inline void * __must_check ERR_PTR(long error)
+{
+	return (void *) error;
+}
+
 /*
  * Helper function to perform a tail call with a constant/immediate map slot.
  */
@@ -340,14 +349,13 @@ extern void bpf_iter_num_destroy(struct bpf_iter_num *it) __weak __ksym;
 	/* initialize and define destructor */							\
 	struct bpf_iter_##type ___it __attribute__((aligned(8), /* enforce, just in case */,	\
 						    cleanup(bpf_iter_##type##_destroy))),	\
-	/* ___p pointer is just to call bpf_iter_##type##_new() *once* to init ___it */		\
 			       *___p __attribute__((unused)) = (				\
-					bpf_iter_##type##_new(&___it, ##args),			\
 	/* this is a workaround for Clang bug: it currently doesn't emit BTF */			\
 	/* for bpf_iter_##type##_destroy() when used from cleanup() attribute */		\
-					(void)bpf_iter_##type##_destroy, (void *)0);		\
+					(void)bpf_iter_##type##_destroy,			\
+					ERR_PTR(bpf_iter_##type##_new(&___it, ##args)));	\
 	/* iteration and termination check */							\
-	(((cur) = bpf_iter_##type##_next(&___it)));						\
+	((!___p) && ((cur) = bpf_iter_##type##_next(&___it)));					\
 )
 #endif /* bpf_for_each */
 
-- 
2.39.1

Re: [PATCH bpf-next 3/3] libbpf: Check the return value of bpf_iter_<type>_new()

[Yonghong Song]

On 2/8/24 1:09 AM, Yafang Shao wrote:
> On success, bpf_iter_<type>_new() return 0. On failure, it returns ERR.
> We'd better check the return value of it.

Not sure whether this patch is necessary or not.

I checked:
   bpf_iter_num_{new,next}
   bpf_iter_task_vma_{new,next}
   bpf_iter_css_task_{new,next}

It looks like the convention is for *_next() return NULL or not
instead of relying on return value of _new() to decide whether to
proceed or not. Maybe Andrii can clarify.

>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>   tools/lib/bpf/bpf_helpers.h | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index 79eaa581be98..2cd2428e3bc6 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -133,6 +133,15 @@
>   # define __bpf_unreachable()	__builtin_trap()
>   #endif
>   
> +#ifndef __must_check
> +#define __must_check __attribute__((warn_unused_result))
> +#endif
> +
> +static inline void * __must_check ERR_PTR(long error)
> +{
> +	return (void *) error;
> +}
> +
>   /*
>    * Helper function to perform a tail call with a constant/immediate map slot.
>    */
> @@ -340,14 +349,13 @@ extern void bpf_iter_num_destroy(struct bpf_iter_num *it) __weak __ksym;
>   	/* initialize and define destructor */							\
>   	struct bpf_iter_##type ___it __attribute__((aligned(8), /* enforce, just in case */,	\
>   						    cleanup(bpf_iter_##type##_destroy))),	\
> -	/* ___p pointer is just to call bpf_iter_##type##_new() *once* to init ___it */		\
>   			       *___p __attribute__((unused)) = (				\
> -					bpf_iter_##type##_new(&___it, ##args),			\
>   	/* this is a workaround for Clang bug: it currently doesn't emit BTF */			\
>   	/* for bpf_iter_##type##_destroy() when used from cleanup() attribute */		\
> -					(void)bpf_iter_##type##_destroy, (void *)0);		\
> +					(void)bpf_iter_##type##_destroy,			\
> +					ERR_PTR(bpf_iter_##type##_new(&___it, ##args)));	\
>   	/* iteration and termination check */							\
> -	(((cur) = bpf_iter_##type##_next(&___it)));						\
> +	((!___p) && ((cur) = bpf_iter_##type##_next(&___it)));					\
>   )
>   #endif /* bpf_for_each */
>   

Re: [PATCH bpf-next 3/3] libbpf: Check the return value of bpf_iter_<type>_new()

[Andrii Nakryiko]

On Thu, Feb 8, 2024 at 9:43 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 2/8/24 1:09 AM, Yafang Shao wrote:
> > On success, bpf_iter_<type>_new() return 0. On failure, it returns ERR.
> > We'd better check the return value of it.
>
> Not sure whether this patch is necessary or not.
>
> I checked:
>    bpf_iter_num_{new,next}
>    bpf_iter_task_vma_{new,next}
>    bpf_iter_css_task_{new,next}
>
> It looks like the convention is for *_next() return NULL or not
> instead of relying on return value of _new() to decide whether to
> proceed or not. Maybe Andrii can clarify.

Yes, exactly. if bpf_iter_xxx_new() failed, subsequent
bpf_iter_xxx_next() should return NULL and bpf_iter_xxx_destroy() will
be a no-op as well. So yes, there is no need for extra error checking
here.

>
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >   tools/lib/bpf/bpf_helpers.h | 16 ++++++++++++----
> >   1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > index 79eaa581be98..2cd2428e3bc6 100644
> > --- a/tools/lib/bpf/bpf_helpers.h
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -133,6 +133,15 @@
> >   # define __bpf_unreachable()        __builtin_trap()
> >   #endif
> >
> > +#ifndef __must_check
> > +#define __must_check __attribute__((warn_unused_result))
> > +#endif
> > +
> > +static inline void * __must_check ERR_PTR(long error)
> > +{
> > +     return (void *) error;
> > +}
> > +
> >   /*
> >    * Helper function to perform a tail call with a constant/immediate map slot.
> >    */
> > @@ -340,14 +349,13 @@ extern void bpf_iter_num_destroy(struct bpf_iter_num *it) __weak __ksym;
> >       /* initialize and define destructor */                                                  \
> >       struct bpf_iter_##type ___it __attribute__((aligned(8), /* enforce, just in case */,    \
> >                                                   cleanup(bpf_iter_##type##_destroy))),       \
> > -     /* ___p pointer is just to call bpf_iter_##type##_new() *once* to init ___it */         \
> >                              *___p __attribute__((unused)) = (                                \
> > -                                     bpf_iter_##type##_new(&___it, ##args),                  \
> >       /* this is a workaround for Clang bug: it currently doesn't emit BTF */                 \
> >       /* for bpf_iter_##type##_destroy() when used from cleanup() attribute */                \
> > -                                     (void)bpf_iter_##type##_destroy, (void *)0);            \
> > +                                     (void)bpf_iter_##type##_destroy,                        \
> > +                                     ERR_PTR(bpf_iter_##type##_new(&___it, ##args)));        \
> >       /* iteration and termination check */                                                   \
> > -     (((cur) = bpf_iter_##type##_next(&___it)));                                             \
> > +     ((!___p) && ((cur) = bpf_iter_##type##_next(&___it)));                                  \
> >   )
> >   #endif /* bpf_for_each */
> >