Re: [PATCH bpf] bpf: Fix sanitize_err() register selection for BPF_K

--===============5234048644432632214==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fdc5fbb1f78c..e5a8547ad723 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13562,6 +13562,7 @@ static int sanitize_err(struct bpf_verifier_env *env, const struct bpf_insn *ins
>  	static const char *err = "pointer arithmetic with it prohibited for !root";
>  	const char *op = BPF_OP(insn->code) == BPF_ADD ? "add" : "sub";
>  	u32 dst = insn->dst_reg, src = insn->src_reg;
> +	bool src_is_imm = BPF_SRC(insn->code) == BPF_K;
>  	struct bpf_reg_state *regs = cur_regs(env);
>
>  	switch (reason) {
> @@ -13571,7 +13572,7 @@ static int sanitize_err(struct bpf_verifier_env *env, const struct bpf_insn *ins
>  		break;
>  	case REASON_TYPE:
>  		verbose(env, "R%d has pointer with unsupported alu operation, %s\n",
> -			regs[src].type == SCALAR_VALUE ? dst : src, err);
> +			src_is_imm || regs[src].type == SCALAR_VALUE ? dst : src, err);
>  		break;
>  	case REASON_PATHS:
>  		verbose(env, "R%d tried to %s from different maps, paths or scalars, %s\n",

Can this code path be reached for BPF_K instructions? The new src_is_imm
check appears to be unreachable.

All three call sites of sanitize_err() (at verifier.c:13765, :13858 and
:14610) fire only when sanitize_ptr_alu() or sanitize_val_alu() return a
negative REASON_* code. Both of those functions begin with:

static bool can_skip_alu_sanitation(const struct bpf_verifier_env *env,
                                    const struct bpf_insn *insn)
{
        return env->bypass_spec_v1 ||
                BPF_SRC(insn->code) == BPF_K ||
                cur_aux(env)->nospec;
}

and check this at their entry:

  sanitize_val_alu()  verifier.c:13415:  if (can_skip_alu_sanitation(env, insn)) return 0;
  sanitize_ptr_alu()  verifier.c:13469:  if (can_skip_alu_sanitation(env, insn)) return 0;

Every REASON_* return in both functions is downstream of that gate, so for
BPF_K instructions they return 0 early. This means sanitize_err() never
executes for BPF_K, making src_is_imm always false inside that function.

The change adds a variable and an always-false disjunct to the conditional,
but produces identical output for all reachable cases. Note also that the
sibling REASON_BOUNDS arm two lines above uses the same regs[src] test and
was left untouched.

If this is defensive hardening against a future change that removes the
BPF_K gate, should that be stated explicitly? Otherwise the code and commit
message imply a functional fix when the behavior is unchanged.

> commit cd52f137178ae1ebdcb1abceac8ccda767f69fe2
> Author: Yiyang Chen <[email protected]>
>
>     bpf: Fix sanitize_err() register selection for BPF_K
>
>     sanitize_err() determines whether the source or destination register holds
>     the pointer by inspecting regs[insn->src_reg]. For BPF_K instructions,
>     src_reg is zero because the offset is encoded as an immediate. R0 must
>     therefore not influence the register reported for REASON_TYPE.
>
>     Treat BPF_K as an immediate scalar source and select the destination
>     register for the diagnostic.
>
>     Fixes: a15970d916b3 ("bpf: Simplify sanitize_err() signature")

Does the Fixes: tag accurately describe a bug that existed in a15970d916b3?

The format of the tag is correct: SHA-1 a15970d916b3 is 12 hex chars,
exists and is reachable, and the subject matches exactly. However, the tag
asserts a reachable defect that the code analysis above contradicts.

The commit a15970d916b3 replaced the off_reg == dst_reg comparison with
regs[src].type == SCALAR_VALUE, and its changelog states: "regs[src].type
is safe to access, as it is not mutated by the callers." That commit did
not introduce a reachable misreport, because sanitize_err() was already
unreachable for BPF_K via can_skip_alu_sanitation() both before and after
it.

The scenario the current commit message describes (a pointer in R0 being
selected as the reported register for a BPF_K instruction) cannot occur
because sanitize_err() is never called for BPF_K instructions. Per the
reachability analysis, the emitted verifier message is byte-for-byte
identical before and after this patch for every possible input program.

If this patch is intended as hardening for a future relaxation of the BPF_K
gate, stating that explicitly and dropping the Fixes: tag would more
accurately describe the change. Otherwise the tag attributes a bug that
cannot be triggered and may cause this to be backported as a functional fix
when it has no runtime effect.


---
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/30825118624
--===============5234048644432632214==--