Re: [PATCH bpf-next v2 01/35] bpf: Add may_goto to the loops that the verifier marks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: unadjusted BPF_JMP32 | BPF_JA self-jump in bpf_insert_loop_guards()
- [Low] bpf: bpf_insert_loop_guards() logs shifted insn index instead of orig_idx
--

commit 9383027921f2fdc75a3e30a579a0417926be85e9
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Add may_goto to the loops that the verifier marks

This commit introduces bpf_insert_loop_guards() to prepend may_goto
instructions to loops marked by the verifier, along with per-subprogram
budget exit sequences when the guard unwinds or exits the subprogram.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index f95af2ca3591b..6fd79c2fbdc1e 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -587,6 +587,225 @@ static int bpf_adj_moved_insn(struct bpf_insn *insn, u32 delta)
>  	return 0;
>  }
>  
> +static bool jumps_to_itself(const struct bpf_insn *insn)
> +{
> +	u8 class = BPF_CLASS(insn->code), op = BPF_OP(insn->code);
> +
> +	return (class == BPF_JMP || class == BPF_JMP32) && op != BPF_CALL && op != BPF_EXIT &&
> +	       insn->code != (BPF_JMP32 | BPF_JA) && insn->off == -1;
> +}

[ ... ]

> +int bpf_insert_loop_guards(struct bpf_verifier_env *env)
> +{

[ ... ]

> +	for (s = 0; s < env->subprog_cnt && !err; s++) {
> +		leave[s] = append_budget_exit(env, s);
> +		if (leave[s] < 0)
> +			err = leave[s];
> +	}
> +	/* A guard cannot jump into a patch that is in the list: 'r0 = 0' has no index yet. */
> +	err = err ?: bpf_patch_list_commit(env);

[ ... ]

> +	for (i = 0; i < len; i++) {
> +		to = target[i];
> +		if (to == -1)
> +			continue;

[ ... ]

> +		if (off > S16_MAX || off < S16_MIN) {
> +			verbose(env, "exit of the loop is too far from insn %d for may_goto\n", i);
> +			err = -ERANGE;
> +			break;
> +		}
> +		patch[jmp] = BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, off, 0);
> +		if (to == -2)
> +			patch[jmp] = budget_exit_insn(env, bpf_find_containing_subprog(env, i));
> +		patch[n] = env->prog->insnsi[i];
> +		/* A jump to itself is a back edge. It goes through may_goto like the others. */
> +		if (jumps_to_itself(&patch[n]))
> +			patch[n].off -= n;
> +		else
> +			err = bpf_adj_moved_insn(&patch[n], n);

[Severity: High]
What happens here when the guarded instruction at patch[n] is a 32-bit
unconditional jump to itself (BPF_JMP32 | BPF_JA with imm == -1)?

In jumps_to_itself(), BPF_JMP32 | BPF_JA is excluded and only insn->off == -1
is checked, so jumps_to_itself(&patch[n]) returns false. Control then passes
to bpf_adj_moved_insn(&patch[n], n):

kernel/bpf/fixups.c:bpf_adj_moved_insn() {
	...
	} else if ((class == BPF_JMP || class == BPF_JMP32) &&
		   op != BPF_CALL && op != BPF_EXIT) {
		if (insn->code != (BPF_JMP32 | BPF_JA)) {
			off = insn->off;
			off_min = S16_MIN;
			is_imm = false;
		}
		/* jump to itself stays */
		if (off >= -1)
			return 0;
	...
}

Because off is -1, bpf_adj_moved_insn() returns 0 without subtracting n from
patch[n].imm, and patch_list_adj_insn() during bpf_patch_list_commit() also
leaves it unchanged because the target n + (-1) + 1 = n is within the patch.

Wouldn't that leave patch[n].imm as -1 so that at runtime execution passes
through the prepended may_goto once and then spins forever on patch[n]
itself, bypassing the loop guard? And when adjusting a self-jumping
BPF_JMP32 | BPF_JA, shouldn't n be subtracted from patch[n].imm rather than
patch[n].off?

> +		if (err) {
> +			verbose(env, "insn %d cannot be patched due to 16-bit range\n", i);
> +			break;
> +		}

[Severity: Low]
Should these two verbose() calls in bpf_insert_loop_guards() log
aux[i].orig_idx instead of i, as bpf_patch_list_insert(),
bpf_patch_list_commit(), and bpf_convert_ctx_accesses() do?

Before this loop runs, the first pass in bpf_insert_loop_guards() calls
append_budget_exit() and commits those patches via bpf_patch_list_commit().
Any subprogram that received a budget exit has 2 or 3 instructions appended,
so i in subsequent subprograms is shifted relative to the original program.

Also, in kernel/bpf/verifier.c:bpf_check():

	if (ret == 0)
		ret = bpf_insert_loop_guards(env);

skip_full_check:
	kvfree(env->explored_states);

	/* might decrease stack depth, keep it before passes that
	 * allocate additional slots.
	 */
	if (ret == 0)
		ret = bpf_remove_fastcall_spills_fills(env);

	if (ret == 0)
		ret = check_max_stack_depth(env);

	/* instruction rewrites happen after this point */

Calling bpf_insert_loop_guards() before check_max_stack_depth() rewrites
instructions before check_exception_cb(), check_recursion(), and
check_callbacks(), which log raw instruction indices from env->prog->insnsi.
Should bpf_insert_loop_guards() be placed after check_max_stack_depth() with
the other instruction rewrites?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105204513.2230344-1-alexei.starovoitov@gmail.com?part=1