Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   David Laight <David.Laight@ACULAB.COM>
To:     'Kees Cook' <keescook@chromium.org>
CC:     "'linux-kernel@vger.kernel.org'" <linux-kernel@vger.kernel.org>,
        "'Andy Shevchenko'" <andriy.shevchenko@linux.intel.com>,
        'Andrew Morton' <akpm@linux-foundation.org>,
        "'Matthew Wilcox (Oracle)'" <willy@infradead.org>,
        'Christoph Hellwig' <hch@infradead.org>,
        "'Jason A. Donenfeld'" <Jason@zx2c4.com>,
        Linus Torvalds <torvalds@linux-foundation.org>
Subject: RE: [PATCH next v3 0/5] minmax: Relax type checks in min() and max().
Thread-Topic: [PATCH next v3 0/5] minmax: Relax type checks in min() and
 max().
Thread-Index: AdnGwQ6IGYkn0IjZSjuTaOSyeQI0UwIK8m4AABl1+KA=
Date:   Tue, 15 Aug 2023 08:55:55 +0000
Message-ID: <2dd09c4033644239a314247e635fa735@AcuMS.aculab.com>
References: <01e3e09005e9434b8f558a893a47c053@AcuMS.aculab.com>
 <202308141416.89AC5C2@keescook>
In-Reply-To: <202308141416.89AC5C2@keescook>
Accept-Language: en-GB, en-US
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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.

(Clearly that getsockopt code only works for len != 4 on LE.)

	David

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