linux-kernel - Re: [PATCH] bpf: verifier: fix WARNING in reg_bounds_sanity

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aMkUFNr-0kYLfMNH@mail.gmail.com>
Date: Tue, 16 Sep 2025 09:39:00 +0200
From: Paul Chaignon <paul.chaignon@...il.com>
To: Kriish Sharma <kriish.sharma2006@...il.com>
Cc: ast@...nel.org, daniel@...earbox.net, john.fastabend@...il.com,
	andrii@...nel.org, martin.lau@...ux.dev, eddyz87@...il.com,
	song@...nel.org, yonghong.song@...ux.dev, kpsingh@...nel.org,
	sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org,
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
	syzbot+c950cc277150935cc0b5@...kaller.appspotmail.com,
	Andrii Nakryiko <andrii.nakryiko@...il.com>
Subject: Re: [PATCH] bpf: verifier: fix WARNING in reg_bounds_sanity_check (2)

On Mon, Sep 15, 2025 at 12:54:20PM -0700, Andrii Nakryiko wrote:
> On Sat, Sep 13, 2025 at 3:24 PM Kriish Sharma
> <kriish.sharma2006@...il.com> wrote:
> >
> > syzbot reported a "REG INVARIANTS VIOLATION" triggered in reg_bounds_sanity_check()
> > due to inconsistent umin/umax and var_off state after min/max updates.
> >
> > reg_set_min_max() and adjust_reg_min_max_vals() could leave a register state
> > partially updated before syncing the bounds, causing verifier_bug() to fire.
> >
> > This patch ensures reg_bounds_sync() is called after updates, and additionally
> > marks registers unbounded if min/max values are inconsistent, so that umin/umax,
> > smin/smax, and var_off remain consistent.
> >
> > Fixes: d69eb204c255 ("Merge tag 'net-6.17-rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net")
> > Reported-by: syzbot+c950cc277150935cc0b5@...kaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=c950cc277150935cc0b5
> > Signed-off-by: Kriish Sharma <kriish.sharma2006@...il.com>
> > ---
> >  kernel/bpf/verifier.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index c4f69a9e9af6..8f5f02d39005 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -16299,6 +16299,19 @@ static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state
> >         }
> >  }
> >
> > +/* Ensure that a register's min/max bounds are sane.
> > + * If any of the unsigned/signed bounds are inconsistent, mark the
> > + * register as unbounded to prevent verifier invariant violations.
> > + */
> > +static void __maybe_normalize_reg(struct bpf_reg_state *reg)
> > +{
> > +       if (reg->umin_value > reg->umax_value ||
> > +               reg->smin_value > reg->smax_value ||
> > +               reg->u32_min_value > reg->u32_max_value ||
> > +               reg->s32_min_value > reg->s32_max_value)
> > +                       __mark_reg_unbounded(reg);
> > +}
> > +
> >  /* Adjusts the register min/max values in the case that the dst_reg and
> >   * src_reg are both SCALAR_VALUE registers (or we are simply doing a BPF_K
> >   * check, in which case we have a fake SCALAR_VALUE representing insn->imm).
> > @@ -16325,11 +16338,15 @@ static int reg_set_min_max(struct bpf_verifier_env *env,
> >         regs_refine_cond_op(false_reg1, false_reg2, rev_opcode(opcode), is_jmp32);
> >         reg_bounds_sync(false_reg1);
> >         reg_bounds_sync(false_reg2);
> > +       __maybe_normalize_reg(false_reg1);
> > +       __maybe_normalize_reg(false_reg2);
> >
> >         /* jump (TRUE) branch */
> >         regs_refine_cond_op(true_reg1, true_reg2, opcode, is_jmp32);
> >         reg_bounds_sync(true_reg1);
> >         reg_bounds_sync(true_reg2);
> > +       __maybe_normalize_reg(true_reg1);
> > +       __maybe_normalize_reg(true_reg2);
> 
> We are actually taking a different approach to this problem. Eduard is
> going to modify verifier logic to use the fact that register' tnum and
> range bounds are incompatible to detect branches that cannot be taken,
> and process it as dead code. This way we don't lose information (like
> with the approach in this patch), but rather take advantage of it to
> improve verification performance.
> 
> Thanks for your patch, but I think we should go with the more generic
> solution I outlined above.

Agree with Andrii here. And even without Eduard's approach, there's a
better fix for the specific invariant violations reported here. The
reproducers end with:

  5: (1f) r3 -= r0                ; R0=0x8000050 R3_w=scalar(smin=0xffffffffefffff60,smax=smax32=0,umin=umin32=32,umax=0xffffffffffffffe0,smin32=0xf7ffffe0,umax32=0xffffffe0,var_off=(0x20; 0xffffffffffffffc0))
  6: (2e) if w3 > w0 goto pc+2
  REG INVARIANTS VIOLATION (false_reg1): range bounds violation u64=[0x20, 0xffffffff08000050] s64=[0xffffffffefffff60, 0x0] u32=[0x20, 0x8000050] s32=[0x20, 0x0] var_off=(0x20, 0xffffffff0fffffc0)

We can see that on instruction 5, the smin32 and umin32 ranges can
actually be improved for R3 because they overlap. They're not currently
updated because they cross the sign boundary, so a patch like
00bf8d0c6c9b ("bpf: Improve bounds when s64 crosses sign boundary"),
but for 32-bits would be needed. Once those ranges are updated, it's
clear the jump is never taken.

We can reconsider this after Eduard's patch lands, but I doubt such a
fix will still be needed at that point :)

> 
> pw-bot: cr
> 
> 
> >
> >         err = reg_bounds_sanity_check(env, true_reg1, "true_reg1");
> >         err = err ?: reg_bounds_sanity_check(env, true_reg2, "true_reg2");
> > --
> > 2.34.1
> >
>