[PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[Fabio M. De Francesco]

Use crc32_le in place of the custom getcrc32. This change makes GCC
to warn about incorrect castings to the restricted type __le32, but
they can be safely ignored because crc32_le calculates bitwise
little-endian Ethernet AUTODIN II CRC32.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/rtl8188eu/core/rtw_security.c | 22 ++++---------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c b/drivers/staging/rtl8188eu/core/rtw_security.c
index 1b2cb6196463..5f010cb66970 100644
--- a/drivers/staging/rtl8188eu/core/rtw_security.c
+++ b/drivers/staging/rtl8188eu/core/rtw_security.c
@@ -111,21 +111,6 @@ static void crc32_init(void)
 	bcrc32initialized = 1;
 }
 
-static __le32 getcrc32(u8 *buf, int len)
-{
-	u8 *p;
-	u32  crc;
-
-	if (bcrc32initialized == 0)
-		crc32_init();
-
-	crc = 0xffffffff;       /* preload shift register, per CRC-32 spec */
-
-	for (p = buf; len > 0; ++p, --len)
-		crc = crc32_table[(crc ^ *p) & 0xff] ^ (crc >> 8);
-	return cpu_to_le32(~crc);    /* transmit complement, per CRC-32 spec */
-}
-
 /* Need to consider the fragment  situation */
 void rtw_wep_encrypt(struct adapter *padapter, struct xmit_frame *pxmitframe)
 {
@@ -609,14 +594,15 @@ u32	rtw_tkip_encrypt(struct adapter *padapter, struct xmit_frame *pxmitframe)
 
 				if ((curfragnum + 1) == pattrib->nr_frags) {	/* 4 the last fragment */
 					length = pattrib->last_txcmdsz - pattrib->hdrlen - pattrib->iv_len - pattrib->icv_len;
-					*((__le32 *)crc) = getcrc32(payload, length);/* modified by Amy*/
+					*((__le32 *)crc) = ~crc32_le(~0, payload, length);
 
 					arcfour_init(&mycontext, rc4key, 16);
 					arcfour_encrypt(&mycontext, payload, payload, length);
 					arcfour_encrypt(&mycontext, payload + length, crc, 4);
 				} else {
 					length = pxmitpriv->frag_len - pattrib->hdrlen - pattrib->iv_len - pattrib->icv_len;
-					*((__le32 *)crc) = getcrc32(payload, length);/* modified by Amy*/
+					*((__le32 *)crc) = ~crc32_le(~0, payload, length);
+
 					arcfour_init(&mycontext, rc4key, 16);
 					arcfour_encrypt(&mycontext, payload, payload, length);
 					arcfour_encrypt(&mycontext, payload + length, crc, 4);
@@ -682,7 +668,7 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, struct recv_frame *precvframe)
 			arcfour_init(&mycontext, rc4key, 16);
 			arcfour_encrypt(&mycontext, payload, payload, length);
 
-			*((__le32 *)crc) = getcrc32(payload, length - 4);
+			*((__le32 *)crc) = ~crc32_le(~0, payload, length - 4);
 
 			if (crc[3] != payload[length - 1] ||
 			    crc[2] != payload[length - 2] ||
-- 
2.32.0

RE: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[David Laight]

From: Fabio M. De Francesco
> Sent: 01 July 2021 14:38
> 
> Use crc32_le in place of the custom getcrc32. This change makes GCC
> to warn about incorrect castings to the restricted type __le32, but
> they can be safely ignored because crc32_le calculates bitwise
> little-endian Ethernet AUTODIN II CRC32.
> 
...
> -					*((__le32 *)crc) = getcrc32(payload, length);/* modified by Amy*/
> +					*((__le32 *)crc) = ~crc32_le(~0, payload, length);

Haven't we been round this before?
You need to get rid of these crappy casts.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[Fabio M. De Francesco]

On Thursday, July 1, 2021 4:52:08 PM CEST David Laight wrote:
> From: Fabio M. De Francesco
> 
> > Sent: 01 July 2021 14:38
> > 
> > Use crc32_le in place of the custom getcrc32. This change makes GCC
> > to warn about incorrect castings to the restricted type __le32, but
> > they can be safely ignored because crc32_le calculates bitwise
> > little-endian Ethernet AUTODIN II CRC32.
> 
> ...
> 
> > -					*((__le32 *)crc) = 
getcrc32(payload, length);/* modified by Amy*/
> > +					*((__le32 *)crc) = 
~crc32_le(~0, payload, length);
> 
> Haven't we been round this before?
>
No, I don't think so. At least, not you and I.

Unfortunately, I don't get paid to read every conversation sent to the Linux 
related mailing lists.

However, since you pointed out that somewhere, in an unspecified time, this 
topic had already been discussed I've just found an email of yours: https://
lore.kernel.org/lkml/bdc0c31a7d28426995d229eb9014cd2b@AcuMS.aculab.com/

I think that there you are talking of something else.

Perhaps, in the same thread there's an email from Al Viro that seems to be 
more pertinent: https://lore.kernel.org/lkml/YMz0DH0+v39xsCYU@zeniv-ca.linux.org.uk/

In particular, Al Viro writes "[...] Either make crc __le32 (and turn 
assignment into crc = cpu_to_le32(...)), or
make that *(__le32 *)crc = ...".

> You need to get rid of these crappy casts.

So, what am I missing?

Thanks,

Fabio
>
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 
1PT, UK
> Registration No: 1397386 (Wales)

RE: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[David Laight]

From: Fabio M. De Francesco
> Sent: 01 July 2021 16:24
> 
> On Thursday, July 1, 2021 4:52:08 PM CEST David Laight wrote:
> > From: Fabio M. De Francesco
> >
> > > Sent: 01 July 2021 14:38
> > >
> > > Use crc32_le in place of the custom getcrc32. This change makes GCC
> > > to warn about incorrect castings to the restricted type __le32, but
> > > they can be safely ignored because crc32_le calculates bitwise
> > > little-endian Ethernet AUTODIN II CRC32.
> >
> > ...
> >
> > > -					*((__le32 *)crc) =
> getcrc32(payload, length);/* modified by Amy*/
> > > +					*((__le32 *)crc) =
> ~crc32_le(~0, payload, length);
> >
> > Haven't we been round this before?
> >
> No, I don't think so. At least, not you and I.
> 

That was rt1872 this is rtl8188 but I think it is the same crap.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[Fabio M. De Francesco]

On Thursday, July 1, 2021 5:54:49 PM CEST David Laight wrote:
> From: Fabio M. De Francesco
> 
> > Sent: 01 July 2021 16:24
> > 
> > On Thursday, July 1, 2021 4:52:08 PM CEST David Laight wrote:
> > 
> > > From: Fabio M. De Francesco
> > >
> > >
> > >
> > > > Sent: 01 July 2021 14:38
> > > >
> > > >
> > > >
> > > > Use crc32_le in place of the custom getcrc32. This change makes GCC
> > > > to warn about incorrect castings to the restricted type __le32, but
> > > > they can be safely ignored because crc32_le calculates bitwise
> > > > little-endian Ethernet AUTODIN II CRC32.
> > >
> > >
> > >
> > > ...
> > >
> > >
> > >
> > > > -					*((__le32 *)crc) =
> > 
> > getcrc32(payload, length);/* modified by Amy*/
> > 
> > > > +					*((__le32 *)crc) =
> > 
> > ~crc32_le(~0, payload, length);
> > 
> > >
> > >
> > > Haven't we been round this before?
> > >
> > >
> > 
> > No, I don't think so. At least, not you and I.
> > 
> 
> 
> That was rt1872 this is rtl8188 but I think it is the same crap.
>
Perhaps it is the same crap... However, the patch is in accordance to one of 
the two solution that Al Viro wrote about.

I think I'll leave the patch as is and wait for the final review by Greg K-H.

Thanks,

Fabio
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 
1PT, UK
> Registration No: 1397386 (Wales)

Re: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5721 bytes --]

