Re: [PATCH bpf-next v10 11/24] bpf: Prepare JITed programs for dispatching cleanup pads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: backward jump offsets in bpf_exc_patch_unwind_calls are corrupted by bpf_adj_branches
--

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

bpf: Prepare JITed programs for dispatching cleanup pads

This patch prepares JITed programs to dispatch exception cleanup pads.
It updates the verifier to patch bpf_unwind() calls with a sequence that
clears r0 and jumps to the exception pad or exits.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 64baee1a37e60..1cc025c45ecda 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
[ ... ]
> @@ -1733,6 +1805,48 @@ static int may_goto_expand(struct bpf_insn *insn_buf, int off, int stack_off,
[ ... ]
> +/*
> + * Follow each bpf_unwind() call with 'r0 = 0; exit', or with
> + * 'r0 = 0; goto pad' where a record covers the call.
> + */
> +int bpf_exc_patch_unwind_calls(struct bpf_verifier_env *env)
> +{
> +	int insn_cnt = env->prog->len;
> +	struct bpf_insn insn_buf[3];
> +	struct bpf_prog *new_prog;
> +	int i, off, delta = 0;
> +
> +	if (!bpf_prog_may_unwind(env))
> +		return 0;
> +
> +	for (i = 0; i < insn_cnt; i++) {
> +		int call = i + delta, pad;
> +		struct bpf_insn *insn = env->prog->insnsi + call;
> +
> +		if (!bpf_is_unwind_kfunc(insn))
> +			continue;
> +
> +		insn_buf[0] = *insn;
> +		insn_buf[1] = BPF_MOV64_IMM(BPF_REG_0, 0);
> +		insn_buf[2] = BPF_EXIT_INSN();
> +		pad = bpf_exc_pad_of_call(env, call);
> +		if (pad >= 0) {
> +			/* The goto is at call + 2; a pad after the call moves down by 2. */
> +			if (pad > call)
> +				pad += 2;
> +			off = pad - (call + 3);
> +			insn_buf[2] = off == (s16)off ? BPF_JMP_A(off) : BPF_JMP32_A(off);

[Severity: High]
Does this manually calculated offset get corrupted if the landing pad is located
before the unwind call (pad < call)?

bpf_patch_insn_data() is called immediately after this, which delegates to
bpf_adj_branches() to adjust all relative offsets in the program.

When bpf_adj_branches() processes the newly inserted goto instruction, it
evaluates it as if it were a shifted original instruction (because
curr >= end_new evaluates to true). If the jump is backwards,
bpf_adj_branches() might erroneously subtract the insertion delta (2) from the
already-correct offset.

Could this result in the jump target being shifted 2 instructions before the
actual landing pad, potentially leading to the execution of unintended or
unverified bytecode during exception unwinding?

> +		}
> +
> +		new_prog = bpf_patch_insn_data(env, call, insn_buf, 3);
> +		if (!new_prog)
> +			return -ENOMEM;
> +		delta += 2;
> +		env->prog = new_prog;
> +	}
> +	return 0;
> +}

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