Re: [RFC PATCH bpf-next v1 01/11] bpf: Give a helper a way to call its callback with a pad

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: clobber of local register variables during arg evaluation in __bpf_call_cb()
- [High] bpf: dead-store elimination of return address write in bpf_unwind_leave()
--

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

bpf: Give a helper a way to call its callback with a pad

This commit introduces bpf_call_cb() and bpf_unwind_leave() in
include/linux/bpf_unwind.h, along with bpf_unwind_target() in the x86-64
and arm64 JITs, so C helpers invoking BPF callbacks can catch callback
unwinds via an EX_TYPE_BPF exception table entry and redirect their own
return address to the caller's unwind pad.

> diff --git a/include/linux/bpf_unwind.h b/include/linux/bpf_unwind.h
> new file mode 100644
> index 0000000000000..190d6dbae8568
> --- /dev/null
> +++ b/include/linux/bpf_unwind.h
> @@ -0,0 +1,282 @@

[ ... ]

> +#define __bpf_call_cb(type, fn, a1, a2, a3, a4, a5, r0, unwound)		\
> +do {										\
> +	register u64 __fn asm("r11") = (u64)(long)(fn);				\
> +	register u64 __a5 asm("r8") = (u64)(long)(a5);				\
> +	register u64 __c5 asm("r8");						\
> +	register u64 __c11 asm("r11");						\
> +	u64 __c1, __c2, __c3, __c4;						\
> +										\
> +	asm_goto_output(BPF_KCFI_CHECK(type)					\
> +		 "	" CALL_NOSPEC "\n"					\
> +		 "1:\n"								\
> +		 _ASM_EXTABLE_TYPE(1b, %l[unwound], EX_TYPE_BPF)		\
> +		 : "=a"(r0), "=D"(__c1), "=S"(__c2), "=d"(__c3), "=c"(__c4),	\
> +		   "=r"(__c5), "=r"(__c11)					\
> +		 : [thunk_target] "r"(__fn), "D"((u64)(long)(a1)),		\
> +		   "S"((u64)(long)(a2)), "d"((u64)(long)(a3)),			\
> +		   "c"((u64)(long)(a4)), "r"(__a5)				\
> +		 : "r9", "r10", "memory", "cc"					\
> +		 : unwound);							\
> +} while (0)
> +
> +/*
> + * The return address of the helper is the word below its canonical frame
> + * address. The pad is in the frame that BPF code called, see above, so there is
> + * a pad to go to: a miss is a bug, and the exception would be lost in a return.
> + */
> +static __always_inline void bpf_unwind_leave(void)
> +{
> +	unsigned long *slot = (unsigned long *)__builtin_dwarf_cfa() - 1;
> +	unsigned long to = bpf_unwind_target(*slot);
> +
> +	WARN_ON_ONCE(to == *slot);
> +	*slot = to;
> +}

[ ... ]

