| Message ID | 20230602002612.1117381-1-kpsingh@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf,v2] bpf: Fix UAF in task local storage | expand |
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 > >
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);
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?
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);
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(-)