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

From: Linus Torvalds
> Sent: 27 November 2022 21:54
> 
> On Sun, Nov 27, 2022 at 1:42 PM David Laight <David.Laight@...lab.com> wrote:
> >
> > Why should it be a problem?
> > min(-4, sizeof(X)) becomes min(-4, (int)sizeof(X)) and thus -4.
> > Without the cast the -4 is converted to a very large unsigned
> > value so the result is sizeof(X) - not at all expected.
> 
> That is EXACTLY the problem.
> 
> You even enumerate it, and work through exactly what happens, and then
> you STILL say "this is not a problem".
> 
> It damn well is a HUGE problem. When people say "I need my offset to
> be smaller than the size of the object", then a value like -4 IS NOT
> ACCEPTABLE. It should cause a huge type warning about how the test was
> broken.
> 
> David, this is literally *EXACTLY* why we have those strict type issues.
> 
> The fact that you don't even seem to realize why this would be a
> problem makes me NAK this patch so hard that it isn't even funny.
> 
> Andrew, please remove this from your queue. It's not even remotely
> acceptable. I was hoping I was misreading the patch, but it turns out
> that this "relax the rules way too much" was apparently intentional.

I guess you're the boss :-)

But what actually happens is the compiler bleats about min()
so rather then change a constant to be unsigned (etc) the code
is rewritten with min_t() and both sides are cast to (usually)
an unsigned type.
There are a non-zero number of cases where the cast masks high
bits off the large value.

Given the number of min_t(u8,x,y) and min_t(u16,x,y) it is
pretty clear a lot of people don't actually know the C arithmetic
promotion rules.

Forcing an unsigned comparison can be done by adding having:
#define min_unsigned(x, y) min((x) + 0u + 0ull, (y) + 0u + 0ull)
that will never mask off bits and generates sane code.
Almost all the min_t() could be replaced by that definition.

	David

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