linux-kernel - Re: [PATCH 06/44] bpf: Verifier, remove some unusual uses of min_t() and max

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: <CAADnVQJFxWParvYg9_YZaD7HFFPW6yzStm7e1nKgdMQ+UYUtqw@mail.gmail.com>
Date: Sun, 23 Nov 2025 08:39:51 -0800
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: David Laight <david.laight.linux@...il.com>
Cc: LKML <linux-kernel@...r.kernel.org>, bpf <bpf@...r.kernel.org>, 
	Alexei Starovoitov <ast@...nel.org>, Andrii Nakryiko <andrii@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH 06/44] bpf: Verifier, remove some unusual uses of min_t()
 and max_t()

On Fri, Nov 21, 2025 at 2:21 PM David Laight
<david.laight.linux@...il.com> wrote:
>
> On Fri, 21 Nov 2025 13:40:36 -0800
> Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
>
> > On Wed, Nov 19, 2025 at 2:42 PM <david.laight.linux@...il.com> wrote:
> > >
> > > From: David Laight <david.laight.linux@...il.com>
> > >
> > > min_t() and max_t() are normally used to change the signedness
> > > of a positive value to avoid a signed-v-unsigned compare warning.
> > >
> > > However they are used here to convert an unsigned 64bit pattern
> > > to a signed to a 32/64bit signed number.
> > > To avoid any confusion use plain min()/max() and explicitely cast
> > > the u64 expression to the correct signed value.
> > >
> > > Use a simple max() for the max_pkt_offset calulation and delete the
> > > comment about why the cast to u32 is safe.
> > >
> > > Signed-off-by: David Laight <david.laight.linux@...il.com>
> > > ---
> > >  kernel/bpf/verifier.c | 29 +++++++++++------------------
> > >  1 file changed, 11 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index ff40e5e65c43..22fa9769fbdb 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -2319,12 +2319,12 @@ static void __update_reg32_bounds(struct bpf_reg_state *reg)
> > >         struct tnum var32_off = tnum_subreg(reg->var_off);
> > >
> > >         /* min signed is max(sign bit) | min(other bits) */
> > > -       reg->s32_min_value = max_t(s32, reg->s32_min_value,
> > > -                       var32_off.value | (var32_off.mask & S32_MIN));
> > > +       reg->s32_min_value = max(reg->s32_min_value,
> > > +                       (s32)(var32_off.value | (var32_off.mask & S32_MIN)));
> > >         /* max signed is min(sign bit) | max(other bits) */
> > > -       reg->s32_max_value = min_t(s32, reg->s32_max_value,
> > > -                       var32_off.value | (var32_off.mask & S32_MAX));
> > > -       reg->u32_min_value = max_t(u32, reg->u32_min_value, (u32)var32_off.value);
> > > +       reg->s32_max_value = min(reg->s32_max_value,
> > > +                       (s32)(var32_off.value | (var32_off.mask & S32_MAX)));
> >
> > Nack.
> > This is plain ugly for no good reason.
> > Leave the code as-is.
>
> It is really horrid before.
> From what i remember var32_off.value (and .mask) are both u64.
> The pattern actually patches that used a few lines down the file.
>
> I've been trying to build allmodconfig with the size test added to min_t()
> and max_t().
> The number of real (or potentially real) bugs I've found is stunning.
> The only fix is to nuke min_t() and max_t() to they can't be used.

No. min_t() is going to stay. It's not broken and
this crusade against it is inappropriate.

> The basic problem is the people have used the type of the target not that
> of the largest parameter.
> The might be ok for ulong v uint (on 64bit), but there are plenty of places
> where u16 and u8 are used - a lot are pretty much buggy.
>
> Perhaps the worst ones I've found are with clamp_t(),
> this is from 2/44:
> -               (raw_inode)->xtime = cpu_to_le32(clamp_t(int32_t, (ts).tv_sec, S32_MIN, S32_MAX));      \
> +               (raw_inode)->xtime = cpu_to_le32(clamp((ts).tv_sec, S32_MIN, S32_MAX)); \
> If also found clamp_t(u8, xxx, 0, 255).
>
> There are just so many broken examples.

clamp_t(u8, xxx, 0, 255) is not wrong. It's silly, but
it's doing the right thing and one can argue and explicit
clamp values serve as a documentation.
clamp_t(int32_t, (ts).tv_sec, S32_MIN, S32_MAX)) is indeed incorrect,
but it's a bug in the implementation of __clamp_once().
Fix it, instead of spamming people with "_t" removal.