netdev - Re: [RFC][PATCH] apparent big-endian bugs in dwc-xlgmac

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: <51551924-28b6-2c1c-2b3d-1d307e1d70ab@synopsys.com>
Date:   Mon, 11 Dec 2017 14:18:08 +0800
From:   Jie Deng <Jie.Deng1@...opsys.com>
To:     Al Viro <viro@...IV.linux.org.uk>,
        Jie Deng <Jie.Deng1@...opsys.com>
CC:     <netdev@...r.kernel.org>
Subject: Re: [RFC][PATCH] apparent big-endian bugs in dwc-xlgmac

On 2017/12/11 13:05, Al Viro wrote:

> On Mon, Dec 11, 2017 at 12:33:42PM +0800, Jie Deng wrote:
>> Hi AI Viro,
>>> @@ -125,8 +125,8 @@
>>>  	typeof(len) _len = (len);					\
>>>  	typeof(val) _val = (val);					\
>>>  	_val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos);		\
>>> -	_var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val;		\
>>> -	cpu_to_le32(_var);						\
>>> +	(_var & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | 	\
>>> +		cpu_to_le32(_val);					\
>>>  })
>>>  
>>>  struct xlgmac_pdata;
>> Make sense.  But I think what you want is fix as follows. Right ?
>>
>> @@ -125,8 +125,8 @@
>>  	typeof(len) _len = (len);						\
>> - 	typeof(val) _val = (val);						\
>> +       u32 _var = le32_to_cpu((var));                                          \ 	
>>         _val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos);			\
>> -	_var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val;			\
>> -	cpu_to_le32(_var);							\
>> +	(cpu_to_le32(_var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | 	\
>> +		cpu_to_le32(_val);						\
>>  })
> What for?  Sure, this variant will work, but why bother with
> 	a = le32_to_cpu(b);
> 	(cpu_to_le32(a) & ....) | ....
> and how is that better than
> 	(b & ...) | ...
>
> IDGI...  Mind you, I'm not sure if there is any point keeping _var in that thing,
> seeing that we use var only once - might be better off with
> 	((var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | 	\
> 		cpu_to_le32(_val);					\
I agree. Could you please send the patch with this better fix ?