Re: [PATCH next v3 0/5] minmax: Relax type checks in min() and max().

[Kees Cook <keescook@chromium.org>]

On Tue, Aug 15, 2023 at 08:55:55AM +0000, David Laight wrote:
> From: Kees Cook
> > Sent: 14 August 2023 22:21
> > 
> > On Fri, Aug 04, 2023 at 10:50:59AM +0000, David Laight wrote:
> > > [...]
> > > I also suspect that many of the min_t(u16, ...) are actually wrong.
> > > For example copy_data() in printk_ringbuffer.c contains:
> > >         data_size = min_t(u16, buf_size, len);
> > > Here buf_size is 'unsigned int' and len 'u16', pass a 64k buffer
> > > (can you prove that doesn't happen?) and no data is returned.
> > 
> > Stars aligning... this exact bug (as you saw in the other thread[1]) got
> > hit. And in the analysis, I came to the same conclusion: min_t() is a
> > serious foot-gun, and we should be able to make min() Just Work in the
> > most common situations.
> 
> It is all a question of what 'work' means.
> To my mind (but Linus disagrees!) the only problematic case
> is where a negative signed value gets converted to a large
> unsigned value.
> This snippet from do_tcp_getsockopt() shows what I mean:
> 
> 	copy_from_user(&len,...)
> 	len = min_t(unsigned int, len, sizeof(int));
> 
> 	if (len < 0)
> 		return -EINVAL;
> 
> That can clearly never return -EINVAL.
> That has actually been broken since the test was added in 2.4.4.
> That predates min_t() in 2.4.10 (renamed from min() in 2.4.9
> when the 'strict typecheck' version on min() was added).
> So min_t() actually predates min()!
> 
> > 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?

For example:

	int a = ...;
	unsigned int b = ...;
	int c = min_t(unsigned int, a, b);

This is goes bad when "a < 0". And it violates your case (1) above.

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.

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".

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).

-Kees

-- 
Kees Cook