Hi "Fabio,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Fabio-M-De-Francesco/staging-rtl8188eu-Replace-a-custom-function-with-crc32_le/20210701-213922
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 77ad1f0e99bd00af024e650b862cfda3137af660
config: powerpc64-randconfig-s032-20210709 (attached as .config)
compiler: powerpc64le-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://github.com/0day-ci/linux/commit/c296b1e11db85710adc1831c4cdb2295192e447f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Fabio-M-De-Francesco/staging-rtl8188eu-Replace-a-custom-function-with-crc32_le/20210701-213922
        git checkout c296b1e11db85710adc1831c4cdb2295192e447f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/staging/rtl8188eu/core/rtw_security.c:597:58: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [usertype] @@     got unsigned int @@
   drivers/staging/rtl8188eu/core/rtw_security.c:597:58: sparse:     expected restricted __le32 [usertype]
   drivers/staging/rtl8188eu/core/rtw_security.c:597:58: sparse:     got unsigned int
   drivers/staging/rtl8188eu/core/rtw_security.c:604:58: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [usertype] @@     got unsigned int @@
   drivers/staging/rtl8188eu/core/rtw_security.c:604:58: sparse:     expected restricted __le32 [usertype]
   drivers/staging/rtl8188eu/core/rtw_security.c:604:58: sparse:     got unsigned int
   drivers/staging/rtl8188eu/core/rtw_security.c:671:42: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [usertype] @@     got unsigned int @@
   drivers/staging/rtl8188eu/core/rtw_security.c:671:42: sparse:     expected restricted __le32 [usertype]
   drivers/staging/rtl8188eu/core/rtw_security.c:671:42: sparse:     got unsigned int

