From: Kees Cook <keescook@chromium.org>
> Sent: Monday, August 21, 2023 7:24 PM
....
> > > It seems like the existing type_max/type_min macros could be used to
> > > figure out that the args are safe to appropriately automatically cast,
> > > etc. e.g. type_max(u16) <= type_max(unsigned int) && type_min(u16) >=
> > > type_min(unsigned int) ...
> >
> > That doesn't really help; min(a,b) is ok if any of:
> > 1) is_signed(a) == is_signed(b).
> > 2) is_signed(a + 0) == is_signed(b + 0)  // Converts char/short to int.
> > 3) a or b is a constant between 0 and MAXINT and is cast to int.
> >
> > The one you found passes (1) - both types are unsigned.
> > min(len, sizeof (int)) passes (3) and is converted to
> > min(len, (int)sizeof (int)) and can still return the expected negatives.
> 
> It seems like the foot-gun problems are when a value gets clamped by the
> imposed type. Can't we just warn about those cases?

Also when -1 gets converted to 0xffffffff.

...
> But this is also unsafe:
> 
> 	unsigned int a = ...;
>         u16 b = ...;
> 	unsigned int c = min_t(u16, a, b);
> 
> Both are unsigned, but "a > U16_MAX" still goes sideways.

Right, this is one reason why min_t() is broken.
If min() allowed that - no reason why it shouldn't - then it wouldn't
get written in the first place.

> I worry that weakening the min/max() type checking gets into silent errors:
> 
> 	unsigned int a = ...;
>         u16 b = ...;
> 	u16 c = max(a, b);
> 
> when "a > U16_MAX".

Nothing can be done on the RHS to detect invalid narrowing in assignments.
And you don't want the compiler to complain because that will just cause
explicit casts be added making the code harder to read and (probably)
adding more bugs.

> Looking at warning about clamped types on min_t(), though I see tons of
> int vs unsigned int issue. (e.g. dealing with PAGE_SIZE vs an int).

Linus doesn't like me silently converting unsigned constants
to signed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Wed, 23 Aug 2023 at 01:52, David Laight <David.Laight@aculab.com> wrote:
>
> Linus doesn't like me silently converting unsigned constants
> to signed.

Note that my dislike is more about changing the sign of the
*comparison*, and not warning about it.

It's not the constant conversion itself that ends up being the
problem, but the downstream issues it causes.

Having grepped for those annoying "min_t(size_t..)" uses, lots of them
seem to have perfectly fine signedness, and the 'size_t' is literally
just due to different sizes of unsigned values. In fact, several of
them seem to be due to the unfortunate fact that 'size_t' can be
'unsigned int' on 32-bit architectures, so mixing 'size_t' and
'unsigned long' will sadly warn without it.

So we literally have the issue that 'min(a,b)' will warn even if 'a'
and 'b' have the same signedness *and* the same size, because 'size_t'
and 'unsigned long' are different types.

Your patch 2/5 will fix that. And then I'd certainly be ok with a
"comparing an unsigned value to a signed positive constant" thing
(just not the other way around: "comparing a signed value to an
unsigned positive constant" is wrong)

That might get rid of a number of the more annoying cases.

            Linus

From: Linus Torvalds
> Sent: Wednesday, August 23, 2023 4:32 PM
...
> That might get rid of a number of the more annoying cases.

The one it leaves is code like:

	int length = get_length(...);
	if (length <= 0)
		return error:
	do {
		frag_len = some_min_function(length, PAGE_SIZE);
		...
	} while ((length -= frag_len) != 0);

As written it is ok for all reasonable some_min_function().

But if the (length <= 0) test is missing it really doesn't
matter what some_min_function() returns because the code
isn't going to do anything sensible - and may just loop.

About the only thing you could do is add a run-time check
and then BUG() if negative.
But that is horrid...

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)