Re: [PATCH bpf-next v10 10/24] bpf: Do not use a private stack for a program that can unwind

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: bpf_prog_may_unwind misses might_throw, causing private stack corruption
--

commit 38438f251d7fce91364c50842e5e55c25b6033f4
Author: Yonghong Song <yonghong.song@linux.dev>

bpf: Do not use a private stack for a program that can unwind

This patch aims to prevent BPF programs that can unwind from using a private
stack. A private stack keeps its frame pointer in %r9 on x86-64, and an
unwind skips the restore, leading to stack addressing issues. The fix forces
NO_PRIV_STACK for programs that might unwind.

> diff --git a/kernel/bpf/exception.c b/kernel/bpf/exception.c
> index 2b9640099269e..f45dd3157f7fb 100644
> --- a/kernel/bpf/exception.c
> +++ b/kernel/bpf/exception.c

[ ... ]

> @@ -183,6 +183,16 @@ static void mark_call_sites(struct bpf_verifier_env *env)
>  	}
>  }
>  
> +bool bpf_prog_may_unwind(const struct bpf_verifier_env *env)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < env->subprog_cnt; i++)
> +		if (env->subprog_info[i].might_unwind)

[Severity: High]
Does bpf_prog_may_unwind() miss the might_throw condition?

Since might_unwind and might_throw are tracked separately in
bpf_subprog_info, it appears a BPF program using bpf_throw() would bypass
this check and still be compiled with a private stack.

> +			return true;
> +	return false;
> +}
> +
>  int bpf_exc_check_prog(struct bpf_verifier_env *env)
>  {

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 59b275bfc949d..35013a5d84019 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -5801,6 +5801,17 @@ static int check_max_stack_depth(struct bpf_verifier_env *env)
>  		}
>  	}
>  
> +	/*
> +	 * A private stack keeps its frame pointer in %r9 on x86-64, restored
> +	 * by a pop after the call that an unwind skips. A frame resumed at a
> +	 * pad then addresses its stack through a stale pointer, and a frame
> +	 * sent to its epilogue instead pops its callee-saved registers one
> +	 * slot off. Do not use a private stack for any program that can
> +	 * unwind, on every arch for now.
> +	 */
> +	if (env->cleanup_info_cnt || bpf_prog_may_unwind(env))
> +		priv_stack_mode = NO_PRIV_STACK;

[Severity: High]
Will this leave programs that throw exceptions vulnerable to stack shifting
in check_max_stack_depth()?

If a program uses bpf_throw() and is compiled with a private stack
(PRIV_STACK_ADAPTIVE), the x86 JIT pushes the private frame pointer (%r9)
before every call.

When bpf_throw() is called, the ORC unwinder calculates the boundary frame's
stack pointer as the value of %rsp before the call, which includes the
un-popped %r9 (shifting it by -8 bytes). Then bpf_exception_cb() uses this
value to restore the boundary frame's callee-saved registers.

Because of the unaccounted %r9 push, the stack pointer is off by 8 bytes.
Could this cause pop_callee_regs to pop the stale %r9 into a callee-saved
register, shift all other registers by one slot, and ultimately corrupt the
callee-saved registers?

> +
>  	if (priv_stack_mode == PRIV_STACK_UNKNOWN)
>  		priv_stack_mode = bpf_enable_priv_stack(env->prog);
>

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