netdev - Re: [PATCH 1/2] lib: test_scanf: Fix incorrect use of type

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: <0650840d-1b7d-3bc0-c04f-3a22ddc1ced1@opensource.cirrus.com>
Date:   Tue, 25 May 2021 11:10:10 +0100
From:   Richard Fitzgerald <rf@...nsource.cirrus.com>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>, <pmladek@...e.com>,
        <rostedt@...dmis.org>, <sergey.senozhatsky@...il.com>,
        <andriy.shevchenko@...ux.intel.com>, <w@....eu>, <lkml@....org>,
        <davem@...emloft.net>, <kuba@...nel.org>
CC:     <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <patches@...nsource.cirrus.com>
Subject: Re: [PATCH 1/2] lib: test_scanf: Fix incorrect use of type_min() with
 unsigned types

On 25/05/2021 10:55, Rasmus Villemoes wrote:
> On 24/05/2021 17.59, Richard Fitzgerald wrote:
>> sparse was producing warnings of the form:
>>
>>   sparse: cast truncates bits from constant value (ffff0001 becomes 1)
>>
>> The problem was that value_representable_in_type() compared unsigned types
>> against type_min(). But type_min() is only valid for signed types because
>> it is calculating the value -type_max() - 1.

Ok, I see I was wrong about that. It does in fact work safely. Do you
want me to update the commit message to remove this?

> 
> ... and casts that to (T), so it does produce 0 as it should. E.g. for
> T==unsigned char, we get
> 
> #define type_min(T) ((T)((T)-type_max(T)-(T)1))
> (T)((T)-255 - (T)1)
> (T)(-256)
> 

sparse warns about those truncating casts.

> which is 0 of type unsigned char.
> 
> The minimum value of an
>> unsigned is obviously 0, so only type_max() need be tested.
> 
> That part is true.
> 
> But type_min and type_max have been carefully created to produce values
> of the appropriate type that actually represent the minimum/maximum
> representable in that type, without invoking UB. If this program doesn't
> produce the expected results for you, I'd be very interested in knowing
> your compiler version:
> 

 From the kernel test robot report:

compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce:
         wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross 
-O ~/bin/make.cross
         chmod +x ~/bin/make.cross
         # apt-get install sparse
         # sparse version: v0.6.3-341-g8af24329-dirty
         # 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=50f530e176eac808e64416732e54c0686ce2c39b
         git remote add linux-next 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
         git fetch --no-tags linux-next master
         git checkout 50f530e176eac808e64416732e54c0686ce2c39b
         # save the attached .config to linux build tree
         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cro

I get the same warnings with Linaro GCC 7.5-2019.12) and sparse
v0.6.3-184-g1b896707.

> #include <stdio.h>
> 
> #define is_signed_type(type)       (((type)(-1)) < (type)1)
> #define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 -
> is_signed_type(type)))
> #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
> #define type_min(T) ((T)((T)-type_max(T)-(T)1))
> 
> int main(int argc, char *argv[])
> {
> #define p(T, PT, fmt) do {					\
> 		PT vmin = type_min(T);				\
> 		PT vmax = type_max(T);				\
> 		printf("min(%s) = "fmt", max(%s) = "fmt"\n",#T, vmin, #T, vmax); \
> 	} while (0)
> 
> 	p(_Bool, int, "%d");
> 	p(unsigned char, int, "%d");
> 	p(signed char, int, "%d");
> 	p(unsigned int, unsigned int, "%u");
> 	p(unsigned long long, unsigned long long, "%llu");
> 	p(signed long long, signed long long, "%lld");
> 	
> 	return 0;
> }
> 
> 
> 
>>   lib/test_scanf.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/test_scanf.c b/lib/test_scanf.c
>> index 8d577aec6c28..48ff5747a4da 100644
>> --- a/lib/test_scanf.c
>> +++ b/lib/test_scanf.c
>> @@ -187,8 +187,8 @@ static const unsigned long long numbers[] __initconst = {
>>   #define value_representable_in_type(T, val)					 \
>>   (is_signed_type(T)								 \
>>   	? ((long long)(val) >= type_min(T)) && ((long long)(val) <= type_max(T)) \
>> -	: ((unsigned long long)(val) >= type_min(T)) &&				 \
>> -	  ((unsigned long long)(val) <= type_max(T)))
>> +	: ((unsigned long long)(val) <= type_max(T)))
> 
> 
> With or without this, these tests are tautological when T is "long long"
> or "unsigned long long". I don't know if that is intended. But it won't,
> say, exclude ~0ULL if that is in the numbers[] array from being treated
> as fitting in a "long long".

I don't entirely understand your comment. But the point of the test is
to exclude values that can't be represented by a type shorter than
long long or unsigned long long.
> 
> Rasmus
>