> +#define __bpf_call_cb(type, fn, a1, a2, a3, a4, a5, r0, unwound)		\
> +	___bpf_call_cb(__UNIQUE_ID(unwound), type, fn, a1, a2, a3, a4, a5, r0, unwound)
> +
> +#define ___bpf_call_cb(lbl, type, fn, a1, a2, a3, a4, a5, r0, unwound)		\
> +do {										\
> +	register u64 __fn asm("x15") = (u64)(long)(fn);				\
> +	register u64 __a0 asm("x0") = (u64)(long)(a1);				\
> +	register u64 __a1 asm("x1") = (u64)(long)(a2);				\
> +	register u64 __a2 asm("x2") = (u64)(long)(a3);				\
> +	register u64 __a3 asm("x3") = (u64)(long)(a4);				\
> +	register u64 __a4 asm("x4") = (u64)(long)(a5);				\

[Severity: High]
Can evaluating the macro arguments in ___bpf_call_cb() on arm64 and
__bpf_call_cb() on x86-64 clobber the caller-saved local register
variables before the asm_goto_output() statement is reached?

On arm64, ___bpf_call_cb() in include/linux/bpf_unwind.h initializes x15
and x0..x4 directly from the macro expressions in sequence:

	register u64 __fn asm("x15") = (u64)(long)(fn);
	register u64 __a0 asm("x0") = (u64)(long)(a1);
	register u64 __a1 asm("x1") = (u64)(long)(a2);
	register u64 __a2 asm("x2") = (u64)(long)(a3);
	register u64 __a3 asm("x3") = (u64)(long)(a4);
	register u64 __a4 asm("x4") = (u64)(long)(a5);

And on x86-64, __bpf_call_cb() assigns __fn (r11) and __a5 (r8) before
evaluating a1..a4 in the asm_goto_output() input operands:

	register u64 __fn asm("r11") = (u64)(long)(fn);
	register u64 __a5 asm("r8") = (u64)(long)(a5);
	...
		 : [thunk_target] "r"(__fn), "D"((u64)(long)(a1)),
		   "S"((u64)(long)(a2)), "d"((u64)(long)(a3)),
		   "c"((u64)(long)(a4)), "r"(__a5)

Per GCC and Clang local register variable rules, local register variables
do not reserve caller-saved registers across function calls that occur
between their assignment and the extended asm statement.

Later in this series, commit "bpf: Let the callback of
bpf_for_each_map_elem unwind" updates bpf_each_rhash_elem() in
kernel/bpf/hashtab.c to pass a helper call and pointer dereferences as
macro arguments:

	bpf_call_cb(callback_fn, map, elem->data,
		    rhtab_elem_value(elem, map->key_size), callback_ctx, 0,
		    ret, unwound);

If evaluating a3 or another argument invokes a function call -- such as an
out-of-line call to rhtab_elem_value() (which is marked static inline
rather than __always_inline) or sanitizer instrumentation like
__asan_load4() under outline KASAN or __tsan_read4() under KCSAN -- that
call executes after __fn (x15 / r11) and earlier argument registers (x0,
x1 / r8) have already been populated, clobbering the callback target and
arguments prior to blr x15 or CALL_NOSPEC.

Should fn and a1..a5 be evaluated into ordinary C temporary variables
first before assigning them to the local register variables, similar to
how include/linux/arm-smccc.h stages register arguments?

> +	register u64 __c0 asm("x0");						\
> +	register u64 __c1 asm("x1");						\
> +	register u64 __c2 asm("x2");						\
> +	register u64 __c3 asm("x3");						\
> +	register u64 __c4 asm("x4");						\
> +	register u64 __c15 asm("x15");						\

[ ... ]

> +static __always_inline void bpf_unwind_leave(void)
> +{
> +	unsigned long *slot = (unsigned long *)__builtin_frame_address(0) + 1;
> +	unsigned long from = ptrauth_strip_kernel_insn_pac(*slot);
> +	unsigned long to = bpf_unwind_target(from);
> +
> +	WARN_ON_ONCE(to == from);
> +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
> +	{
> +		register unsigned long x17 asm("x17") = to;
> +#ifdef __clang__
> +		register unsigned long x16 asm("x16") = (unsigned long)__builtin_sponentry();
> +#else
> +		register unsigned long x16 asm("x16") = (unsigned long)__builtin_dwarf_cfa();
> +#endif
> +
> +		asm("pacia1716" : "+r"(x17) : "r"(x16));
> +		to = x17;
> +	}
> +#endif
> +	*slot = to;
> +#ifdef CONFIG_SHADOW_CALL_STACK
> +	if (scs_is_enabled()) {
> +		unsigned long *scs;
> +
> +		asm("mov %0, x18" : "=r"(scs));
> +		scs[-1] = to;
> +	}
> +#endif
> +}

[Severity: High]
Could the compiler eliminate the *slot = to store in bpf_unwind_leave()
(on both x86-64 and arm64) as a dead store to the current function's stack
frame right before returning?

In include/linux/bpf_unwind.h, both implementations of bpf_unwind_leave()
compute slot from __builtin_dwarf_cfa() or __builtin_frame_address(0) as a
non-volatile unsigned long * and perform a plain store:

	*slot = to;

Because bpf_unwind_leave() is __always_inline and runs at the end of the
helper's unwound path immediately before return, nothing in C reads *slot
before the function exits. In GCC's RTL dead-store elimination pass
(pass_rtl_dse1, which runs before prologue/epilogue threading),
non-volatile frame-related stores based on arg_pointer_rtx
(__builtin_dwarf_cfa()) or hard_frame_pointer_rtx
(__builtin_frame_address(0)) that are not read before EXIT_BLOCK are
candidates for elimination. If *slot = to is elided, the helper returns to
its normal call site instead of the caller's unwind pad, losing the BPF
exception after the helper has already released its resources.

Existing kernel code that overwrites the stack return address via
__builtin_frame_address(0) in drivers/misc/lkdtm/cfi.c:set_return_addr()
explicitly marks the store volatile for this reason:

	/* Use of volatile is to make sure final write isn't seen as a dead store. */
	unsigned long * volatile *ret_addr =
		(unsigned long **)__builtin_frame_address(0) + FRAME_RA_OFFSET;

	/* Make sure we've found the right place on the stack before writing it. */
	if (no_pac_addr(*ret_addr) == expected)
		*ret_addr = (addr);

Should bpf_unwind_leave() use WRITE_ONCE(*slot, to) (and
WRITE_ONCE(scs[-1], to) on arm64) or qualify slot as volatile so the
return-address update cannot be optimized away?

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