vim +597 drivers/staging/rtl8188eu/core/rtw_security.c

   544	
   545	/* The hlen isn't include the IV */
   546	u32	rtw_tkip_encrypt(struct adapter *padapter, struct xmit_frame *pxmitframe)
   547	{																	/*  exclude ICV */
   548		u16	pnl;
   549		u32	pnh;
   550		u8	rc4key[16];
   551		u8   ttkey[16];
   552		u8	crc[4];
   553		u8   hw_hdr_offset = 0;
   554		struct arc4context mycontext;
   555		int			curfragnum, length;
   556	
   557		u8	*pframe, *payload, *iv, *prwskey;
   558		union pn48 dot11txpn;
   559		struct	sta_info		*stainfo;
   560		struct	pkt_attrib	 *pattrib = &pxmitframe->attrib;
   561		struct	security_priv	*psecuritypriv = &padapter->securitypriv;
   562		struct	xmit_priv		*pxmitpriv = &padapter->xmitpriv;
   563		u32	res = _SUCCESS;
   564	
   565		if (!pxmitframe->buf_addr)
   566			return _FAIL;
   567	
   568		hw_hdr_offset = TXDESC_SIZE +
   569			 (pxmitframe->pkt_offset * PACKET_OFFSET_SZ);
   570		pframe = pxmitframe->buf_addr + hw_hdr_offset;
   571		/* 4 start to encrypt each fragment */
   572		if (pattrib->encrypt == _TKIP_) {
   573			if (pattrib->psta)
   574				stainfo = pattrib->psta;
   575			else
   576				stainfo = rtw_get_stainfo(&padapter->stapriv, &pattrib->ra[0]);
   577	
   578			if (stainfo) {
   579				if (is_multicast_ether_addr(pattrib->ra))
   580					prwskey = psecuritypriv->dot118021XGrpKey[psecuritypriv->dot118021XGrpKeyid].skey;
   581				else
   582					prwskey = &stainfo->dot118021x_UncstKey.skey[0];
   583	
   584				for (curfragnum = 0; curfragnum < pattrib->nr_frags; curfragnum++) {
   585					iv = pframe + pattrib->hdrlen;
   586					payload = pframe + pattrib->iv_len + pattrib->hdrlen;
   587	
   588					GET_TKIP_PN(iv, dot11txpn);
   589	
   590					pnl = (u16)(dot11txpn.val);
   591					pnh = (u32)(dot11txpn.val >> 16);
   592					phase1((u16 *)&ttkey[0], prwskey, &pattrib->ta[0], pnh);
   593					phase2(&rc4key[0], prwskey, (u16 *)&ttkey[0], pnl);
   594	
   595					if ((curfragnum + 1) == pattrib->nr_frags) {	/* 4 the last fragment */
   596						length = pattrib->last_txcmdsz - pattrib->hdrlen - pattrib->iv_len - pattrib->icv_len;
 > 597						*((__le32 *)crc) = ~crc32_le(~0, payload, length);
   598	
   599						arcfour_init(&mycontext, rc4key, 16);
   600						arcfour_encrypt(&mycontext, payload, payload, length);
   601						arcfour_encrypt(&mycontext, payload + length, crc, 4);
   602					} else {
   603						length = pxmitpriv->frag_len - pattrib->hdrlen - pattrib->iv_len - pattrib->icv_len;
   604						*((__le32 *)crc) = ~crc32_le(~0, payload, length);
   605	
   606						arcfour_init(&mycontext, rc4key, 16);
   607						arcfour_encrypt(&mycontext, payload, payload, length);
   608						arcfour_encrypt(&mycontext, payload + length, crc, 4);
   609	
   610						pframe += pxmitpriv->frag_len;
   611						pframe = (u8 *)round_up((size_t)(pframe), 4);
   612					}
   613				}
   614			} else {
   615				res = _FAIL;
   616			}
   617		}
   618		return res;
   619	}
   620	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40008 bytes --]

