Re: [PATCH bpf-next v1 06/29] bpf: Keep last insn's aux data in add_hidden_subprog()

[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: misplaced nospec flag in adjust_insn_aux_data() [1]
--

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

bpf: Keep last insn's aux data in add_hidden_subprog()

This commit swaps the aux data of the original last instruction and the end
of the patch in add_hidden_subprog() so that the original instruction keeps
its aux data (while leaving indirect_target on the first instruction).

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index eb5a5bca5c8d5..8597b443d5619 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -2280,8 +2280,10 @@ static int add_hidden_subprog(struct bpf_verifier_env *env, struct bpf_insn *pat
>  {
>  	struct bpf_subprog_info *info = env->subprog_info;
>  	int err, cnt = env->subprog_cnt;
> +	struct bpf_insn_aux_data *aux;
>  	struct bpf_insn *last;
>  	struct bpf_prog *prog;
> +	bool indirect;
>  
>  	/* We only reserve one slot for hidden subprogs in subprog_info. */
>  	if (env->hidden_subprog_cnt) {
> @@ -2299,6 +2301,17 @@ static int add_hidden_subprog(struct bpf_verifier_env *env, struct bpf_insn *pat
>  	if (err)
>  		return err;
>  	prog = env->prog;
> +	/*
> +	 * The patch starts with the original last insn, but the commit moved that
> +	 * insn's aux data to the end of the patch: give it back to the insn. All
> +	 * but indirect_target, which the commit left with the insn, since an
> +	 * indirect jump to it lands there still, see patch_list_adj_aux().
> +	 */
> +	aux = &env->insn_aux_data[prog->len - len];
> +	indirect = aux->indirect_target;
> +	swap(aux[0], aux[len - 1]);
> +	aux[0].indirect_target = indirect;
> +	aux[len - 1].indirect_target = false;
>  	info[cnt + 1].start = info[cnt].start;
>  	info[cnt].start = prog->len - len + 1;
>  	/* The patch ends with the exit. */

[Severity: Critical]
This problem wasn't introduced by this patch, but can patch_list_adj_aux()
misplace the nospec flag [1] when expanding a single instruction into a
multi-instruction sequence?

kernel/bpf/fixups.c:patch_list_adj_aux() {
	...
	*last = *old;
	/* Recompute zext_dst: the last insn of the patch need not be the patched one. */
	last->zext_dst = bpf_insn_def32(prog, &patch[p->len - 1]) >= 0;
	memset(slot, 0, sizeof(*slot) * (p->len - 1));
	...
	/* indirect jumps to the replaced insn land on p->target */
	if (last->indirect_target && p->target != p->len - 1) {
		slot[p->target].indirect_target = 1;
		last->indirect_target = 0;
	}
}

During verification with Spectre v1 mitigation active, a bpf_loop() call can
be eligible for inlining on an architectural path while a speculative path to
the same instruction fails in check_func_args() with -EACCES, causing
do_check() to set insn_aux->nospec on the call instruction and stop verifying
that speculative path.

When bpf_optimize_bpf_loop() -> inline_bpf_loop() later replaces the call
with a 19-instruction sequence, patch_list_adj_aux() moves old aux data to
last and zeroes the preceding entries while only restoring indirect_target to
slot[p->target].  As a result, bpf_convert_ctx_accesses() emits BPF_ST_NOSPEC()
before the last instruction of the inlined loop instead of at its entry,
allowing speculative execution to enter the loop body and invoke the callback
with unverified state.

[1] https://sashiko.dev/#/bug/linux-9fe39327-eb21-4b95-9330-3ea552a3b9fc

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