[PATCH v3 0/2] staging: rtl8188eu: Replace a custom function with crc32_le()

[PATCH v3 0/2] staging: rtl8188eu: Replace a custom function with crc32_le()

[Fabio M. De Francesco]

Use kernel API crc32_le() in place of the custom getcrc32(). Remove no
more used functions and variables after the changes made by patch 1/2. 

v2 --> v3:
Join two related patches in a series because they must be applied in
order (first 1/2, then 2/2). Rebase to the current Greg K-H's tree and resend.

v1 --> v2:
Use cpu_to_le32() to assign native endian return value of crc32_le to
the crc variable in little endian byte order.

Fabio M. De Francesco (2):
  staging: rtl8188eu: Replace a custom function with crc32_le()
  staging: rtl8188eu: Remove no more used functions and variables

 drivers/staging/rtl8188eu/core/rtw_security.c | 59 ++-----------------
 1 file changed, 5 insertions(+), 54 deletions(-)

-- 
2.32.0

[PATCH v3 1/2] staging: rtl8188eu: Replace a custom function with crc32_le()

[Fabio M. De Francesco]

Use crc32_le() in place of the custom getcrc32().

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---

v2 --> v3:
Join two related patches in a series because they must be applied in 
order (first 1/2, then 2/2). Rebase to the current Greg K-H's tree and resend.

v1 --> v2:
Use cpu_to_le32() to assign native endian return value of crc32_le to
crc in little endian byte order.

 drivers/staging/rtl8188eu/core/rtw_security.c | 23 ++++---------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c b/drivers/staging/rtl8188eu/core/rtw_security.c
index 1b2cb6196463..e9de61afe881 100644
--- a/drivers/staging/rtl8188eu/core/rtw_security.c
+++ b/drivers/staging/rtl8188eu/core/rtw_security.c
@@ -11,6 +11,7 @@
 #include <wifi.h>
 #include <osdep_intf.h>
 #include <net/lib80211.h>
+#include <linux/types.h>
 
 /* WEP related ===== */
 
@@ -111,21 +112,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 +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);
@@ -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] ||
-- 
2.32.0

RE: [PATCH v3 1/2] staging: rtl8188eu: Replace a custom function with crc32_le()

[David Laight]

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.

> @@ -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).

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

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)

Re: [PATCH v3 1/2] staging: rtl8188eu: Replace a custom function with crc32_le()

[Fabio M. De Francesco]

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)

RE: [PATCH v3 1/2] staging: rtl8188eu: Replace a custom function with crc32_le()

[David Laight]

From: Fabio M. De Francesco
> Sent: 23 July 2021 18:41
> 
> 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...

Your mailer mail a right pigs breakfast of this quoted text.
Even outlook (which work makes me use) isn't that bad
- it is pretty shitty though.

...
> > 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?

Try a cast :-)
arcfour_encrypt() takes a 'buffer' - so it should be 'const void *'.
That will let the call pass what they want.

...
> > > @@ -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?

Because they aren't needed.
Think about what happens when the CRC is processed bit by bit with
a hardware shift register.
When 'length - 4' bytes have been processed the receivers CRC register
matches that the transmitter had at the same point.
The transmitter inverts each crc bit before sending it.
So when the receiver XOR's a received bit with a CRC bit (to feed back
into the shift register) it always gets a '1' bits
(either the crc bit or the data is a 1).
Once the receiver has processed 'length' bytes the CRC register always
contains the crc of four 0xff bytes - regardless of the input frame.

If you don't invert the crc before sending (some early frame formats
didn't) then the last data bits and crc bits cancel each other out
and the crc is always 0 for a good frame.

It is also worth realising that a crc is a linear function.
This means that if you 'exclusive or' two frames with valid CRC
the resultant buffer also has a valid crc (align the ends of
the frames).
Initialising the crc to ~0 has the effect of including the frame
length in the crc - particularly useful if the frame starts with
zero bytes.

Since the crc algorithm is reversable, if you get a CRC error
you can 'wind back' the error bits until only a small number
of bits are affected - flip the matching bits of the receive
data and so correct a single short error burst (the most
likely errors) to get the original data.

	David

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

[PATCH v3 2/2] staging: rtl8188eu: Remove no more used functions and variables

[Fabio M. De Francesco]

Remove the functions and variables from rtw_security.c that are no more
necessary since the patch (1/2) that replaces getcrc32() with
crc32_le().

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---

v2 --> v3:
Join two related patches in a series because they must be applied in 
order (first 1/2, then 2/2). Rebase to the current Greg K-H's tree and resend.

v1 -> v2:
Update the commit message with the URL of v2 of the above-mentioned
patch.

 drivers/staging/rtl8188eu/core/rtw_security.c | 36 -------------------
 1 file changed, 36 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c b/drivers/staging/rtl8188eu/core/rtw_security.c
index e9de61afe881..8ec0b897ba3b 100644
--- a/drivers/staging/rtl8188eu/core/rtw_security.c
+++ b/drivers/staging/rtl8188eu/core/rtw_security.c
@@ -76,42 +76,6 @@ static void arcfour_encrypt(struct arc4context *parc4ctx, u8 *dest, u8 *src, u32
 		dest[i] = src[i] ^ (unsigned char)arcfour_byte(parc4ctx);
 }
 
-static int bcrc32initialized;
-static u32 crc32_table[256];
-
-static u8 crc32_reverseBit(u8 data)
-{
-	return (u8)((data << 7) & 0x80) | ((data << 5) & 0x40) | ((data << 3) & 0x20) |
-		   ((data << 1) & 0x10) | ((data >> 1) & 0x08) | ((data >> 3) & 0x04) |
-		   ((data >> 5) & 0x02) | ((data >> 7) & 0x01);
-}
-
-static void crc32_init(void)
-{
-	int i, j;
-	u32 c;
-	u8 *p = (u8 *)&c, *p1;
-	u8 k;
-
-	if (bcrc32initialized == 1)
-		return;
-
-	c = 0x12340000;
-
-	for (i = 0; i < 256; ++i) {
-		k = crc32_reverseBit((u8)i);
-		for (c = ((u32)k) << 24, j = 8; j > 0; --j)
-			c = c & 0x80000000 ? (c << 1) ^ CRC32_POLY : (c << 1);
-		p1 = (u8 *)&crc32_table[i];
-
-		p1[0] = crc32_reverseBit(p[3]);
-		p1[1] = crc32_reverseBit(p[2]);
-		p1[2] = crc32_reverseBit(p[1]);
-		p1[3] = crc32_reverseBit(p[0]);
-	}
-	bcrc32initialized = 1;
-}
-
 /* Need to consider the fragment  situation */
 void rtw_wep_encrypt(struct adapter *padapter, struct xmit_frame *pxmitframe)
 {
-- 
2.32.0