On Sat, Feb 17, 2024 at 8:05 PM Oleg Nesterov <oleg@xxxxxxxxxx> 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@xxxxxxxxx/
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@xxxxxxxxx>
> > Acked-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> > Cc: Chuyi Zhou <zhouchuyi@xxxxxxxxxxxxx>
> > Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
> > ---
> > 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
