Re: [PATCH bpf-next v2 02/22] bpf: Remap the verifier state through the patch list

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3432 bytes --]

> bpf: Remap the verifier state through the patch list

TL;DR: patch_list_adj_aux() may clear indirect_target for single-instruction
patches, so a gotox target could lose its ENDBR/BTI landing pad on IBT/BTI
kernels. Latent for now, as there are no callers yet; the comment and
changelog are also inaccurate.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index f062ef7dd943..0697f66b1ffc 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -380,6 +380,157 @@ struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 off,

[ ... ]

> +/*
> + * Fill the aux data of the 'p->len' insns at 'slot' from the aux data 'old'
> + * of the insn they replace, the same way as adjust_insn_aux_data() does.
> + */
> +static void patch_list_adj_aux(struct bpf_prog *prog, struct bpf_insn_patch *p,
> +			       struct bpf_insn *patch,
> +			       const struct bpf_insn_aux_data *old,
> +			       struct bpf_insn_aux_data *slot)
> +{
> +	struct bpf_insn_aux_data *last = slot + p->len - 1;
> +	struct bpf_insn *orig = &p->orig;
> +	u32 i;
> +
> +	*last = *old;

[ ... ]

> +	/* indirect jumps to the replaced insn land on the first new one */
> +	if (last->indirect_target) {
> +		slot[0].indirect_target = 1;
> +		last->indirect_target = 0;
> +	}
> +}

For a single-instruction patch (p->len == 1), 'last' is 'slot + p->len - 1',
which is 'slot'.  Can this block clear the flag it just set?

    slot[0].indirect_target = 1;
    last->indirect_target = 0;     /* last == &slot[0] */

bpf_patch_list_add() only rejects !len, so len == 1 patches are accepted.
An instruction that is the target of an indirect jump (gotox) would lose
its indirect_target flag when it is replaced one-for-one.

adjust_insn_aux_data() does not behave this way.  For cnt == 1 it takes the
early return and leaves indirect_target alone.

bpf_insn_is_indirect_target() hands this flag to the JITs, and x86 do_jit()
emits ENDBR and arm64 build_insn() emits 'BTI J' only for flagged
instructions.  An indirect jump to an unflagged instruction would then hit
a missing landing pad on IBT/BTI-enabled kernels.

The len == 1 path also treats non_stack_access differently.  It keeps the
old marking, while adjust_insn_aux_data() recomputes it with
bpf_is_mem_insn(insn + off).

So the comment above patch_list_adj_aux() ("the same way as
adjust_insn_aux_data() does") and the commit message line "filling in the
aux data of patched instructions the way adjust_insn_aux_data() does" do
not hold for single-instruction patches.

This looks latent in this commit.  bpf_patch_list_add() and
bpf_patch_list_commit() have no callers here, and in the series the next
commit a3a12f55a1ad ("bpf: Apply single-instruction patches of the verifier
in place") applies len == 1 patches in place before any caller exists.  The
first callers arrive in a9b03b9b8596 ("bpf: Queue the rewrites of
bpf_do_misc_fixups()"), and 3c859182c47c ("bpf: Teach the patch list to
insert in front of an instruction") later adds a 'p->target != p->len - 1'
guard to this block.

Would it make sense to fold a3a12f55a1ad into this commit, so the helper is
correct on its own and its comment and the changelog are accurate?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41053659762