Re: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5843 bytes --]

Hi "Fabio,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Fabio-M-De-Francesco/staging-rtl8188eu-Replace-a-custom-function-with-crc32_le/20210701-213922
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 77ad1f0e99bd00af024e650b862cfda3137af660
config: powerpc64-randconfig-s032-20210709 (attached as .config)
compiler: powerpc64le-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://github.com/0day-ci/linux/commit/c296b1e11db85710adc1831c4cdb2295192e447f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Fabio-M-De-Francesco/staging-rtl8188eu-Replace-a-custom-function-with-crc32_le/20210701-213922
        git checkout c296b1e11db85710adc1831c4cdb2295192e447f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/staging/rtl8188eu/core/rtw_security.c:597:58: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [usertype] @@     got unsigned int @@
   drivers/staging/rtl8188eu/core/rtw_security.c:597:58: sparse:     expected restricted __le32 [usertype]
   drivers/staging/rtl8188eu/core/rtw_security.c:597:58: sparse:     got unsigned int
   drivers/staging/rtl8188eu/core/rtw_security.c:604:58: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [usertype] @@     got unsigned int @@
   drivers/staging/rtl8188eu/core/rtw_security.c:604:58: sparse:     expected restricted __le32 [usertype]
   drivers/staging/rtl8188eu/core/rtw_security.c:604:58: sparse:     got unsigned int
   drivers/staging/rtl8188eu/core/rtw_security.c:671:42: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [usertype] @@     got unsigned int @@
   drivers/staging/rtl8188eu/core/rtw_security.c:671:42: sparse:     expected restricted __le32 [usertype]
   drivers/staging/rtl8188eu/core/rtw_security.c:671:42: sparse:     got unsigned int

