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: <20171211050520.GV21978@ZenIV.linux.org.uk>
Date:   Mon, 11 Dec 2017 05:05:20 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Jie Deng <Jie.Deng1@...opsys.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [RFC][PATCH] apparent big-endian bugs in dwc-xlgmac

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);					\