Re: [PATCH bpf-next v7 3/4] bpf: Prevent task work context reuse while irq_work is busy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: UAF in task_work/timer callbacks due to async context re-init on deleted map elements
--

commit 580f53f590a0a7077f231292b13de90213f646c5
Author: Yun Lu <luyun@kylinos.cn>

bpf: Prevent task work context reuse while irq_work is busy

This commit fixes a race condition where a task work context could return to
standby before its scheduling irq_work handler completes. It initializes the
irq_work during context creation and checks for the BUSY state before reusing
it to prevent overlapping invocations.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 028fc6796b597..350269b8646e4 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -4553,6 +4553,7 @@ static struct bpf_task_work_ctx *bpf_task_work_fetch_ctx(struct bpf_task_work *t
>  	memset(ctx, 0, sizeof(*ctx));
>  	refcount_set(&ctx->refcnt, 1); /* map's own ref */
>  	ctx->state = BPF_TW_STANDBY;
> +	init_irq_work(&ctx->irq_work, bpf_task_work_irq);
>  
>  	old_ctx = cmpxchg(&twk->ctx, NULL, ctx);

[Severity: High]
Can a newly allocated context be installed on a deleted map element here?

If a concurrent map deletion calls bpf_task_work_cancel_and_free(), it
synchronously clears the context:

bpf_task_work_cancel_and_free(struct bpf_task_work *tw) {
    ...
    ctx = xchg(&twk->ctx, NULL);
    ...
}

Since it doesn't leave a poison value, a concurrent BPF program running under
RCU read lock that calls bpf_task_work_schedule() would see twk->ctx as NULL
and successfully cmpxchg the newly allocated context into the deleted element.

[ ... ]

> @@ -4644,7 +4657,6 @@ static int bpf_task_work_schedule(struct task_struct *task, struct bpf_task_work
>  	ctx->map = map;
>  	ctx->map_val = (void *)tw - map->record->task_work_off;
>  	init_task_work(&ctx->work, bpf_task_work_callback);
> -	init_irq_work(&ctx->irq_work, bpf_task_work_irq);
>  
>  	irq_work_queue(&ctx->irq_work);
>  	return 0;

[Severity: High]
Could this lead to a use-after-free if the element was concurrently deleted?

If bpf_task_work_fetch_ctx() re-initializes a context on a deleted element as
described above, ctx->map_val captures a pointer to the element's memory:

bpf_task_work_schedule(...) {
    ...
    ctx->map_val = (void *)tw - map->record->task_work_off;
    ...
    irq_work_queue(&ctx->irq_work);
}

Since the deleted map element will be freed after the RCU grace period, but
the newly initialized asynchronous irq_work remains armed, could the later
execution of this callback access the freed element memory through
ctx->map_val?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012061915.231254-1-luyun_611@163.com?part=3