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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzY_f=iNKC2CVz-myfe_OERN9XWHiuNG6vng43-MXUAvSw@mail.gmail.com>
Date: Mon, 15 Sep 2025 12:54:20 -0700
From: Andrii Nakryiko <andrii.nakryiko@...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
Subject: Re: [PATCH] bpf: verifier: fix WARNING in reg_bounds_sanity_check (2)

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.

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
>