minmax: Add notes to min_t and max_t

  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.


 include/linux/minmax.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
  

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 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/

> 
> 
>  include/linux/minmax.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> index 2ec559284a9f..96646f840a1f 100644
> --- a/include/linux/minmax.h
> +++ b/include/linux/minmax.h
> @@ -154,6 +154,18 @@
>  
>  /**
>   * 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))
> + *
>   * @type: data type to use
>   * @x: first value
>   * @y: second value

Please keep the types immediately after the definition -- notes can go
after.

> @@ -162,6 +174,10 @@
>  
>  /**
>   * max_t - return maximum of two values, using the specified type
> + *
> + * Note: Downcasting types in this macro can cause incorrect results. Prefer to
> + * use max() which does typechecking.
> + *
>   * @type: data type to use
>   * @x: first value
>   * @y: second value

Same.

But yes, I welcome the added comments! :)
  

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.

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)
  

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.