linux-kernel - RE: [PATCH v3 5/5] minmax: Relax check to allow comparison between int and small unsigned constants.

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]

Date:   Mon, 7 Aug 2023 10:50:07 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Linus Torvalds' <torvalds@...ux-foundation.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>
Subject: RE: [PATCH v3 5/5] minmax: Relax check to allow comparison between
 int and small unsigned constants.

From: Linus Torvalds
> Sent: 04 August 2023 19:14
> 
> On Fri, 4 Aug 2023 at 03:56, David Laight <David.Laight@...lab.com> wrote:
> >
> > Convert constants between 0 and INT_MAX to 'int' prior to comparisons
> > so that min(signed_var, 20u) and, more commonly, min(signed_var, sizeof())
> > are both valid.
> 
> I really think this whole series is broken.
> 
> What does the "are both valid" even *MEAN*?

To my mind the value of min(variable, TWENTY) shouldn't depend
on how TWENTY is defined regardless of the type of the variable.
So 20, 20u, 20l, 20ul, (char)20, sizeof (foo), offsetof(x, y)
should all be equally valid and all generate the same result.

Note that the patches still reject min(unsigned_var, -1),
min(signed_var, 0x80000000u) and min(signed_var, unsigned_var)
(unless the unsigned_var is char/short).

It isn't as though all the constants really have the 'correct'
type. I found one clamp(val, MIN_FOO, MAX_FOO) where MIN_FOO
was 20 and MAX_FOO an expression including sizeof().

One of the problems I'm trying to solve is that pretty much
no one seems to have gone back through the definitions to
fix a type error reported by min(), what always happens is
they decide the answer is min_t().

Unfortunately it is all too easy to pick the wrong type.
With a = min(b, limit) often typeof(a) is picked or, even
worse, typeof(b) without any regard for whether that can
truncate 'limit'.
In essence the casting done in min_t() is really worse than
having a min() that doesn't to the type check at all.
Both are likely to convert negative values to large positive
ones, but min_t() can also mask off high bits.

I've found all sorts of dubious min_t() while writing these patches.
One in a filesystem was min_t(ulong, ulong_var, u64_var) and I
couldn't convince myself it was right on 32bit.

Checking that both values have the same signedness (patch 3)
removes a lot of the requirements for min_t().
In particular it allows min(uint_var, sizeof ()).

Re-checking with unsigned char/short promoted to int (as happens
pretty much as soon as you breath on the value).
This means that checks of char structure members are likely
to be accepted without likely incorrect '& 0xff) being
applied to the other value by a min_t(u8, a, b).

	David

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