Re: [PATCH bpf-next v10 15/24] bpf, arm64: Dispatch exception cleanup pads at run time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] arm64/bpf: PAC modifier mismatch for bpf_throw's C frame causes FPAC panic
--

commit 2f751c03b321c9d53d97196968f653cf6c3fa289
Author: Yonghong Song <yonghong.song@linux.dev>

bpf, arm64: Dispatch exception cleanup pads at run time

This commit adds arm64 support for unwinding BPF frames and redirecting
their return addresses to cleanup pads. It signs the modified return
addresses using PAC when address authentication is enabled.

> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 3ebcf8c53fb04..56d1310eca323 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c

[ ... ]

> +/* PACIASP's modifier is the entry sp, record + 16 for a BPF prologue. */
> +static void bpf_unwind_store_ra(unsigned long record, u64 ra)
> +{
> +	struct frame_record *rec = (struct frame_record *)record;
> +
> +	/*
> +	 * Whether the slot holds a signed address is a property of the build,
> +	 * not one to be read off the value: a PAC can come out equal to the
> +	 * bits stripping puts back, and a signed address would then be taken
> +	 * for an unsigned one. What signs is CONFIG_ARM64_PTR_AUTH_KERNEL --
> +	 * the prologue here, and -mbranch-protection for everything the
> +	 * compiler emits.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL) &&
> +	    system_supports_address_auth())
> +		ra = bpf_unwind_sign_ra(ra, record + sizeof(struct frame_record));

[Severity: High]
Will this hardcoded PAC modifier cause an FPAC panic when returning from
bpf_throw()?

The unwinder uses bpf_unwind_store_ra() to redirect the return address for
the first BPF frame. The caller of this first BPF frame is the C function
bpf_throw().

While record + sizeof(struct frame_record) correctly assumes a strict BPF
JIT prologue where the entry SP is FP + 16, bpf_throw() is a compiled C
function. Its actual frame size is compiler-dependent and may be larger
than 16 bytes due to local variables.

When bpf_throw() returns, its epilogue will authenticate the return address
using its actual entry SP. If we sign it here using FP + 16 instead of the
actual entry SP, could authentication fail and trigger a kernel panic on
PAC-enabled kernels?

> +	WRITE_ONCE(rec->lr, ra);
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009041447.17092-1-yonghong.song@linux.dev?part=15