Re: [PATCH bpf-next v1 02/29] bpf: Add bpf_throw_unwind() and an arch entry hook

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] bpf: missing __bpf_unwind_entry and notrace on non-returning
  bpf_throw()
- [Medium] bpf: missing noinline and __used/__retain/__noclone on
  bpf_throw_unwind()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] bpf: unprotected dentry mutation via static kfunc rewrite
  in specialize_kfunc() [1]
--

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

bpf: Add bpf_throw_unwind() and an arch entry hook

This commit moves the stack-unwinding body of bpf_throw() into
bpf_throw_unwind() and adds a weak bpf_arch_throw_entry() hook so an
architecture can capture callee-saved registers before unwinding.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 937a0d7d68390..de31fdd18411e 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -3460,7 +3460,13 @@ static bool bpf_stack_walker(void *cookie, u64 ip, u64 sp, u64 bp)
>  	return false;
>  }
>  
> -__bpf_kfunc void bpf_throw(u64 cookie)
> +/*
> + * 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.
> + */
> +__bpf_unwind_entry
> +notrace void bpf_throw_unwind(struct bpf_throw_regs *regs, u64 cookie)
>  {
>  	struct bpf_throw_ctx ctx = {};

[Severity: Medium]
Should bpf_throw_unwind() keep the noinline, __used, __retain, and
__noclone attributes that bpf_throw() had via __bpf_kfunc?

Without noinline, the compiler can inline bpf_throw_unwind(NULL, cookie)
into its same-file caller bpf_throw(), running the unwinding body inside a
function that is neither __noscs nor notrace.  And on architectures without
an assembly caller to bpf_throw_unwind(), LTO, --gc-sections, or IPA-SRA can
drop or clone the standalone symbol so resolve_btfids fails on
BTF_ID(func, bpf_throw_unwind) in noreturn_deny.

> @@ -3481,6 +3487,11 @@ __bpf_kfunc void bpf_throw(u64 cookie)
>  	WARN(1, "A call to BPF exception callback should never return\n");
>  }
>  
> +__bpf_kfunc void bpf_throw(u64 cookie)
> +{
> +	bpf_throw_unwind(NULL, cookie);
> +}

[Severity: High]
Does bpf_throw() also need __bpf_unwind_entry and notrace?

Whenever bpf_arch_throw_entry() returns NULL, specialize_kfunc() leaves
calls to bpf_throw() targeting this C wrapper, which never returns either.
Under CONFIG_FRAME_POINTER=y (-fno-optimize-sibling-calls), bpf_throw()
cannot tail-call bpf_throw_unwind(), so on arm64 with shadow call stack
enabled its prologue still pushes a return address onto the shadow call
stack that is never popped, corrupting the return address of the BPF
program's caller.  Likewise, without notrace, function_graph tracing on
bpf_throw() pushes an entry onto current->ret_stack that is never popped on
unwind.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 20b1b9ff574c7..c5b4025ee2223 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -24793,6 +24795,11 @@ static int specialize_kfunc(struct bpf_verifier_env *env, struct bpf_kfunc_desc
>  	} else if (func_id == special_kfunc_list[KF_bpf_arena_free_pages]) {
>  		if (env->insn_aux_data[insn_idx].non_sleepable)
>  			addr = (unsigned long)bpf_arena_free_pages_non_sleepable;
> +	} else if (func_id == special_kfunc_list[KF_bpf_throw]) {
> +		void *entry = bpf_arch_throw_entry();
> +
> +		if (entry)
> +			addr = (unsigned long)entry;
>  	}
>  	desc->addr = addr;
>  	return 0;

[Severity: Critical]
This problem wasn't introduced by this patch, but can specialize_kfunc()
cause an unlocked dentry xattr mutation [1] when rewriting
KF_bpf_set_dentry_xattr and KF_bpf_remove_dentry_xattr?

kernel/bpf/verifier.c:specialize_kfunc() {
    ...
	} else if (func_id == special_kfunc_list[KF_bpf_set_dentry_xattr]) {
		if (bpf_lsm_has_d_inode_locked(prog))
			addr = (unsigned long)bpf_set_dentry_xattr_locked;
	} else if (func_id == special_kfunc_list[KF_bpf_remove_dentry_xattr]) {
		if (bpf_lsm_has_d_inode_locked(prog))
			addr = (unsigned long)bpf_remove_dentry_xattr_locked;
    ...
}

The rewrite checks only whether the LSM hook holds i_rwsem on the hook's
own dentry, not whether the dentry argument passed to the kfunc is that
same dentry.  If a program attached to such a hook passes an unrelated
dentry (for example, obtained via bpf_task_get_open_file()), the _locked
variant runs __vfs_setxattr() or __vfs_removexattr() on that dentry without
holding its inode's i_rwsem.

[1] https://sashiko.dev/#/bug/linux-1f1b04a2-315c-40c0-91e1-333f3c841441

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