Message-ID: <CANFp7mVm6QQ0+rm_VJ6MJbVDM+agyTd+5cEwE3EneSxS1-z70w@mail.gmail.com>
Date: Mon, 12 Feb 2024 09:55:29 -0800
From: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
To: David Laight <David.Laight@...lab.com>
Cc: Kees Cook <keescook@...omium.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"pmalani@...omium.org" <pmalani@...omium.org>, 
	"andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>, 
	"torvalds@...ux-foundation.org" <torvalds@...ux-foundation.org>, Andrew Morton <akpm@...ux-foundation.org>, 
	Herve Codina <herve.codina@...tlin.com>, "Matthew Wilcox (Oracle)" <willy@...radead.org>
Subject: Re: [PATCH] minmax: Add notes to min_t and max_t

On Sat, Feb 10, 2024 at 4:04 AM David Laight <David.Laight@...lab.com> 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@AcuMS.aculab.com/
> > > Link: https://lore.kernel.org/all/CAHk-
> > =whwEAc22wm8h9FESPB5X+P4bLDgv0erBQMa1buTNQW7tA@...l.gmail.com/
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@...omium.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 devs
> > > (**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.aculab.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.com/
is already merged.

Powered by blists - more mailing lists