Message-ID: <65ba51bc65b24c13af4ce02e79f78dee@AcuMS.aculab.com>
Date: Sat, 10 Feb 2024 12:04:13 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Kees Cook' <keescook@...omium.org>, Abhishek Pandit-Subedi
	<abhishekpandit@...omium.org>
CC: "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

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.

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)

Powered by blists - more mailing lists