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.
>
> 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()?
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
>
>