vim +597 drivers/staging/rtl8188eu/core/rtw_security.c

   544	
   545	/* The hlen isn't include the IV */
   546	u32	rtw_tkip_encrypt(struct adapter *padapter, struct xmit_frame *pxmitframe)
   547	{																	/*  exclude ICV */
   548		u16	pnl;
   549		u32	pnh;
   550		u8	rc4key[16];
   551		u8   ttkey[16];
   552		u8	crc[4];
   553		u8   hw_hdr_offset = 0;
   554		struct arc4context mycontext;
   555		int			curfragnum, length;
   556	
   557		u8	*pframe, *payload, *iv, *prwskey;
   558		union pn48 dot11txpn;
   559		struct	sta_info		*stainfo;
   560		struct	pkt_attrib	 *pattrib = &pxmitframe->attrib;
   561		struct	security_priv	*psecuritypriv = &padapter->securitypriv;
   562		struct	xmit_priv		*pxmitpriv = &padapter->xmitpriv;
   563		u32	res = _SUCCESS;
   564	
   565		if (!pxmitframe->buf_addr)
   566			return _FAIL;
   567	
   568		hw_hdr_offset = TXDESC_SIZE +
   569			 (pxmitframe->pkt_offset * PACKET_OFFSET_SZ);
   570		pframe = pxmitframe->buf_addr + hw_hdr_offset;
   571		/* 4 start to encrypt each fragment */
   572		if (pattrib->encrypt == _TKIP_) {
   573			if (pattrib->psta)
   574				stainfo = pattrib->psta;
   575			else
   576				stainfo = rtw_get_stainfo(&padapter->stapriv, &pattrib->ra[0]);
   577	
   578			if (stainfo) {
   579				if (is_multicast_ether_addr(pattrib->ra))
   580					prwskey = psecuritypriv->dot118021XGrpKey[psecuritypriv->dot118021XGrpKeyid].skey;
   581				else
   582					prwskey = &stainfo->dot118021x_UncstKey.skey[0];
   583	
   584				for (curfragnum = 0; curfragnum < pattrib->nr_frags; curfragnum++) {
   585					iv = pframe + pattrib->hdrlen;
   586					payload = pframe + pattrib->iv_len + pattrib->hdrlen;
   587	
   588					GET_TKIP_PN(iv, dot11txpn);
   589	
   590					pnl = (u16)(dot11txpn.val);
   591					pnh = (u32)(dot11txpn.val >> 16);
   592					phase1((u16 *)&ttkey[0], prwskey, &pattrib->ta[0], pnh);
   593					phase2(&rc4key[0], prwskey, (u16 *)&ttkey[0], pnl);
   594	
   595					if ((curfragnum + 1) == pattrib->nr_frags) {	/* 4 the last fragment */
   596						length = pattrib->last_txcmdsz - pattrib->hdrlen - pattrib->iv_len - pattrib->icv_len;
 > 597						*((__le32 *)crc) = ~crc32_le(~0, payload, length);
   598	
   599						arcfour_init(&mycontext, rc4key, 16);
   600						arcfour_encrypt(&mycontext, payload, payload, length);
   601						arcfour_encrypt(&mycontext, payload + length, crc, 4);
   602					} else {
   603						length = pxmitpriv->frag_len - pattrib->hdrlen - pattrib->iv_len - pattrib->icv_len;
   604						*((__le32 *)crc) = ~crc32_le(~0, payload, length);
   605	
   606						arcfour_init(&mycontext, rc4key, 16);
   607						arcfour_encrypt(&mycontext, payload, payload, length);
   608						arcfour_encrypt(&mycontext, payload + length, crc, 4);
   609	
   610						pframe += pxmitpriv->frag_len;
   611						pframe = (u8 *)round_up((size_t)(pframe), 4);
   612					}
   613				}
   614			} else {
   615				res = _FAIL;
   616			}
   617		}
   618		return res;
   619	}
   620	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 40008 bytes --]

Re: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[Fabio M. De Francesco]

On Saturday, July 10, 2021 2:25:12 AM CEST kernel test robot wrote:
> Hi "Fabio,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on staging/staging-testing]
> 
> url:   
> https://github.com/0day-ci/linux/commits/Fabio-M-De-Francesco/staging-rtl8188eu-Replace
> -a-custom-function-with-crc32_le/20210701-213922 base:  
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> 77ad1f0e99bd00af024e650b862cfda3137af660 config: powerpc64-randconfig-
s032-20210709

[CUT]

> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> drivers/staging/rtl8188eu/core/rtw_security.c:597:58: sparse: sparse: 
incorrect type
> >> in assignment (different base types) @@     expected restricted __le32 
[usertype] @@
> >>     got unsigned int @@
>    drivers/staging/rtl8188eu/core/rtw_security.c:597:58: sparse:     
expected restricted
> __le32 [usertype] drivers/staging/rtl8188eu/core/rtw_security.c:597:58: 
sparse:     got
> unsigned int drivers/staging/rtl8188eu/core/rtw_security.c:604:58: sparse: 
sparse:
> incorrect type in assignment (different base types) @@     expected 
restricted __le32
> [usertype] @@     got unsigned int @@
> drivers/staging/rtl8188eu/core/rtw_security.c:604:58: sparse:     expected 
restricted
> __le32 [usertype] drivers/staging/rtl8188eu/core/rtw_security.c:604:58: 
sparse:     got
> unsigned int drivers/staging/rtl8188eu/core/rtw_security.c:671:42: sparse: 
sparse:
> incorrect type in assignment (different base types) @@     expected 
restricted __le32
> [usertype] @@     got unsigned int @@
> drivers/staging/rtl8188eu/core/rtw_security.c:671:42: sparse:     expected 
restricted
> __le32 [usertype] drivers/staging/rtl8188eu/core/rtw_security.c:671:42: 
sparse:     got
> unsigned int

[CUT]

I suppose that these warnings are false positives for the reasons explained in 
my patch.

Furthermore, I found a commit by Fabio Aiuto <fabioaiuto83@gmail.com> that, as 
far as I understand, do the same changes and that has been accepted by Greg K-
H and merged in his tree: b97fad10de387c09ae46f607955c7237afa96654. So, I 
think there one reason more to suspect of false positives.

