linux-kernel - RE: [PATCH 0/1] Slightly relax the type checking done by 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: <03db8d2f34fa4bffada8b34ee87b090a@AcuMS.aculab.com>
Date:   Fri, 25 Nov 2022 19:47:14 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Andy Shevchenko' <andriy.shevchenko@...ux.intel.com>
CC:     LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        "'Joe Perches'" <joe@...ches.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: RE: [PATCH 0/1] Slightly relax the type checking done by min() and
 max().

From: 'Andy Shevchenko'
> Sent: 25 November 2022 17:48
> 
> On Fri, Nov 25, 2022 at 04:14:58PM +0000, David Laight wrote:
> > From: 'Andy Shevchenko'
> > > Sent: 25 November 2022 15:58
> > > On Fri, Nov 25, 2022 at 03:27:07PM +0000, David Laight wrote:
> 
> ...
> 
> > > Any better example, please?
> >
> > How about:
> 
> Better, indeed.
> 
> > data_size = min_t(u16, buf_size, len);
> >
> > https://elixir.bootlin.com/linux/v6.1-rc6/source/kernel/printk/printk_ringbuffer.c#L1738
> >
> > Now, maybe, you could claim that buf_size > 64k never happens.
> > But the correct cast here is u32 to match buf_size.
> > len (being u16) will be promoted to int before the compare.
> >
> > Just search the kernel for "min_t(u8," or "min_t(u16," while some might
> > be ok, I really wouldn't want to verify each case.
> >
> > If you look hard enough there are also some:
> > 	u32_var = min_t(u32, u32_val, u64_val);
> > where the intent is to limit values that might be invalid for u32.
> 
> Wouldn't be better to actually issue a warning if the desired type is shorter
> than one of the min_t() arguments?
> 
> Then you go through all cases and fix them accordingly.

That is an option, but that is separate from this change.

> Blindly relaxing the rules is not an option in my opinion.

The point is I'm not really relaxing the rules.
If anything I'm actually tightening them by allowing min() to
be used in more cases - so letting the buggy min_t() casts be
removed at some point in the future.

The purpose of the type check is to error code like:
	int x = -4;
	x = min(x, 100u);
where integer promotion changes the comparison 'x < 100u' to
the unexpected '(unsigned int)x < 100u' so x is set to 100.

However is also errors the opposite:
	unsigned int x = 4;
	x = min(x, 100);
But, in this case, the compiler just converts 100 to 100u and
everything is fine.

It also errors the perfectly valid (and reasonable looking):
	unsigned char x = 4;
	x = min(x, 100u);
because 100u is 'unsigned int'.
Indeed, 'x' gets converted to 'signed int' for the comparison.

When the (usually) RHS is a compile-time constant that is known
to be positive (and less than 2^31) I'm just changing the test to:
	if (variable < (int)constant)
If 'variable' is a signed type this always generates the
required signed compare.
If 'variable is 'unsigned char' or 'unsigned short' then it
gets promoted to signed int and a signed compare of positive
values is done.
If variable is 'unsigned int', 'unsigned long' or 'unsigned long long'
then the RHS is converted to unsigned - but keeps its value since
it is known to be positive.
In all cases the outcome is exactly what is required.
No change of putting in the wrong cast.

There are a few other common cases that are valid but currently
generate an error:
	min(s8_var, int_expr)
	min(u8_var, int_expr)
	min(u8_var, unsigned_int_expr)
These generate an error regardless of whether the sizes match:
	min(int_expr, long_expr)
	min(unsigned_int_expr, unsigned_long_expr)
	min(unsigned_long_expr, unsigned_long_long_expr)
This is less common but should also be allowed:
	min(long_long_int_expr, unsigned_int_expr)
(ie when the signed type is larger than the unsigned one)

I have a plan for how to allow all those as well.

Checking typeof((x) + 0) against typeof((y) + 0) allows for the
promotion of char/short to signed int - but not any of the
other comparisons that never implicitly convert a signed value
to unsigned (and hence negative values to large positive ones).

	David

	

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