Re: [PATCH bpf-next 1/2] bpf: Skip bounds adjustment for conditional jumps on same register

[Eduard Zingerman <eddyz87@gmail.com>]

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.edu.cn/
> > 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().