This small patchset is about avoid verifier bug warning when conditional jumps on same register when the register holds a scalar with range.
--- KaFai Wan (2): bpf: Skip bounds adjustment for conditional jumps on same register selftests/bpf: Add test for conditional jumps on same register
kernel/bpf/verifier.c | 4 ++++ .../selftests/bpf/progs/verifier_bounds.c | 17 +++++++++++++++++ 2 files changed, 21 insertions(+)
When conditional jumps are performed on the same register (e.g., r0 <= r0, r0 > r0, r0 < r0) where the register holds a scalar with range, the verifier incorrectly attempts to adjust the register's min/max bounds. This leads to invalid range bounds and triggers a BUG warning:
verifier bug: REG INVARIANTS VIOLATION (true_reg1): range bounds violation u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0] var_off=(0x0, 0x0) WARNING: CPU: 0 PID: 93 at kernel/bpf/verifier.c:2731 reg_bounds_sanity_check+0x163/0x220 Modules linked in: CPU: 0 UID: 0 PID: 93 Comm: repro-x-3 Tainted: G W 6.18.0-rc1-ge7586577b75f-dirty #218 PREEMPT(full) Tainted: [W]=WARN Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 RIP: 0010:reg_bounds_sanity_check+0x163/0x220 Call Trace: <TASK> reg_set_min_max.part.0+0x1b1/0x360 check_cond_jmp_op+0x1195/0x1a60 do_check_common+0x33ac/0x33c0 ...
The issue occurs in reg_set_min_max() function where bounds adjustment logic is applied even when both registers being compared are the same. Comparing a register with itself should not change its bounds since the comparison result is always known (e.g., r0 == r0 is always true, r0 < r0 is always false).
Fix this by adding an early return in reg_set_min_max() when false_reg1 and false_reg2 point to the same register, skipping the unnecessary bounds adjustment that leads to the verifier bug.
Reported-by: Kaiyan Mei M202472210@hust.edu.cn Reported-by: Yinhao Hu dddddd@hust.edu.cn Closes: https://lore.kernel.org/all/1881f0f5.300df.199f2576a01.Coremail.kaiyanm@hust... Fixes: 0df1a55afa83 ("bpf: Warn on internal verifier errors") Signed-off-by: KaFai Wan kafai.wan@linux.dev --- kernel/bpf/verifier.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6d175849e57a..420ad512d1af 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -16429,6 +16429,10 @@ static int reg_set_min_max(struct bpf_verifier_env *env, if (false_reg1->type != SCALAR_VALUE || false_reg2->type != SCALAR_VALUE) return 0;
+ /* If conditional jumps on the same register, skip the adjustment */ + if (false_reg1 == false_reg2) + return 0; + /* fallthrough (FALSE) branch */ regs_refine_cond_op(false_reg1, false_reg2, rev_opcode(opcode), is_jmp32); reg_bounds_sync(false_reg1);
On 10/22/25 9:44 AM, KaFai Wan wrote:
When conditional jumps are performed on the same register (e.g., r0 <= r0, r0 > r0, r0 < r0) where the register holds a scalar with range, the verifier incorrectly attempts to adjust the register's min/max bounds. This leads to invalid range bounds and triggers a BUG warning:
verifier bug: REG INVARIANTS VIOLATION (true_reg1): range bounds violation u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0] var_off=(0x0, 0x0) WARNING: CPU: 0 PID: 93 at kernel/bpf/verifier.c:2731 reg_bounds_sanity_check+0x163/0x220 Modules linked in: CPU: 0 UID: 0 PID: 93 Comm: repro-x-3 Tainted: G W 6.18.0-rc1-ge7586577b75f-dirty #218 PREEMPT(full) Tainted: [W]=WARN Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 RIP: 0010:reg_bounds_sanity_check+0x163/0x220 Call Trace:
<TASK> reg_set_min_max.part.0+0x1b1/0x360 check_cond_jmp_op+0x1195/0x1a60 do_check_common+0x33ac/0x33c0 ...
The issue occurs in reg_set_min_max() function where bounds adjustment logic is applied even when both registers being compared are the same. Comparing a register with itself should not change its bounds since the comparison result is always known (e.g., r0 == r0 is always true, r0 < r0 is always false).
Fix this by adding an early return in reg_set_min_max() when false_reg1 and false_reg2 point to the same register, skipping the unnecessary bounds adjustment that leads to the verifier bug.
Reported-by: Kaiyan Mei M202472210@hust.edu.cn Reported-by: Yinhao Hu dddddd@hust.edu.cn Closes: https://lore.kernel.org/all/1881f0f5.300df.199f2576a01.Coremail.kaiyanm@hust... Fixes: 0df1a55afa83 ("bpf: Warn on internal verifier errors") Signed-off-by: KaFai Wan kafai.wan@linux.dev
kernel/bpf/verifier.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6d175849e57a..420ad512d1af 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -16429,6 +16429,10 @@ static int reg_set_min_max(struct bpf_verifier_env *env, if (false_reg1->type != SCALAR_VALUE || false_reg2->type != SCALAR_VALUE) return 0;
- /* If conditional jumps on the same register, skip the adjustment */
- if (false_reg1 == false_reg2)
return 0;
Your change looks good. But this is a special case and it should not happen for any compiler generated code. So could you investigate why regs_refine_cond_op() does not work? Since false_reg1 and false_reg2 is the same, so register refinement should keep the same. Probably some minor change in regs_refine_cond_op(...) should work?
- /* fallthrough (FALSE) branch */ regs_refine_cond_op(false_reg1, false_reg2, rev_opcode(opcode), is_jmp32); reg_bounds_sync(false_reg1);
On Wed, 2025-10-22 at 11:14 -0700, Yonghong Song wrote:
On 10/22/25 9:44 AM, KaFai Wan wrote:
When conditional jumps are performed on the same register (e.g., r0 <= r0, r0 > r0, r0 < r0) where the register holds a scalar with range, the verifier incorrectly attempts to adjust the register's min/max bounds. This leads to invalid range bounds and triggers a BUG warning:
verifier bug: REG INVARIANTS VIOLATION (true_reg1): range bounds violation u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0] var_off=(0x0, 0x0) WARNING: CPU: 0 PID: 93 at kernel/bpf/verifier.c:2731 reg_bounds_sanity_check+0x163/0x220 Modules linked in: CPU: 0 UID: 0 PID: 93 Comm: repro-x-3 Tainted: G W 6.18.0-rc1-ge7586577b75f-dirty #218 PREEMPT(full) Tainted: [W]=WARN Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 RIP: 0010:reg_bounds_sanity_check+0x163/0x220 Call Trace:
<TASK> reg_set_min_max.part.0+0x1b1/0x360 check_cond_jmp_op+0x1195/0x1a60 do_check_common+0x33ac/0x33c0 ...
The issue occurs in reg_set_min_max() function where bounds adjustment logic is applied even when both registers being compared are the same. Comparing a register with itself should not change its bounds since the comparison result is always known (e.g., r0 == r0 is always true, r0 < r0 is always false).
Fix this by adding an early return in reg_set_min_max() when false_reg1 and false_reg2 point to the same register, skipping the unnecessary bounds adjustment that leads to the verifier bug.
Reported-by: Kaiyan Mei M202472210@hust.edu.cn Reported-by: Yinhao Hu dddddd@hust.edu.cn Closes: https://lore.kernel.org/all/1881f0f5.300df.199f2576a01.Coremail.kaiyanm@hust... Fixes: 0df1a55afa83 ("bpf: Warn on internal verifier errors") Signed-off-by: KaFai Wan kafai.wan@linux.dev
kernel/bpf/verifier.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6d175849e57a..420ad512d1af 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -16429,6 +16429,10 @@ static int reg_set_min_max(struct bpf_verifier_env *env, if (false_reg1->type != SCALAR_VALUE || false_reg2->type != SCALAR_VALUE) return 0;
- /* If conditional jumps on the same register, skip the adjustment */
- if (false_reg1 == false_reg2)
return 0;Your change looks good. But this is a special case and it should not happen for any compiler generated code. So could you investigate why regs_refine_cond_op() does not work? Since false_reg1 and false_reg2 is the same, so register refinement should keep the same. Probably some minor change in regs_refine_cond_op(...) should work?
- /* fallthrough (FALSE) branch */ regs_refine_cond_op(false_reg1, false_reg2, rev_opcode(opcode), is_jmp32); reg_bounds_sync(false_reg1);
I think regs_refine_cond_op() is not written in a way to handle same registers passed as reg1 and reg2. E.g. in this particular case the condition is reformulated as "r0 < r0", and then the following branch is taken:
static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2, u8 opcode, bool is_jmp32) { ... case BPF_JLT: // condition is rephrased as r0 < r0 if (is_jmp32) { ... } else { reg1->umax_value = min(reg1->umax_value, reg2->umax_value - 1); reg2->umin_value = max(reg1->umin_value + 1, reg2->umin_value); } break; ... }
Note that intent is to adjust umax of the LHS (reg1) register and umin of the RHS (reg2) register. But here it ends up adjusting the same register.
(a) before refinement: u64=[0x0, 0x80000000] s64=[0x0, 0x80000000] u32=[0x0, 0x80000000] s32=[0x80000000, 0x0] (b) after refinement: u64=[0x1, 0x7fffffff] s64=[0x0, 0x80000000] u32=[0x0, 0x80000000] s32=[0x80000000, 0x0] (c) after sync : u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0]
At (b) the u64 range translated to s32 is > 0, while s32 range is <= 0, hence the invariant violation.
I think it's better to move the reg1 == reg2 check inside regs_refine_cond_op(), or to handle this case in is_branch_taken().
On Wed, Oct 22, 2025 at 12:46 PM Eduard Zingerman eddyz87@gmail.com wrote:
On Wed, 2025-10-22 at 11:14 -0700, Yonghong Song wrote:
On 10/22/25 9:44 AM, KaFai Wan wrote:
When conditional jumps are performed on the same register (e.g., r0 <= r0, r0 > r0, r0 < r0) where the register holds a scalar with range, the verifier incorrectly attempts to adjust the register's min/max bounds. This leads to invalid range bounds and triggers a BUG warning:
verifier bug: REG INVARIANTS VIOLATION (true_reg1): range bounds violation u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0] var_off=(0x0, 0x0) WARNING: CPU: 0 PID: 93 at kernel/bpf/verifier.c:2731 reg_bounds_sanity_check+0x163/0x220 Modules linked in: CPU: 0 UID: 0 PID: 93 Comm: repro-x-3 Tainted: G W 6.18.0-rc1-ge7586577b75f-dirty #218 PREEMPT(full) Tainted: [W]=WARN Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 RIP: 0010:reg_bounds_sanity_check+0x163/0x220 Call Trace:
<TASK> reg_set_min_max.part.0+0x1b1/0x360 check_cond_jmp_op+0x1195/0x1a60 do_check_common+0x33ac/0x33c0 ...
The issue occurs in reg_set_min_max() function where bounds adjustment logic is applied even when both registers being compared are the same. Comparing a register with itself should not change its bounds since the comparison result is always known (e.g., r0 == r0 is always true, r0 < r0 is always false).
Fix this by adding an early return in reg_set_min_max() when false_reg1 and false_reg2 point to the same register, skipping the unnecessary bounds adjustment that leads to the verifier bug.
Reported-by: Kaiyan Mei M202472210@hust.edu.cn Reported-by: Yinhao Hu dddddd@hust.edu.cn Closes: https://lore.kernel.org/all/1881f0f5.300df.199f2576a01.Coremail.kaiyanm@hust... Fixes: 0df1a55afa83 ("bpf: Warn on internal verifier errors") Signed-off-by: KaFai Wan kafai.wan@linux.dev
kernel/bpf/verifier.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6d175849e57a..420ad512d1af 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -16429,6 +16429,10 @@ static int reg_set_min_max(struct bpf_verifier_env *env, if (false_reg1->type != SCALAR_VALUE || false_reg2->type != SCALAR_VALUE) return 0;
- /* If conditional jumps on the same register, skip the adjustment */
- if (false_reg1 == false_reg2)
return 0;Your change looks good. But this is a special case and it should not happen for any compiler generated code. So could you investigate why regs_refine_cond_op() does not work? Since false_reg1 and false_reg2 is the same, so register refinement should keep the same. Probably some minor change in regs_refine_cond_op(...) should work?
- /* fallthrough (FALSE) branch */ regs_refine_cond_op(false_reg1, false_reg2, rev_opcode(opcode), is_jmp32); reg_bounds_sync(false_reg1);
I think regs_refine_cond_op() is not written in a way to handle same registers passed as reg1 and reg2. E.g. in this particular case the condition is reformulated as "r0 < r0", and then the following branch is taken:
static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2, u8 opcode, bool is_jmp32) { ... case BPF_JLT: // condition is rephrased as r0 < r0 if (is_jmp32) { ... } else { reg1->umax_value = min(reg1->umax_value, reg2->umax_value - 1); reg2->umin_value = max(reg1->umin_value + 1, reg2->umin_value); } break; ... }
Note that intent is to adjust umax of the LHS (reg1) register and umin of the RHS (reg2) register. But here it ends up adjusting the same register.
(a) before refinement: u64=[0x0, 0x80000000] s64=[0x0, 0x80000000] u32=[0x0, 0x80000000] s32=[0x80000000, 0x0] (b) after refinement: u64=[0x1, 0x7fffffff] s64=[0x0, 0x80000000] u32=[0x0, 0x80000000] s32=[0x80000000, 0x0] (c) after sync : u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0]
At (b) the u64 range translated to s32 is > 0, while s32 range is <= 0, hence the invariant violation.
I think it's better to move the reg1 == reg2 check inside regs_refine_cond_op(), or to handle this case in is_branch_taken().
hmm. bu then regs_refine_cond_op() will skip it, yet reg_set_min_max() will still be doing pointless work with reg_bounds_sync() and sanity check. The current patch makes more sense to me.
On Wed, 2025-10-22 at 13:12 -0700, Alexei Starovoitov wrote:
On Wed, Oct 22, 2025 at 12:46 PM Eduard Zingerman eddyz87@gmail.com wrote:
On Wed, 2025-10-22 at 11:14 -0700, Yonghong Song wrote:
On 10/22/25 9:44 AM, KaFai Wan wrote:
When conditional jumps are performed on the same register (e.g., r0 <= r0, r0 > r0, r0 < r0) where the register holds a scalar with range, the verifier incorrectly attempts to adjust the register's min/max bounds. This leads to invalid range bounds and triggers a BUG warning:
verifier bug: REG INVARIANTS VIOLATION (true_reg1): range bounds violation u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0] var_off=(0x0, 0x0) WARNING: CPU: 0 PID: 93 at kernel/bpf/verifier.c:2731 reg_bounds_sanity_check+0x163/0x220 Modules linked in: CPU: 0 UID: 0 PID: 93 Comm: repro-x-3 Tainted: G W 6.18.0-rc1-ge7586577b75f-dirty #218 PREEMPT(full) Tainted: [W]=WARN Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 RIP: 0010:reg_bounds_sanity_check+0x163/0x220 Call Trace:
<TASK> reg_set_min_max.part.0+0x1b1/0x360 check_cond_jmp_op+0x1195/0x1a60 do_check_common+0x33ac/0x33c0 ...
The issue occurs in reg_set_min_max() function where bounds adjustment logic is applied even when both registers being compared are the same. Comparing a register with itself should not change its bounds since the comparison result is always known (e.g., r0 == r0 is always true, r0 < r0 is always false).
Fix this by adding an early return in reg_set_min_max() when false_reg1 and false_reg2 point to the same register, skipping the unnecessary bounds adjustment that leads to the verifier bug.
Reported-by: Kaiyan Mei M202472210@hust.edu.cn Reported-by: Yinhao Hu dddddd@hust.edu.cn Closes: https://lore.kernel.org/all/1881f0f5.300df.199f2576a01.Coremail.kaiyanm@hust... Fixes: 0df1a55afa83 ("bpf: Warn on internal verifier errors") Signed-off-by: KaFai Wan kafai.wan@linux.dev
kernel/bpf/verifier.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6d175849e57a..420ad512d1af 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -16429,6 +16429,10 @@ static int reg_set_min_max(struct bpf_verifier_env *env, if (false_reg1->type != SCALAR_VALUE || false_reg2->type != SCALAR_VALUE) return 0;
- /* If conditional jumps on the same register, skip the adjustment */
- if (false_reg1 == false_reg2)
return 0;Your change looks good. But this is a special case and it should not happen for any compiler generated code. So could you investigate why regs_refine_cond_op() does not work? Since false_reg1 and false_reg2 is the same, so register refinement should keep the same. Probably some minor change in regs_refine_cond_op(...) should work?
- /* fallthrough (FALSE) branch */ regs_refine_cond_op(false_reg1, false_reg2, rev_opcode(opcode), is_jmp32); reg_bounds_sync(false_reg1);
I think regs_refine_cond_op() is not written in a way to handle same registers passed as reg1 and reg2. E.g. in this particular case the condition is reformulated as "r0 < r0", and then the following branch is taken:
static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2, u8 opcode, bool is_jmp32) { ... case BPF_JLT: // condition is rephrased as r0 < r0 if (is_jmp32) { ... } else { reg1->umax_value = min(reg1->umax_value, reg2->umax_value - 1); reg2->umin_value = max(reg1->umin_value + 1, reg2->umin_value); } break; ... }
Note that intent is to adjust umax of the LHS (reg1) register and umin of the RHS (reg2) register. But here it ends up adjusting the same register.
(a) before refinement: u64=[0x0, 0x80000000] s64=[0x0, 0x80000000] u32=[0x0, 0x80000000] s32=[0x80000000, 0x0] (b) after refinement: u64=[0x1, 0x7fffffff] s64=[0x0, 0x80000000] u32=[0x0, 0x80000000] s32=[0x80000000, 0x0] (c) after sync : u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0]
At (b) the u64 range translated to s32 is > 0, while s32 range is <= 0, hence the invariant violation.
I think it's better to move the reg1 == reg2 check inside regs_refine_cond_op(), or to handle this case in is_branch_taken().
hmm. bu then regs_refine_cond_op() will skip it, yet reg_set_min_max() will still be doing pointless work with reg_bounds_sync() and sanity check. The current patch makes more sense to me.
Well, if we want to avoid useless work, we need something like:
@@ -16173,6 +16173,25 @@ static int is_pkt_ptr_branch_taken(struct bpf_reg_state *dst_reg, static int is_branch_taken(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2, u8 opcode, bool is_jmp32) { + if (reg1 == reg2) { + switch (opcode) { + case BPF_JGE: + case BPF_JLE: + case BPF_JSGE: + case BPF_JSLE: + case BPF_JEQ: + case BPF_JSET: + return 1; + case BPF_JGT: + case BPF_JLT: + case BPF_JSGT: + case BPF_JSLT: + case BPF_JNE: + return 0; + default: + return -1; + } + }
But that's too much code for an artificial case. Idk, either way is fine with me.
On Wed, Oct 22, 2025 at 1:30 PM Eduard Zingerman eddyz87@gmail.com wrote:
On Wed, 2025-10-22 at 13:12 -0700, Alexei Starovoitov wrote:
On Wed, Oct 22, 2025 at 12:46 PM Eduard Zingerman eddyz87@gmail.com wrote:
On Wed, 2025-10-22 at 11:14 -0700, Yonghong Song wrote:
On 10/22/25 9:44 AM, KaFai Wan wrote:
When conditional jumps are performed on the same register (e.g., r0 <= r0, r0 > r0, r0 < r0) where the register holds a scalar with range, the verifier incorrectly attempts to adjust the register's min/max bounds. This leads to invalid range bounds and triggers a BUG warning:
verifier bug: REG INVARIANTS VIOLATION (true_reg1): range bounds violation u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0] var_off=(0x0, 0x0) WARNING: CPU: 0 PID: 93 at kernel/bpf/verifier.c:2731 reg_bounds_sanity_check+0x163/0x220 Modules linked in: CPU: 0 UID: 0 PID: 93 Comm: repro-x-3 Tainted: G W 6.18.0-rc1-ge7586577b75f-dirty #218 PREEMPT(full) Tainted: [W]=WARN Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 RIP: 0010:reg_bounds_sanity_check+0x163/0x220 Call Trace:
<TASK> reg_set_min_max.part.0+0x1b1/0x360 check_cond_jmp_op+0x1195/0x1a60 do_check_common+0x33ac/0x33c0 ...
The issue occurs in reg_set_min_max() function where bounds adjustment logic is applied even when both registers being compared are the same. Comparing a register with itself should not change its bounds since the comparison result is always known (e.g., r0 == r0 is always true, r0 < r0 is always false).
Fix this by adding an early return in reg_set_min_max() when false_reg1 and false_reg2 point to the same register, skipping the unnecessary bounds adjustment that leads to the verifier bug.
Reported-by: Kaiyan Mei M202472210@hust.edu.cn Reported-by: Yinhao Hu dddddd@hust.edu.cn Closes: https://lore.kernel.org/all/1881f0f5.300df.199f2576a01.Coremail.kaiyanm@hust... Fixes: 0df1a55afa83 ("bpf: Warn on internal verifier errors") Signed-off-by: KaFai Wan kafai.wan@linux.dev
kernel/bpf/verifier.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6d175849e57a..420ad512d1af 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -16429,6 +16429,10 @@ static int reg_set_min_max(struct bpf_verifier_env *env, if (false_reg1->type != SCALAR_VALUE || false_reg2->type != SCALAR_VALUE) return 0;
- /* If conditional jumps on the same register, skip the adjustment */
- if (false_reg1 == false_reg2)
return 0;Your change looks good. But this is a special case and it should not happen for any compiler generated code. So could you investigate why regs_refine_cond_op() does not work? Since false_reg1 and false_reg2 is the same, so register refinement should keep the same. Probably some minor change in regs_refine_cond_op(...) should work?
- /* fallthrough (FALSE) branch */ regs_refine_cond_op(false_reg1, false_reg2, rev_opcode(opcode), is_jmp32); reg_bounds_sync(false_reg1);
I think regs_refine_cond_op() is not written in a way to handle same registers passed as reg1 and reg2. E.g. in this particular case the condition is reformulated as "r0 < r0", and then the following branch is taken:
static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2, u8 opcode, bool is_jmp32) { ... case BPF_JLT: // condition is rephrased as r0 < r0 if (is_jmp32) { ... } else { reg1->umax_value = min(reg1->umax_value, reg2->umax_value - 1); reg2->umin_value = max(reg1->umin_value + 1, reg2->umin_value); } break; ... }
Note that intent is to adjust umax of the LHS (reg1) register and umin of the RHS (reg2) register. But here it ends up adjusting the same register.
(a) before refinement: u64=[0x0, 0x80000000] s64=[0x0, 0x80000000] u32=[0x0, 0x80000000] s32=[0x80000000, 0x0] (b) after refinement: u64=[0x1, 0x7fffffff] s64=[0x0, 0x80000000] u32=[0x0, 0x80000000] s32=[0x80000000, 0x0] (c) after sync : u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0]
At (b) the u64 range translated to s32 is > 0, while s32 range is <= 0, hence the invariant violation.
I think it's better to move the reg1 == reg2 check inside regs_refine_cond_op(), or to handle this case in is_branch_taken().
hmm. bu then regs_refine_cond_op() will skip it, yet reg_set_min_max() will still be doing pointless work with reg_bounds_sync() and sanity check. The current patch makes more sense to me.
Well, if we want to avoid useless work, we need something like:
@@ -16173,6 +16173,25 @@ static int is_pkt_ptr_branch_taken(struct bpf_reg_state *dst_reg, static int is_branch_taken(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2, u8 opcode, bool is_jmp32) {
if (reg1 == reg2) {switch (opcode) {case BPF_JGE:case BPF_JLE:case BPF_JSGE:case BPF_JSLE:case BPF_JEQ:case BPF_JSET:return 1;case BPF_JGT:case BPF_JLT:case BPF_JSGT:case BPF_JSLT:case BPF_JNE:return 0;default:return -1;}}But that's too much code for an artificial case. Idk, either way is fine with me.
Makes sense to me.
On Wed, 2025-10-22 at 13:30 -0700, Eduard Zingerman wrote:
On Wed, 2025-10-22 at 13:12 -0700, Alexei Starovoitov wrote:
On Wed, Oct 22, 2025 at 12:46 PM Eduard Zingerman eddyz87@gmail.com wrote:
On Wed, 2025-10-22 at 11:14 -0700, Yonghong Song wrote:
On 10/22/25 9:44 AM, KaFai Wan wrote:
When conditional jumps are performed on the same register (e.g., r0 <= r0, r0 > r0, r0 < r0) where the register holds a scalar with range, the verifier incorrectly attempts to adjust the register's min/max bounds. This leads to invalid range bounds and triggers a BUG warning:
verifier bug: REG INVARIANTS VIOLATION (true_reg1): range bounds violation u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0] var_off=(0x0, 0x0) WARNING: CPU: 0 PID: 93 at kernel/bpf/verifier.c:2731 reg_bounds_sanity_check+0x163/0x220 Modules linked in: CPU: 0 UID: 0 PID: 93 Comm: repro-x-3 Tainted: G W 6.18.0-rc1-ge7586577b75f-dirty #218 PREEMPT(full) Tainted: [W]=WARN Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 RIP: 0010:reg_bounds_sanity_check+0x163/0x220 Call Trace: <TASK> reg_set_min_max.part.0+0x1b1/0x360 check_cond_jmp_op+0x1195/0x1a60 do_check_common+0x33ac/0x33c0 ...
The issue occurs in reg_set_min_max() function where bounds adjustment logic is applied even when both registers being compared are the same. Comparing a register with itself should not change its bounds since the comparison result is always known (e.g., r0 == r0 is always true, r0 < r0 is always false).
Fix this by adding an early return in reg_set_min_max() when false_reg1 and false_reg2 point to the same register, skipping the unnecessary bounds adjustment that leads to the verifier bug.
Reported-by: Kaiyan Mei M202472210@hust.edu.cn Reported-by: Yinhao Hu dddddd@hust.edu.cn Closes: https://lore.kernel.org/all/1881f0f5.300df.199f2576a01.Coremail.kaiyanm@hust... Fixes: 0df1a55afa83 ("bpf: Warn on internal verifier errors") Signed-off-by: KaFai Wan kafai.wan@linux.dev
kernel/bpf/verifier.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6d175849e57a..420ad512d1af 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -16429,6 +16429,10 @@ static int reg_set_min_max(struct bpf_verifier_env *env, if (false_reg1->type != SCALAR_VALUE || false_reg2->type != SCALAR_VALUE) return 0;
+ /* If conditional jumps on the same register, skip the adjustment */ + if (false_reg1 == false_reg2) + return 0;
Your change looks good. But this is a special case and it should not happen for any compiler generated code. So could you investigate why regs_refine_cond_op() does not work? Since false_reg1 and false_reg2 is the same, so register refinement should keep the same. Probably some minor change in regs_refine_cond_op(...) should work?
/* fallthrough (FALSE) branch */ regs_refine_cond_op(false_reg1, false_reg2, rev_opcode(opcode), is_jmp32); reg_bounds_sync(false_reg1);
I think regs_refine_cond_op() is not written in a way to handle same registers passed as reg1 and reg2. E.g. in this particular case the condition is reformulated as "r0 < r0", and then the following branch is taken:
static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2, u8 opcode, bool is_jmp32) { ... case BPF_JLT: // condition is rephrased as r0 < r0 if (is_jmp32) { ... } else { reg1->umax_value = min(reg1->umax_value, reg2-
umax_value - 1);
reg2->umin_value = max(reg1->umin_value + 1, reg2->umin_value);
Yes, that's the root cause.
} break; ... }
Note that intent is to adjust umax of the LHS (reg1) register and umin of the RHS (reg2) register. But here it ends up adjusting the same register.
(a) before refinement: u64=[0x0, 0x80000000] s64=[0x0, 0x80000000] u32=[0x0, 0x80000000] s32=[0x80000000, 0x0] (b) after refinement: u64=[0x1, 0x7fffffff] s64=[0x0, 0x80000000] u32=[0x0, 0x80000000] s32=[0x80000000, 0x0] (c) after sync : u64=[0x1, 0x0] s64=[0x1, 0x0] u32=[0x1, 0x0] s32=[0x1, 0x0]
At (b) the u64 range translated to s32 is > 0, while s32 range is <= 0, hence the invariant violation.
I think it's better to move the reg1 == reg2 check inside regs_refine_cond_op(), or to handle this case in is_branch_taken().
hmm. bu then regs_refine_cond_op() will skip it, yet reg_set_min_max() will still be doing pointless work with reg_bounds_sync() and sanity check. The current patch makes more sense to me.
Well, if we want to avoid useless work, we need something like:
@@ -16173,6 +16173,25 @@ static int is_pkt_ptr_branch_taken(struct bpf_reg_state *dst_reg, static int is_branch_taken(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2, u8 opcode, bool is_jmp32) { + if (reg1 == reg2) { + switch (opcode) { + case BPF_JGE: + case BPF_JLE: + case BPF_JSGE: + case BPF_JSLE: + case BPF_JEQ: + case BPF_JSET:
Others are fine, but BPF_JSET on the same register could be 0 (if value is 0). And it's unknown to take the branch if 0 within the range.
+ return 1; + case BPF_JGT: + case BPF_JLT: + case BPF_JSGT: + case BPF_JSLT: + case BPF_JNE: + return 0; + default: + return -1; + } + }
But that's too much code for an artificial case. Idk, either way is fine with me.
There is is_scalar_branch_taken() in is_branch_taken(), I missed it. I'll a) check the opcode one by one in is_scalar_branch_taken(), and b) keep this patch for unknown BPF_JSET branch.
On Thu, 2025-10-23 at 19:26 +0800, KaFai Wan wrote:
[...]
@@ -16173,6 +16173,25 @@ static int is_pkt_ptr_branch_taken(struct bpf_reg_state *dst_reg, static int is_branch_taken(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2, u8 opcode, bool is_jmp32) { + if (reg1 == reg2) { + switch (opcode) { + case BPF_JGE: + case BPF_JLE: + case BPF_JSGE: + case BPF_JSLE: + case BPF_JEQ: + case BPF_JSET:
Others are fine, but BPF_JSET on the same register could be 0 (if value is 0). And it's unknown to take the branch if 0 within the range.
Right, missed that one.
+ return 1; + case BPF_JGT: + case BPF_JLT: + case BPF_JSGT: + case BPF_JSLT: + case BPF_JNE: + return 0; + default: + return -1; + } + }
But that's too much code for an artificial case. Idk, either way is fine with me.
There is is_scalar_branch_taken() in is_branch_taken(), I missed it. I'll a) check the opcode one by one in is_scalar_branch_taken(), and b) keep this patch for unknown BPF_JSET branch.
Sounds good to me. Note that the logic is correct for both scalar and non-scalar cases, so I don't think we have to constrain it to is_scalar_branch_taken() (don't think there is a need to check if pointer comparisons are allowed, as no new information is inferred from comparisons with self).
On Thu, 2025-10-23 at 10:38 -0700, Eduard Zingerman wrote:
On Thu, 2025-10-23 at 19:26 +0800, KaFai Wan wrote:
[...]
@@ -16173,6 +16173,25 @@ static int is_pkt_ptr_branch_taken(struct bpf_reg_state *dst_reg, static int is_branch_taken(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2, u8 opcode, bool is_jmp32) { + if (reg1 == reg2) { + switch (opcode) { + case BPF_JGE: + case BPF_JLE: + case BPF_JSGE: + case BPF_JSLE: + case BPF_JEQ: + case BPF_JSET:
Others are fine, but BPF_JSET on the same register could be 0 (if value is 0). And it's unknown to take the branch if 0 within the range.
Right, missed that one.
+ return 1; + case BPF_JGT: + case BPF_JLT: + case BPF_JSGT: + case BPF_JSLT: + case BPF_JNE: + return 0; + default: + return -1; + } + }
But that's too much code for an artificial case. Idk, either way is fine with me.
There is is_scalar_branch_taken() in is_branch_taken(), I missed it. I'll a) check the opcode one by one in is_scalar_branch_taken(), and b) keep this patch for unknown BPF_JSET branch.
Sounds good to me. Note that the logic is correct for both scalar and non-scalar cases, so I don't think we have to constrain it to is_scalar_branch_taken() (don't think there is a need to check if pointer comparisons are allowed, as no new information is inferred from comparisons with self).
For non-scalar cases we only allow pointer comparison on pkt_ptr, this check is before is_branch_taken()
src_reg = ®s[insn->src_reg]; if (!(reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg)) && is_pointer_value(env, insn->src_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->src_reg); return -EACCES; }
and in the end of check_cond_jmp_op() (after is_branch_taken()), we checked again
} else if (!try_match_pkt_pointers(insn, dst_reg, ®s[insn->src_reg], this_branch, other_branch) && is_pointer_value(env, insn->dst_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->dst_reg); return -EACCES; }
this time we check if it is valid comparison on pkt_ptr in try_match_pkt_pointers().
Currently we just allow 4 opcode (BPF_JGT, BPF_JLT, BPF_JGE, BPF_JLE) on pkt_ptr, and with conditions. But we bypass these prohibits in privileged mode (is_pointer_value() always return false in privileged mode).
So the logic skip these prohibits for pkt_ptr in unprivileged mode.
On Sat, 2025-10-25 at 00:13 +0800, KaFai Wan wrote:
[...]
For non-scalar cases we only allow pointer comparison on pkt_ptr, this check is before is_branch_taken()
src_reg = ®s[insn->src_reg]; if (!(reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg)) && is_pointer_value(env, insn->src_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->src_reg); return -EACCES; }
and in the end of check_cond_jmp_op() (after is_branch_taken()), we checked again
} else if (!try_match_pkt_pointers(insn, dst_reg, ®s[insn->src_reg], this_branch, other_branch) && is_pointer_value(env, insn->dst_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->dst_reg); return -EACCES; }
this time we check if it is valid comparison on pkt_ptr in try_match_pkt_pointers().
Currently we just allow 4 opcode (BPF_JGT, BPF_JLT, BPF_JGE, BPF_JLE) on pkt_ptr, and with conditions. But we bypass these prohibits in privileged mode (is_pointer_value() always return false in privileged mode).
So the logic skip these prohibits for pkt_ptr in unprivileged mode.
Well, yes, but do you really need to do forbid `if r0 > r0 goto ...` in unpriv?
On Fri, 2025-10-24 at 09:21 -0700, Eduard Zingerman wrote:
On Sat, 2025-10-25 at 00:13 +0800, KaFai Wan wrote:
[...]
For non-scalar cases we only allow pointer comparison on pkt_ptr, this check is before is_branch_taken()
src_reg = ®s[insn->src_reg]; if (!(reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg)) && is_pointer_value(env, insn->src_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->src_reg); return -EACCES; }
and in the end of check_cond_jmp_op() (after is_branch_taken()), we checked again
} else if (!try_match_pkt_pointers(insn, dst_reg, ®s[insn->src_reg], this_branch, other_branch) && is_pointer_value(env, insn->dst_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->dst_reg); return -EACCES; }
this time we check if it is valid comparison on pkt_ptr in try_match_pkt_pointers().
Currently we just allow 4 opcode (BPF_JGT, BPF_JLT, BPF_JGE, BPF_JLE) on pkt_ptr, and with conditions. But we bypass these prohibits in privileged mode (is_pointer_value() always return false in privileged mode).
So the logic skip these prohibits for pkt_ptr in unprivileged mode.
Well, yes, but do you really need to do forbid `if r0 > r0 goto ...` in unpriv?
Currently `if r0 > r0 goto ...` is forbid in unpriv, but we can allow it.
On Fri, Oct 24, 2025 at 9:38 AM KaFai Wan kafai.wan@linux.dev wrote:
On Fri, 2025-10-24 at 09:21 -0700, Eduard Zingerman wrote:
On Sat, 2025-10-25 at 00:13 +0800, KaFai Wan wrote:
[...]
For non-scalar cases we only allow pointer comparison on pkt_ptr, this check is before is_branch_taken()
src_reg = ®s[insn->src_reg]; if (!(reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg)) && is_pointer_value(env, insn->src_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->src_reg); return -EACCES; }and in the end of check_cond_jmp_op() (after is_branch_taken()), we checked again
} else if (!try_match_pkt_pointers(insn, dst_reg, ®s[insn->src_reg], this_branch, other_branch) && is_pointer_value(env, insn->dst_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->dst_reg); return -EACCES; }this time we check if it is valid comparison on pkt_ptr in try_match_pkt_pointers().
Currently we just allow 4 opcode (BPF_JGT, BPF_JLT, BPF_JGE, BPF_JLE) on pkt_ptr, and with conditions. But we bypass these prohibits in privileged mode (is_pointer_value() always return false in privileged mode).
So the logic skip these prohibits for pkt_ptr in unprivileged mode.
Well, yes, but do you really need to do forbid `if r0 > r0 goto ...` in unpriv?
Currently `if r0 > r0 goto ...` is forbid in unpriv, but we can allow it.
Let's not relax unpriv. We don't need new threads with researchers whether such things can be exploited.
On Fri, 2025-10-24 at 09:40 -0700, Alexei Starovoitov wrote:
On Fri, Oct 24, 2025 at 9:38 AM KaFai Wan kafai.wan@linux.dev wrote:
On Fri, 2025-10-24 at 09:21 -0700, Eduard Zingerman wrote:
On Sat, 2025-10-25 at 00:13 +0800, KaFai Wan wrote:
[...]
For non-scalar cases we only allow pointer comparison on pkt_ptr, this check is before is_branch_taken()
src_reg = ®s[insn->src_reg]; if (!(reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg)) && is_pointer_value(env, insn->src_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->src_reg); return -EACCES; }
and in the end of check_cond_jmp_op() (after is_branch_taken()), we checked again
} else if (!try_match_pkt_pointers(insn, dst_reg, ®s[insn->src_reg], this_branch, other_branch) && is_pointer_value(env, insn->dst_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->dst_reg); return -EACCES; }
this time we check if it is valid comparison on pkt_ptr in try_match_pkt_pointers().
Currently we just allow 4 opcode (BPF_JGT, BPF_JLT, BPF_JGE, BPF_JLE) on pkt_ptr, and with conditions. But we bypass these prohibits in privileged mode (is_pointer_value() always return false in privileged mode).
So the logic skip these prohibits for pkt_ptr in unprivileged mode.
Well, yes, but do you really need to do forbid `if r0 > r0 goto ...` in unpriv?
Currently `if r0 > r0 goto ...` is forbid in unpriv, but we can allow it.
Let's not relax unpriv. We don't need new threads with researchers whether such things can be exploited.
Ok, I'll keep the logic for both scalar and non-scalar cases.
Add a test case to verify that conditional jumps on the same register (e.g., JGT r0 > r0) do not trigger verifier BUG warnings when the register holds a scalar with range.
Signed-off-by: KaFai Wan kafai.wan@linux.dev --- .../selftests/bpf/progs/verifier_bounds.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/verifier_bounds.c b/tools/testing/selftests/bpf/progs/verifier_bounds.c index 0a72e0228ea9..620095635af5 100644 --- a/tools/testing/selftests/bpf/progs/verifier_bounds.c +++ b/tools/testing/selftests/bpf/progs/verifier_bounds.c @@ -1709,4 +1709,21 @@ __naked void jeq_disagreeing_tnums(void *ctx) : __clobber_all); }
+SEC("socket") +__description("JGT on same register") +__success __log_level(2) +__naked void jgt_same_register(void *ctx) +{ + asm volatile(" \ + call %[bpf_get_prandom_u32]; \ + w8 = 0x80000000; \ + r0 &= r8; \ + if r0 > r0 goto +1; \ + call %[bpf_get_prandom_u32]; \ + exit; \ +" : + : __imm(bpf_get_prandom_u32) + : __clobber_all); +} + char _license[] SEC("license") = "GPL";
linux-kselftest-mirror@lists.linaro.org
-
Alexei Starovoitov -
Eduard Zingerman -
KaFai Wan -
Yonghong Song