linux-kernel - Re: [PATCH next 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: <ZL+mIJiXAJaXDSJF@smile.fi.intel.com>
Date:   Tue, 25 Jul 2023 13:38:24 +0300
From:   'Andy Shevchenko' <andriy.shevchenko@...ux.intel.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        '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 next 0/5] minmax: Relax type checks in min() and max().

On Tue, Jul 25, 2023 at 10:00:40AM +0000, David Laight wrote:
> The min() (etc) functions in minmax.h require that the arguments have
> exactly the same types. This was probably added after an 'accident'
> where a negative value got converted to a large unsigned value.
> 
> However when the type check fails, rather than look at the types and
> fix the type of a variable/constant, everyone seems to jump on min_t().
> In reality min_t() ought to be rare - when something unusual is being
> done, not normality.
> If the wrong type is picked (and it is far too easy to pick the type
> of the result instead of the larger input) then significant bits can
> get discarded.
> Pretty much the worst example is in the derfved clamp_val(), consider:
> 	unsigned char x = 200u;
> 	y = clamp_val(x, 10u, 300u);
> 
> 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.
> 
> The only reason that most of the min_t() are 'fine' is that pretty
> much all the value in the kernel are between 0 and INT_MAX.
> 
> Patch 1 adds min_unsigned(), this uses integer promotions to convert
> both arguments to 'unsigned long long'. It can be used to compare a
> signed type that is known to contain a non-negative value with an
> unsigned type. The compiler typically optimises it all away.
> Added first so that it can be referred to in patch 2.
> 
> Patch 2 replaces the 'same type' check with a 'same signedness' one.
> This makes min(unsigned_int_var, sizeof()) be ok.
> The error message is also improved and will contain the expanded
> form of both arguments (useful for seeing how constants are defined).
> 
> Patch 3 just fixes some whitespace.
> 
> Patch 4 allows comparisons of 'unsigned char' and 'unsigned short'
> to signed types. The integer promotion rules convert them both
> to 'signed int' prior to the comparison so they can never cause
> a negative value be converted to a large positive one.
> 
> Patch 5 is slightly more contentious (Linus may not like it!)
> effectively adds an (int) cast to all constants between 0 and MAX_INT.
> This makes min(signed_int_var, sizeof()) be ok.
> 
> With all the patches applied pretty much all the min_t() could be
> replaced by min(), and most of the rest by min_unsigned().
> However they all need careful inspection due to code like:
> 	sz = min_t(unsigned char, sz - 1, LIM - 1) + 1;
> which converts 0 to LIM.

I don't know how you made this series, but it has no thread. You need to use
--thread when forming the patch series.

-- 
With Best Regards,
Andy Shevchenko