I'd appreciate comments on this topic.

Thanks,

Fabio

Re: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[Fabio M. De Francesco]

[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]

On Saturday, July 10, 2021 2:25:12 AM CEST kernel test robot wrote:
> Hi "Fabio,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on staging/staging-testing]
> 
> url:   
> https://github.com/0day-ci/linux/commits/Fabio-M-De-Francesco/staging-rtl8188eu-Replace
> -a-custom-function-with-crc32_le/20210701-213922 base:  
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> 77ad1f0e99bd00af024e650b862cfda3137af660 config: powerpc64-randconfig-
s032-20210709

[CUT]

> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> drivers/staging/rtl8188eu/core/rtw_security.c:597:58: sparse: sparse: 
incorrect type
> >> in assignment (different base types) @@     expected restricted __le32 
[usertype] @@
> >>     got unsigned int @@
>    drivers/staging/rtl8188eu/core/rtw_security.c:597:58: sparse:     
expected restricted
> __le32 [usertype] drivers/staging/rtl8188eu/core/rtw_security.c:597:58: 
sparse:     got
> unsigned int drivers/staging/rtl8188eu/core/rtw_security.c:604:58: sparse: 
sparse:
> incorrect type in assignment (different base types) @@     expected 
restricted __le32
> [usertype] @@     got unsigned int @@
> drivers/staging/rtl8188eu/core/rtw_security.c:604:58: sparse:     expected 
restricted
> __le32 [usertype] drivers/staging/rtl8188eu/core/rtw_security.c:604:58: 
sparse:     got
> unsigned int drivers/staging/rtl8188eu/core/rtw_security.c:671:42: sparse: 
sparse:
> incorrect type in assignment (different base types) @@     expected 
restricted __le32
> [usertype] @@     got unsigned int @@
> drivers/staging/rtl8188eu/core/rtw_security.c:671:42: sparse:     expected 
restricted
> __le32 [usertype] drivers/staging/rtl8188eu/core/rtw_security.c:671:42: 
sparse:     got
> unsigned int

[CUT]

I suppose that these warnings are false positives for the reasons explained in 
my patch.

Furthermore, I found a commit by Fabio Aiuto <fabioaiuto83@gmail.com> that, as 
far as I understand, do the same changes and that has been accepted by Greg K-
H and merged in his tree: b97fad10de387c09ae46f607955c7237afa96654. So, I 
think there one reason more to suspect of false positives.

I'd appreciate comments on this topic.

Thanks,

Fabio

RE: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[David Laight]

From: Fabio M. De Francesco
> Sent: 10 July 2021 15:39
...
> [CUT]
[PASTE}
> 597						*((__le32 *)crc) = ~crc32_le(~0, payload, length);
> 
> I suppose that these warnings are false positives for the reasons explained in
> my patch.

No they are an indication you aren't doing things 'right'.
You shouldn't need an __le32 cast in this code.
'crc' should be defined as __le32 (not u8[4]) and IIRC
get_unaligned_le32() used in the latter check.

Actually what is this code actually doing.
ISTR it is doing a crc32() and then comparing the result with the
crc in the buffer?
No hardware ever does that.
What happens is the receiver does the crc of the whole buffer
including the transmitted crc and then checks the value is the
required 'magic' constant'.
This all works because the crc is inverted before transmission.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[David Laight]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

From: Fabio M. De Francesco
> Sent: 10 July 2021 15:39
...
> [CUT]
[PASTE}
> 597						*((__le32 *)crc) = ~crc32_le(~0, payload, length);
> 
> I suppose that these warnings are false positives for the reasons explained in
> my patch.

No they are an indication you aren't doing things 'right'.
You shouldn't need an __le32 cast in this code.
'crc' should be defined as __le32 (not u8[4]) and IIRC
get_unaligned_le32() used in the latter check.

Actually what is this code actually doing.
ISTR it is doing a crc32() and then comparing the result with the
crc in the buffer?
No hardware ever does that.
What happens is the receiver does the crc of the whole buffer
including the transmitted crc and then checks the value is the
required 'magic' constant'.
This all works because the crc is inverted before transmission.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[Greg Kroah-Hartman]

On Thu, Jul 01, 2021 at 03:38:09PM +0200, Fabio M. De Francesco wrote:
> Use crc32_le in place of the custom getcrc32. This change makes GCC
> to warn about incorrect castings to the restricted type __le32, but
> they can be safely ignored because crc32_le calculates bitwise
> little-endian Ethernet AUTODIN II CRC32.
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/rtl8188eu/core/rtw_security.c | 22 ++++---------------
>  1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c b/drivers/staging/rtl8188eu/core/rtw_security.c
> index 1b2cb6196463..5f010cb66970 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_security.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_security.c
> @@ -111,21 +111,6 @@ static void crc32_init(void)
>  	bcrc32initialized = 1;
>  }
>  
> -static __le32 getcrc32(u8 *buf, int len)
> -{
> -	u8 *p;
> -	u32  crc;
> -
> -	if (bcrc32initialized == 0)
> -		crc32_init();
> -
> -	crc = 0xffffffff;       /* preload shift register, per CRC-32 spec */
> -
> -	for (p = buf; len > 0; ++p, --len)
> -		crc = crc32_table[(crc ^ *p) & 0xff] ^ (crc >> 8);
> -	return cpu_to_le32(~crc);    /* transmit complement, per CRC-32 spec */
> -}
> -
>  /* Need to consider the fragment  situation */
>  void rtw_wep_encrypt(struct adapter *padapter, struct xmit_frame *pxmitframe)
>  {
> @@ -609,14 +594,15 @@ u32	rtw_tkip_encrypt(struct adapter *padapter, struct xmit_frame *pxmitframe)
>  
>  				if ((curfragnum + 1) == pattrib->nr_frags) {	/* 4 the last fragment */
>  					length = pattrib->last_txcmdsz - pattrib->hdrlen - pattrib->iv_len - pattrib->icv_len;
> -					*((__le32 *)crc) = getcrc32(payload, length);/* modified by Amy*/
> +					*((__le32 *)crc) = ~crc32_le(~0, payload, length);

Why are you casting a native endian return value to a little endian
pointer?  Are you _SURE_ that is correct?

We can not just ignore warnings, they are there for a reason.  Or if
not, then fix the code up to not have the warnings, but I can't take
this as-is, sorry.

thanks,

greg k-h

Re: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[Fabio M. De Francesco]

On Tuesday, July 13, 2021 2:50:47 PM CEST Greg Kroah-Hartman wrote:
> On Thu, Jul 01, 2021 at 03:38:09PM +0200, Fabio M. De Francesco wrote:
> > Use crc32_le in place of the custom getcrc32. This change makes GCC
> > to warn about incorrect castings to the restricted type __le32, but
> > they can be safely ignored because crc32_le calculates bitwise
> > little-endian Ethernet AUTODIN II CRC32.
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> >  drivers/staging/rtl8188eu/core/rtw_security.c | 22 ++++---------------
> >  1 file changed, 4 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c
> > b/drivers/staging/rtl8188eu/core/rtw_security.c index 
1b2cb6196463..5f010cb66970
> > 100644
> > --- a/drivers/staging/rtl8188eu/core/rtw_security.c
> > +++ b/drivers/staging/rtl8188eu/core/rtw_security.c
> > @@ -111,21 +111,6 @@ static void crc32_init(void)
> > 
[...]
> 
> Why are you casting a native endian return value to a little endian
> pointer?  
>
Actually, I was not sure whether or not crc32_le() returns native or little 
endian values: 

(1) That "_le" in crc32_le() made me think it calculates little endian values 
and the documentation of the function is a little misleading where it affirms 
that "[crc32_le()] calculates *bitwise
little-endian* Ethernet AUTODIN II CRC32".

(2) Furthermore, I took commit b97fad10de387 as a model for my patch. You 
applied that commit and it makes exactly the same change that I make in my 
patch. I still cannot understand why you took that. Maybe I'm missing 
something that I can't still see. What is it?
>
> Are you _SURE_ that is correct?
> 
To summarize, I thought that crc32_le is some way returning an LE value in an 
array of four u8 (aka an unsigned integer).

Now I suppose I have to change "*((__le32 *)crc) = ~crc32_le(~0, payload, 
length);" to "*((__le32 *)crc) = cpu_to_le32(~crc32_le(~0, payload, length)".

Thanks,

Fabio
> We can not just ignore warnings, they are there for a reason.  Or if
> not, then fix the code up to not have the warnings, but I can't take
> this as-is, sorry.
> 
> thanks,
> 
> greg k-h