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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2dd09c4033644239a314247e635fa735@AcuMS.aculab.com>
Date:   Tue, 15 Aug 2023 08:55:55 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Kees Cook' <keescook@...omium.org>
CC:     "'linux-kernel@...r.kernel.org'" <linux-kernel@...r.kernel.org>,
        "'Andy Shevchenko'" <andriy.shevchenko@...ux.intel.com>,
        'Andrew Morton' <akpm@...ux-foundation.org>,
        "'Matthew Wilcox (Oracle)'" <willy@...radead.org>,
        'Christoph Hellwig' <hch@...radead.org>,
        "'Jason A. Donenfeld'" <Jason@...c4.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: RE: [PATCH next v3 0/5] minmax: Relax type checks in min() and max().

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)