On Fri, Jun 2, 2023 at 8:02 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 6/1/23 5:26 PM, KP Singh wrote:
> > When task local storage was generalized for tracing programs, the
> > bpf_task_local_storage callback was moved from a BPF LSM hook
> > callback for security_task_free LSM hook to it's own callback. But a
> > failure case in bad_fork_cleanup_security was missed which, when
> > triggered, led to a dangling task owner pointer and a subsequent
> > use-after-free. Move the bpf_task_storage_free to the very end of
> > free_task to handle all failure cases.
> >
> > This issue was noticed when a BPF LSM program was attached to the
> > task_alloc hook on a kernel with KASAN enabled. The program used
> > bpf_task_storage_get to copy the task local storage from the current
> > task to the new task being created.
> >
> > Fixes: a10787e6d58c ("bpf: Enable task local storage for tracing programs")
> > Reported-by: Kuba Piecuch <jpiecuch@xxxxxxxxxx>
> > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
> > ---
> >
> > * v1 -> v2
> >
> > Move the bpf_task_storage_free to free_task as suggested by Martin
> >
> > kernel/fork.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index ed4e01daccaa..cb20f9f596d3 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -627,6 +627,7 @@ void free_task(struct task_struct *tsk)
> > arch_release_task_struct(tsk);
> > if (tsk->flags & PF_KTHREAD)
> > free_kthread_struct(tsk);
> > + bpf_task_storage_free(tsk);
>
> Applied. Thanks for the fix.
>
> A followup question, does it need a "notrace" to be added to
> bpf_task_storage_free to ensure no task storage can be recreated?
Should we do a notrace or a BTF set based deny list?
