Re: [PATCH bpf-next v1 05/29] bpf: Restore callee-saved registers from unwound frames

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] bpf: NULL pointer dereference in bpf_throw() [1]
--

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

bpf: Restore callee-saved registers from unwound frames

This commit restores callee-saved registers from unwound frames during
bpf_throw() unwinding and passes them to the exception callback instead of
saving and popping all callee-saved registers in the exception boundary
prologue.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index de31fdd18411e..0164f183aade2 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
[ ... ]
> @@ -3461,15 +3474,24 @@ static bool bpf_stack_walker(void *cookie, u64 ip, u64 sp, u64 bp)
>  }
>  
>  /*
> - * Unwind the stack to the exception callback of the main program. @regs holds
> - * the callee-saved registers of the throw site, or is NULL when the
> - * architecture has no native entry and the kfunc was called directly.
> + * Unwind the stack to the exception callback of the main program. @regs is the
> + * register file of the throw site, which the entry of the architecture
> + * captured: the callee-saved registers of the frames unwound are restored from
> + * it for the callback. It is NULL when the architecture has no native entry
> + * and the kfunc was called directly.
>   */
>  __bpf_unwind_entry
>  notrace void bpf_throw_unwind(struct bpf_throw_regs *regs, u64 cookie)
>  {
>  	struct bpf_throw_ctx ctx = {};
>  
> +	/*
> +	 * Without the file, on an architecture that has no native entry, the
> +	 * prologue of the main program has saved every callee-saved register
> +	 * and the callback restores them from its frame, not from @saved.
> +	 */
> +	if (regs)
> +		memcpy(ctx.saved, &regs->r[BPF_REG_6], sizeof(ctx.saved));
>  	arch_bpf_stack_walk(bpf_stack_walker, &ctx);
>  	WARN_ON_ONCE(!ctx.aux);
>  	if (ctx.aux)
> @@ -3483,7 +3505,7 @@ notrace void bpf_throw_unwind(struct bpf_throw_regs *regs, u64 cookie)
>  	 * architectures cannot recover sp while unwinding, so fall back to bp.
>  	 */
>  	kasan_unpoison_task_stack_below((void *)(long)(ctx.sp ?: ctx.bp));
> -	ctx.aux->bpf_exception_cb(cookie, ctx.sp + ctx.aux->stack_arg_sp_adjust, ctx.bp, 0, 0);
> +	ctx.aux->bpf_exception_cb(cookie, ctx.sp, ctx.bp, (u64)(long)ctx.saved, 0);

[Severity: Critical]
This problem wasn't introduced by this patch, but can this cause a NULL
pointer dereference [1] on ctx.aux if arch_bpf_stack_walk() fails to
locate the main BPF program frame?

Although the code checks whether ctx.aux is non-NULL before testing
ctx.aux->exception_boundary above, it then dereferences ctx.aux
unconditionally when invoking bpf_exception_cb().

[1] https://sashiko.dev/#/bug/linux-ba8dbe22-2e39-4c7e-90bf-e30065ad3c90

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