[PATCH bpf v2] bpf: Fix UAF in task local storage

[PATCH bpf v2] bpf: Fix UAF in task local storage

[KP Singh]

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@google.com>
Signed-off-by: KP Singh <kpsingh@kernel.org>
---

* 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);
 	free_task_struct(tsk);
 }
 EXPORT_SYMBOL(free_task);
@@ -979,7 +980,6 @@ void __put_task_struct(struct task_struct *tsk)
 	cgroup_free(tsk);
 	task_numa_free(tsk, true);
 	security_task_free(tsk);
-	bpf_task_storage_free(tsk);
 	exit_creds(tsk);
 	delayacct_tsk_free(tsk);
 	put_signal_struct(tsk->signal);
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH bpf v2] bpf: Fix UAF in task local storage

[Song Liu]

On Thu, Jun 1, 2023 at 5:26 PM KP Singh <kpsingh@kernel.org> 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@google.com>
> Signed-off-by: KP Singh <kpsingh@kernel.org>

Acked-by: Song Liu <song@kernel.org>

Thanks,
Song

> ---
>
> * 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);
>         free_task_struct(tsk);
>  }
>  EXPORT_SYMBOL(free_task);
> @@ -979,7 +980,6 @@ void __put_task_struct(struct task_struct *tsk)
>         cgroup_free(tsk);
>         task_numa_free(tsk, true);
>         security_task_free(tsk);
> -       bpf_task_storage_free(tsk);
>         exit_creds(tsk);
>         delayacct_tsk_free(tsk);
>         put_signal_struct(tsk->signal);
> --
> 2.41.0.rc0.172.g3f132b7071-goog
>
>

Re: [PATCH bpf v2] bpf: Fix UAF in task local storage

[Martin KaFai Lau]

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@google.com>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
> 
> * 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?

>   	free_task_struct(tsk);
>   }
>   EXPORT_SYMBOL(free_task);
> @@ -979,7 +980,6 @@ void __put_task_struct(struct task_struct *tsk)
>   	cgroup_free(tsk);
>   	task_numa_free(tsk, true);
>   	security_task_free(tsk);
> -	bpf_task_storage_free(tsk);
>   	exit_creds(tsk);
>   	delayacct_tsk_free(tsk);
>   	put_signal_struct(tsk->signal);

Re: [PATCH bpf v2] bpf: Fix UAF in task local storage

[KP Singh]

On Fri, Jun 2, 2023 at 8:02 PM Martin KaFai Lau <martin.lau@linux.dev> 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@google.com>
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---
> >
> > * 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?