Messages in this thread | | | From | Abhishek Pandit-Subedi <> | Date | Mon, 12 Feb 2024 09:55:29 -0800 | 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@aculab.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@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 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.
| |