Received-SPF: pass (google.com: domain of linux-kernel+bounces-62154-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240209150657.1.I45addf7579e1233fa97c05ba72120cd1c57b4310@changeid>
 <202402091551.16A4A2A7F@keescook> <65ba51bc65b24c13af4ce02e79f78dee@AcuMS.aculab.com>
In-Reply-To: <65ba51bc65b24c13af4ce02e79f78dee@AcuMS.aculab.com>
From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Date: Mon, 12 Feb 2024 09:55:29 -0800
Message-ID: <CANFp7mVm6QQ0+rm_VJ6MJbVDM+agyTd+5cEwE3EneSxS1-z70w@mail.gmail.com>
Subject: Re: [PATCH] minmax: Add notes to min_t and max_t
To: David Laight <David.Laight@aculab.com>
Cc: Kees Cook <keescook@chromium.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
	"pmalani@chromium.org" <pmalani@chromium.org>, 
	"andriy.shevchenko@linux.intel.com" <andriy.shevchenko@linux.intel.com>, 
	"torvalds@linux-foundation.org" <torvalds@linux-foundation.org>, Andrew Morton <akpm@linux-foundation.org>, 
	Herve Codina <herve.codina@bootlin.com>, "Matthew Wilcox (Oracle)" <willy@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Feb 10, 2024 at 4:04=E2=80=AFAM David Laight <David.Laight@aculab.c=
om> wrote:
>
> From: Kees Cook
> > Sent: 09 February 2024 23:56
> >
> > On Fri, Feb 09, 2024 at 03:07:02PM -0800, Abhishek Pandit-Subedi wrote:
> > > Both min_t and max_t are problematic as they can hide issues when
> > > comparing differently sized types (and especially differently signed
> > > types). Update the comments to nudge users to other options until
> > > there is a better fix for these macros.
> > >
> > > Link: https://lore.kernel.org/all/01e3e09005e9434b8f558a893a47c053@Ac=
uMS.aculab.com/
> > > Link: https://lore.kernel.org/all/CAHk-
> > =3DwhwEAc22wm8h9FESPB5X+P4bLDgv0erBQMa1buTNQW7tA@mail.gmail.com/
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > ---
> > > Andy Shevchenko made me aware of this particular footgun in
> > > https://lore.kernel.org/linux-usb/ZcZ_he1jYx8w57mK@smile.fi.intel.com=
/.
> > >
> > > While David + others work on the full fix, I'm hoping to apply a
> > > bandaid in the form of comments so the problem doesn't get worse by d=
evs
> > > (**cough** me **cough**) inadvertently doing the wrong thing.
>
> I'm not sure that adding a comment here actually helps.
> If you read it you probably know what is happening!
>
> With the changes I did (which I think got back-ported at least
> one release) it is actually moderately unlikely that you'll need
> to use min_t() or max_t() (and especially clamp_val() - definitely
> an accident waiting to happen).
>
> I think there is only one clamp_val() that can't just be replaced
> with clamp().
>
> I did post an updated set that really just reduce the generated
> line length - I probably need to report them to wake people up.
>
> > I think a better example for the docs would be something like u16
> > (rather than size_t) which shows very quickly the potential for
> > truncation. See, for example:
> >
> > https://lore.kernel.org/all/20230811054528.never.165-kees@kernel.org/
>
> (I'd found that one when I tried to build with min_t() being min().
> The bug was reported not long after!)
>
> Or an example using 'unsigned char' - there are some very dubious
> ones lurking.
>
> Also look at the code in tcp/udp that validates the length argument
> to getsockopt().
> It checks for negative after doing min_t(unsigned, len, 4).
> It has always been thus, well before min_t() was added.
>
> ...
> > >  /**
> > >   * min_t - return minimum of two values, using the specified type
> > > + *
> > > + * Note: Downcasting types in this macro can cause incorrect results=
 Prefer to
> > > + * use min() which does typechecking.
> > > + *
> > > + * Prefer to use clamp if you are trying to compare to size_t.
> > > + *
> > > + * Don't:
> > > + *   min_t(size_t, buf_size, sizeof(foobar))
> > > + *
> > > + * Do:
> > > + *  clamp(buf_size, 0, sizeof(foobar))
>
> I'm not at all sure that is actually helpful.
> It might be better to just note that min_t(unsigned type, int val, xxx)
> will convert a negative value to a large positive one.
>
> In you case size_t is just the wrong type.
> You need to change the type of the constant (to int) not the
> type of the variable.
> So you want:
>         min(buf_size, (int)sizeof(foobar))
>
> I'm not at all sure that min_t() (casting both args) is actually
> a good idea, requiring the codes explicitly cast one (usually only
> one needs a cast) is likely to be less buggy, more obvious, and
> less typing.
>
> I think min_t() exists because it is an exact replacement for
> a static inline function where the cast was implicit in the call.
>
> Linus didn't like the change that would allow:
>         min(int_size, sizeof(fubar))
> (ie implicitly casting unsigned constants to int before
> the compare.)
> It does make the defines rather more complicated.
>
> Thinking... it might me easier to add smin() (cf umin())
> that will convert an unsigned constant to int
> (and error for non-constant unsigned arguments).
> That would be much safer than min_t() and save all the extra
> complication min() would need, and also annotate the source.

I seemed to have somehow entirely missed umin and the static assert
you added as it does exactly the thing that I would want out of the
docs. https://lore.kernel.org/all/fe7e6c542e094bfca655abcd323c1c98@AcuMS.ac=
ulab.com/

I went back to my working tree (on 6.6) and rebased and I see the
following error:
  min(16, buf_size) signedness error, fix types or consider umin()
before min_t()

This works great! Thanks for adding this!

>
> A long term plan would be to remove all the min_t() and max_t().
> Sorting out some patches for simple cases (both args unsigned
> and the same size would be a start) isn't that hard.
> But they do need to get applied.
>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1=
 1PT, UK
> Registration No: 1397386 (Wales)
>

Please consider this patch closed/abandoned since
https://lore.kernel.org/all/fe7e6c542e094bfca655abcd323c1c98@AcuMS.aculab.c=
om/
is already merged.