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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <420a198d-61f8-81cf-646d-10446cb41def@synopsys.com>
Date:   Mon, 11 Dec 2017 12:33:42 +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

Hi AI Viro,


On 2017/12/10 12:53, Al Viro wrote:
> In xlgmac_dev_xmit():
>
>                         /* Mark it as a CONTEXT descriptor */
>                         dma_desc->desc3 = XLGMAC_SET_REG_BITS_LE(
>                                                 dma_desc->desc3,
>                                                 TX_CONTEXT_DESC3_CTXT_POS,
>                                                 TX_CONTEXT_DESC3_CTXT_LEN,
>                                                 1);
>
>
>
> Looking at XLGMAC_SET_REG_BITS_LE() we see this:
> #define XLGMAC_SET_REG_BITS_LE(var, pos, len, val) ({                   \
>         typeof(var) _var = (var);                                       \
>         typeof(pos) _pos = (pos);                                       \
>         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);                                              \
> })
>
> That thing assumes var to be host-endian and has a little-endian result.
> Unfortunately, we feed it a little-endian and store the result back into
> the same place.  That might work if the original values *was* host-endian
> and we wanted to end up with little-endian.  However, that is immediately
> followed by
>                         /* Indicate this descriptor contains the MSS */
>                         dma_desc->desc3 = XLGMAC_SET_REG_BITS_LE(
>                                                 dma_desc->desc3,
>                                                 TX_CONTEXT_DESC3_TCMSSV_POS,
>                                                 TX_CONTEXT_DESC3_TCMSSV_LEN,
>                                                 1);
>
> where we operate on the now definitely little-endian value.  That really
> can't be right.  I don't have the hardware in question, so I can't test
> that, but it smells like this needs something like diff below, making
> XLGMAC_SET_REG_BITS_LE take le32 and return le32.  GET side of things
> is le32 -> u32; definition looks correct, but slightly misannotated.
>
> Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
> ---
> diff --git a/drivers/net/ethernet/synopsys/dwc-xlgmac.h b/drivers/net/ethernet/synopsys/dwc-xlgmac.h
> index cab3e40a86b9..e95c4c250e16 100644
> --- a/drivers/net/ethernet/synopsys/dwc-xlgmac.h
> +++ b/drivers/net/ethernet/synopsys/dwc-xlgmac.h
> @@ -106,7 +106,7 @@
>  #define XLGMAC_GET_REG_BITS_LE(var, pos, len) ({			\
>  	typeof(pos) _pos = (pos);					\
>  	typeof(len) _len = (len);					\
> -	typeof(var) _var = le32_to_cpu((var));				\
> +	u32 _var = le32_to_cpu((var));				\
>  	((_var) & GENMASK(_pos + _len - 1, _pos)) >> (_pos);		\
>  })
>  
> @@ -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);						\
 })