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

Commit Message

Yafang Shao Feb. 17, 2024, 11:41 a.m. UTC

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 <[e-mail]>
Acked-by: Yonghong Song <[e-mail]>
Cc: Chuyi Zhou <[e-mail]>
Cc: Oleg Nesterov <[e-mail]>
---
 kernel/bpf/task_iter.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Oleg Nesterov Feb. 17, 2024, 12:03 p.m. UTC | #1

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 <[e-mail]>
> Acked-by: Yonghong Song <[e-mail]>
> Cc: Chuyi Zhou <[e-mail]>
> Cc: Oleg Nesterov <[e-mail]>
> ---
>  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
>

Yafang Shao Feb. 17, 2024, 1:11 p.m. UTC | #2

On Sat, Feb 17, 2024 at 8:05 PM Oleg Nesterov <[e-mail]> 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/[e-mail]/

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 <[e-mail]>
> > Acked-by: Yonghong Song <[e-mail]>
> > Cc: Chuyi Zhou <[e-mail]>
> > Cc: Oleg Nesterov <[e-mail]>
> > ---
> >  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
> >
>

Oleg Nesterov Feb. 17, 2024, 4:43 p.m. UTC | #3

On 02/17, Yafang Shao wrote:
>
> On Sat, Feb 17, 2024 at 8:05 PM Oleg Nesterov <[e-mail]> 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/[e-mail]/
>
> 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 <[e-mail]>

Patch

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: