Re: [RFC PATCH bpf-next v1 07/11] bpf: Let the callback of bpf_find_vma unwind

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: bpf_find_vma() unwound pad unlocks mmap_lock with IRQs disabled
- [High] bpf: vma pointer escapes bpf_find_vma() lock scope via r0 on unwind_exit
--

commit d84887aac74ddde9372c2538e6e860df21d923db
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Let the callback of bpf_find_vma unwind

This commit adds an unwind landing pad to bpf_find_vma() that releases the RCU
read lock, unlocks mmap_lock, drops any foreign mm reference, and returns the
callback's r0 when the callback leaves via unwind_exit.

> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> index 2ee7b3b3c10be..ae67c97fe845e 100644
> --- a/kernel/bpf/task_iter.c
> +++ b/kernel/bpf/task_iter.c

[ ... ]

> @@ -827,6 +828,20 @@ BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
>  		mmput_async(mm);
>  #endif
>  	return ret;
> +
> +unwound:
> +	/*
> +	 * The callback left with unwind_exit, under the mmap lock and in the
> +	 * RCU section. The way out is the same as after a return.
> +	 */
> +	rcu_read_unlock();
> +	bpf_mmap_unlock_mm(work, mm);
> +#ifdef CONFIG_MMU
> +	if (mmput_needed)
> +		mmput_async(mm);
> +#endif

[Severity: High]
Can the unwound pad end up calling mmap_read_unlock() and mmput_async() with
hardware interrupts disabled?

When bpf_find_vma() is entered with interrupts enabled for a foreign task, the
irqs_disabled() check passes, mmput_needed is set to true, and
bpf_mmap_unlock_guard_get() returns NULL because !irqs_disabled():

kernel/bpf/mmap_unlock_work.h:bpf_mmap_unlock_guard_get() {
	struct mmap_unlock_irq_work *work;

	if (!irqs_disabled())
		return NULL;
    ...
}

Inside the callback, callback_ctx (R3) can point to a stack slot in the
caller's frame. If the callback calls bpf_local_irq_save() on that caller
stack slot and then executes unwind_exit, process_bpf_exit_full() calls
check_resource_leak() with check_lock set to false because curframe > 0:

kernel/bpf/verifier.c:process_bpf_exit_full() {
	int err = check_resource_leak(env, exception_exit,
				      exception_exit || !env->cur_state->curframe,
				      exception_exit ? "bpf_throw" :
				      "BPF_EXIT instruction in main prog");
    ...
}

Unlike a normal callback return, unwind_exit sets state->unwound = true in
prepare_func_exit(), so push_callback_call() returns 0 on revisit without
verifying a second callback round and jumps via unwind_goto to the caller's
pad where bpf_local_irq_restore() restores the caller's stack slot.

At runtime, execution enters the unwound label in bpf_find_vma() with hardware
interrupts disabled while work is NULL and mmput_needed is true.
bpf_mmap_unlock_mm(NULL, mm) then calls mmap_read_unlock(mm) directly with
IRQs disabled instead of deferring via irq_work, and mmput_async(mm) is also
called with IRQs disabled, risking a deadlock on rq_lock during reader wakeup
and triggering an invalid wait context under CONFIG_PREEMPT_RT.

> +	bpf_unwind_leave();
> +	return r0;
>  }

[Severity: High]
What prevents the callback from escaping the vma pointer (or pointers walked
from it, such as vma->vm_file, &vma->vm_file->f_path, or
vma->vm_file->f_inode) in r0 via unwind_exit after mmap_lock, rcu_read_lock(),
and the foreign mm reference have been dropped?

In set_find_vma_callback_state(), R2 (struct vm_area_struct *vma) is marked as
a plain PTR_TO_BTF_ID:

kernel/bpf/verifier.c:set_find_vma_callback_state() {
    ...
	callee->regs[BPF_REG_2].type = PTR_TO_BTF_ID;
	__mark_reg_known_zero(&callee->regs[BPF_REG_2]);
	callee->regs[BPF_REG_2].btf =  btf_vmlinux;
	callee->regs[BPF_REG_2].btf_id = btf_tracing_ids[BTF_TRACING_TYPE_VMA];
    ...
}

When the callback leaves via unwind_exit, prepare_func_exit() skips the
SCALAR_VALUE check on R0 and copies callee->regs[BPF_REG_0] into
caller->regs[BPF_REG_0]:

kernel/bpf/verifier.c:prepare_func_exit() {
    ...
	if (callee->in_callback_fn && !unwinds) {
		if (r0->type != SCALAR_VALUE) {
			verbose(env, "R0 not a scalar value\n");
			return -EACCES;
		}
    ...
	} else {
		for (i = 0; i < nregs; i++) {
			u32 regno = ret_regs[i];

			bpf_diag_mod_begin(env, &caller->regs[regno], &callee->regs[regno],
					   BPF_DIAG_MOD_WRITE);
			caller->regs[regno] = callee->regs[regno];
			bpf_diag_mod_end(env);
		}
	}
    ...
}

Once the unwound pad in bpf_find_vma() runs rcu_read_unlock(),
bpf_mmap_unlock_mm(work, mm), and mmput_async(mm), the caller's unwind pad
receives the plain PTR_TO_BTF_ID in R0 outside the lock and RCU scope. If the
VMA and its backing file or inode are concurrently unmapped and freed, passing
that dangling R0 pointer to helpers accepting ARG_PTR_TO_BTF_ID (such as
bpf_ima_file_hash(), bpf_ima_inode_hash(), bpf_d_path(), or
bpf_inode_storage_get()) can trigger a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=7