On Thu, Jun 1, 2023 at 6:18 PM Song Liu <song@xxxxxxxxxx> wrote:
>
> On Thu, Jun 1, 2023 at 5:26 AM KP Singh <kpsingh@xxxxxxxxxx> wrote:
> >
> > When the the 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.
> >
> > 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.
>
> This is pretty tricky. Let's add a selftest for this.
I don't have an easy repro for this (the UAF does not trigger
immediately), Also I am not sure how one would test a UAF in a
selftest. What actually happens is:
* We have a dangling task pointer in local storage.
* This is used sometime later which then leads to weird memory
corruption errors.
>
> >
> > Fixes: a10787e6d58c ("bpf: Enable task local storage for tracing programs")
> > Reported-by: Kuba Piecuch <jpiecuch@xxxxxxxxxx>
> > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
> > ---
> >
> > This fixes the regression from the LSM blob based implementation, we can
> > still have UAFs, if bpf_task_storage_get is invoked after bpf_task_storage_free
> > in the cleanup path.
>
> Can we fix this by calling bpf_task_storage_free() from free_task()?
I think we can yeah. But, this is yet another deviation from how the
security blob is managed (security_task_free) frees the blob that we
were previously using.
>
> Thanks,
> Song
>
> >
> > kernel/fork.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index ed4e01daccaa..112d85091ae6 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -2781,6 +2781,7 @@ __latent_entropy struct task_struct *copy_process(
> > exit_sem(p);
> > bad_fork_cleanup_security:
> > security_task_free(p);
> > + bpf_task_storage_free(p);
> > bad_fork_cleanup_audit:
> > audit_free(p);
> > bad_fork_cleanup_perf:
> > --
> > 2.41.0.rc0.172.g3f132b7071-goog
> >
> >
