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

From: Kees Cook
Date: Mon Aug 21 2023 - 14:24:16 EST


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