netdev - Re: [v4] Fix to avoid IS_ERR_VALUE and IS

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: <DB5PR0401MB19280ACA87C7F5EE2822D5FB91040@DB5PR0401MB1928.eurprd04.prod.outlook.com>
Date:	Mon, 1 Aug 2016 16:55:43 +0000
From:	Scott Wood <scott.wood@....com>
To:	Arnd Bergmann <arnd@...db.de>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>
CC:	Arvind Yadav <arvind.yadav.cs@...il.com>,
	"zajec5@...il.com" <zajec5@...il.com>,
	"leoli@...escale.com" <leoli@...escale.com>,
	"qiang.zhao@...escale.com" <qiang.zhao@...escale.com>,
	"viresh.kumar@...aro.org" <viresh.kumar@...aro.org>,
	"linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
	"David.Laight@...lab.com" <David.Laight@...lab.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"scottwood@...escale.com" <scottwood@...escale.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"linux@...ck-us.net" <linux@...ck-us.net>
Subject: Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit
 systems.

On 08/01/2016 02:02 AM, Arnd Bergmann wrote:
> On Sunday, July 31, 2016 4:48:44 PM CEST Arvind Yadav wrote:
>> IS_ERR_VALUE() assumes that parameter is an unsigned long.
>> It can not be used to check if 'unsigned int' is passed insted.
>> Which tends to reflect an error.
>>
>> In 64bit architectures sizeof (int) == 4 && sizeof (long) == 8.
>> IS_ERR_VALUE(x) is ((x) >= (unsigned long)-4095).
>>
>> IS_ERR_VALUE() of 'unsigned int' is always false because the 32bit
>> value is zero extended to 64 bits.
>>
>> Value of (unsigned int)-4095 is always less than value of
>> (unsigned long)-4095.
>>
>> Now We are taking only first 32 bit for error checking rest of the 32 bit
>> we ignore such that we get appropriate comparison on 64bit system as well.
> 
> This is completely wrong: if you have a valid 64-bit pointer like
> 0x00001234ffffff00, this will be interpreted as an error now.
> 
>> First 32bit of Value of (unsigned int)-4095 and (unsigned long)-4095 will
>> be equal.
>>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@...il.com>
>> ---
>>  include/linux/err.h | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/err.h b/include/linux/err.h
>> index 1e35588..c2a2789 100644
>> --- a/include/linux/err.h
>> +++ b/include/linux/err.h
>> @@ -18,7 +18,17 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  
>> -#define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO)
>> +#define IS_ERR_VALUE(x) unlikely(is_error_check(x))
>> +
>> +static inline int is_error_check(unsigned long error)
> 
> Please leave the existing macro alone. I think you were looking for
> something specific to the return code of qe_muram_alloc() function,
> so please add a helper in that subsystem if you need it, not in
> the generic header files.

qe_muram_alloc (a.k.a. cpm_muram_alloc) returns unsigned long.  The
problem is certain callers that store the return value in a u32.  Why
not just fix those callers to store it in unsigned long (at least until
error checking is done)?

-Scott