* Re: [PATCH bpf] bpf: Fix sanitize_err() register selection for BPF_K
2026-08-03 14:44 [PATCH bpf] bpf: Fix sanitize_err() register selection for BPF_K Yiyang Chen
@ 2026-08-03 14:57 ` sashiko-bot
2026-08-03 15:25 ` bot+bpf-ci
1 sibling, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-08-03 14:57 UTC (permalink / raw)
To: Yiyang Chen; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch modifies `sanitize_err()` to handle `BPF_K` instructions, but this code path is mathematically unreachable because BPF_K instructions bypass ALU sanitation entirely.
--
--- Patch [1]: [PATCH bpf] bpf: Fix sanitize_err() register selection for BPF_K ---
commit 08e8e3769c4c795ad93d26ddc7c02599ba8cab32
Author: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>
bpf: Fix sanitize_err() register selection for BPF_K
This patch modifies sanitize_err() to correctly report the destination
register for BPF_K instructions when generating ALU sanitization errors.
It ensures the source register field, which is zero for immediate
constants, does not interfere with the diagnostic output.
Closes: https://lore.kernel.org/bpf/20260729153536.F3E2E1F000E9@smtp.kernel.org/
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- 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);
[Severity: Medium]
Is this code path actually reachable for BPF_K instructions?
Looking at can_skip_alu_sanitation(), it appears to unconditionally return
true when BPF_SRC(insn->code) == BPF_K:
kernel/bpf/verifier.c:can_skip_alu_sanitation() {
return env->bypass_spec_v1 ||
BPF_SRC(insn->code) == BPF_K ||
cur_aux(env)->nospec;
}
Since sanitize_ptr_alu() and sanitize_val_alu() both check
can_skip_alu_sanitation() first, won't they always return 0 for BPF_K?
If so, this would prevent sanitize_err() from ever being called for these
instructions, making this new logic unreachable.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-c3-035-sanitize-err-v1-v1-1-93e5ff61731f@mails.tsinghua.edu.cn?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf] bpf: Fix sanitize_err() register selection for BPF_K
2026-08-03 14:44 [PATCH bpf] bpf: Fix sanitize_err() register selection for BPF_K Yiyang Chen
2026-08-03 14:57 ` sashiko-bot
@ 2026-08-03 15:25 ` bot+bpf-ci
2026-08-06 17:52 ` Eduard Zingerman
1 sibling, 1 reply; 4+ messages in thread
From: bot+bpf-ci @ 2026-08-03 15:25 UTC (permalink / raw)
To: chenyy23, ast, daniel, andrii, eddyz87, memxor
Cc: john.fastabend, martin.lau, song, yonghong.song, jolsa, emil, bpf,
linux-kernel, sashiko-bot, chenyy23, ast, andrii, daniel,
martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 4764 bytes --]
> 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 <chenyy23@mails.tsinghua.edu.cn>
>
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread