linux-kernel - Re: [PATCH v3 1/2] staging: rtl8188eu: Replace a custom function with crc32

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: <3650881.QlJdx9khu8@localhost.localdomain>
Date:   Fri, 23 Jul 2021 19:40:35 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Larry Finger <Larry.Finger@...inger.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-staging@...ts.linux.dev" <linux-staging@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        David Laight <David.Laight@...lab.com>
Subject: Re: [PATCH v3 1/2] staging: rtl8188eu: Replace a custom function with crc32_le()

Hi David,

This driver is going to be replaced by a "better" version, so I'm not sure 
whether or not this patch is still needed.

However, I see that we have similar problems in rtl8723bs and perhaps also in 
other drivers. Therefore, I'd like to solve this problem, whatever will happen 
to the "worse" rti8188eu, and change the code where else it needs to be 
changed.

Now I have a few questions... 

On Thursday, July 22, 2021 5:30:08 PM CEST David Laight wrote:
> From: Fabio M. De Francesco
> 
> > Sent: 21 July 2021 12:01
> > 
> > Use crc32_le() in place of the custom getcrc32().
> 
> ...
> 
> > @@ -609,14 +595,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) = 
cpu_to_le32(~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) = 
cpu_to_le32(~crc32_le(~0, payload, length));
> > +
> > 
> >  					
arcfour_init(&mycontext, rc4key, 16);
> >  					
arcfour_encrypt(&mycontext, payload, payload, length);
> >  					
arcfour_encrypt(&mycontext, payload + length, crc, 4);
> 
> Change crc to be __le32, kill the casts and pass &crc in the last call.
> 

I could do it, but the last call (that to arcfour_encrypt() takes a pointer to 
u8 type as the third parameter. How can I use a __le32 for that?

I think that one possible solution is to change crc to be an union and use two 
fields in the following lines:

union {
	__le32 f0; /* More descriptive name? */
	u8 f1[4]; /* More descriptive name? */
} crc;

[...]

crc.f0 = cpu_to_le32(~crc32_le(~0, payload, length - 4));

[...]

arcfour_encrypt(&mycontext, payload + length, crc.f1, 4);

Please, tell me... What about the solution above?

> > @@ -682,7 +669,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) = cpu_to_le32(~crc32_le(~0, 
payload, length - 4));
> > 
> >  			if (crc[3] != payload[length - 1] ||
> >  			
> >  			    crc[2] != payload[length - 2] ||
> 
> You could to the same here, or make crc u32, remove the cpu_to_le32()
> and use get_unaligned_u32(payload + length - 4) (or whatever it is called).
> 

Sorry, I can't understand this line. Can you please elaborate it a bit more?

> But it is much better to do:
> 	crc = crc32_le(~0, payload, length);
> 	if (crc != VALID_CRC32)
> 		res = _FAIL;
>

Why "crc = crc32_le(~0, payload, length);"? Shouldn't it be "crc = 
cpu_to_le32(~crc32_le(~0, patload, length);"? 

Why did you drop both the cpu_to_le32() call and the '~' operator?

Thanks in advance,

Fabio

> You can lookup VALID_CRC32, but it is crc32_le(0, "\xff\xff\xff\xff", 4);
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 
1PT, UK
> Registration No: 1397386 (Wales)