staging: rtl8188eu: Replace a custom function with crc32_le()

PATCH Superseded Fabio M. De Francesco 2 participants 6 messages Jul 21, 2021 – Jul 24, 2021 Staging

Share & tools

Fabio M. De Francesco

author

2021-07-21 11:00 UTC #0 lore raw

v3 cover staging: rtl8188eu: Replace a custom function with crc32_le()

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(-)

Signature · 1 line

2.32.0

Fabio M. De Francesco

author

2021-07-21 11:01 UTC #1 lore raw

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

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

Signed-off-by Fabio M. De Francesco <[email protected]>

---

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(-)

drivers/staging/rtl8188eu/core/rtw_security.c +5 ~~-18~~

index 1b2cb6196463..e9de61afe881 100644
@@ -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] ||

Signature · 1 line

2.32.0

David Laight

2021-07-22 15:30 UTC #2 lore raw

From: Fabio M. De Francesco

Quoted 4 lines

Sent: 21 July 2021 12:01
Use crc32_le() in place of the custom getcrc32().

...

Quoted 18 lines

drivers/staging/rtl8188eu/core/rtw_security.c +3 -2

@@ -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));
+

drivers/staging/rtl8188eu/core/rtw_security.c u32 rtw_tkip_encrypt(struct adapter *padapter, struct xmit_frame *pxmitframe) · line 602

Quoted 3 lines

drivers/staging/rtl8188eu/core/rtw_security.c +0 -0

 					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.

drivers/staging/rtl8188eu/core/rtw_security.c u32 rtw_tkip_decrypt(struct adapter *padapter, struct recv_frame *precvframe) · line 675

Quoted 9 lines

drivers/staging/rtl8188eu/core/rtw_security.c +1 -1

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

Fabio M. De Francesco

author

2021-07-23 17:40 UTC #3 lore raw

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

Quoted 9 lines · CEST David Laight

From: Fabio M. De Francesco
Quoted 3 lines
Sent: 21 July 2021 12:01
Use crc32_le() in place of the custom getcrc32().
...
Review u32 rtw_tkip_encrypt(struct adapter *padapter,
Quoted 1 line
(quoted diff) +0 -0
@@ -609,14 +595,15 @@ u32	rtw_tkip_encrypt(struct adapter *padapter, 

struct xmit_frame

Quoted 10 lines

Review u32 rtw_tkip_encrypt(struct adapter *padapter, · line 597
Quoted 3 lines
(quoted diff) +0 -0
 *pxmitframe)
 
 				if ((curfragnum + 1) == pattrib-
nr_frags) { /* 4 the last fragment */
Review u32 rtw_tkip_encrypt(struct adapter *padapter, · line 598
Quoted 2 lines
(quoted diff) +0 -0
 					length = pattrib-
last_txcmdsz - pattrib->hdrlen - pattrib->iv_len -
Review u32 rtw_tkip_encrypt(struct adapter *padapter, · line 614
Quoted 3 lines
(quoted diff) +0 -1
 pattrib->icv_len;
-					*((__le32 *)crc) = 

getcrc32(payload, length);/* modified by Amy*/

Quoted 1 line

Review u32 rtw_tkip_encrypt(struct adapter *padapter, · line 600
Quoted 1 line
(quoted diff) +1 -0
+					*((__le32 *)crc) = 

cpu_to_le32(~crc32_le(~0, payload, length));

Quoted 2 lines

Quoted 2 lines

arcfour_init(&mycontext, rc4key, 16);

Quoted 1 line

Quoted 1 line

arcfour_encrypt(&mycontext, payload, payload, length);

Quoted 1 line

Quoted 1 line

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

Quoted 8 lines

Review
Quoted 4 lines
(quoted diff) +0 -0
 				} else {
 
 					length = pxmitpriv-
frag_len - pattrib->hdrlen - pattrib->iv_len -
Review
Quoted 3 lines
pattrib->icv_len;
(quoted diff) +0 -1
-					*((__le32 *)crc) = 

getcrc32(payload, length);/* modified by Amy*/

Quoted 1 line

Review
Quoted 1 line
(quoted diff) +1 -0
+					*((__le32 *)crc) = 

cpu_to_le32(~crc32_le(~0, payload, length));

Quoted 3 lines

Quoted 3 lines
+

arcfour_init(&mycontext, rc4key, 16);

Quoted 1 line

Quoted 1 line

arcfour_encrypt(&mycontext, payload, payload, length);

Quoted 1 line

Quoted 1 line

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

Quoted 3 lines

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?

Quoted 1 line

Review u32 rtw_tkip_decrypt(struct adapter *padapter, struct
Quoted 1 line
(quoted diff) +0 -0
@@ -682,7 +669,7 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, struct 

recv_frame

Quoted 3 lines

Review u32 rtw_tkip_decrypt(struct adapter *padapter, struct · line 671
Quoted 3 lines
(quoted diff) +0 -0
 *precvframe)> 
 			arcfour_init(&mycontext, rc4key, 16);
 			arcfour_encrypt(&mycontext, payload, 

payload, length);

Quoted 2 lines

Review u32 rtw_tkip_decrypt(struct adapter *padapter, struct · line 685
Quoted 2 lines
(quoted diff) +0 -1
-			*((__le32 *)crc) = getcrc32(payload, length 

- 4);

Quoted 1 line

Review u32 rtw_tkip_decrypt(struct adapter *padapter, struct · line 672
Quoted 1 line
(quoted diff) +1 -0
+			*((__le32 *)crc) = cpu_to_le32(~crc32_le(~0, 

payload, length - 4));

Quoted 8 lines

drivers/staging/rtl8188eu/core/rtw_security.c u32 rtw_tkip_decrypt(struct adapter *padapter, struct recv_frame *precvframe) · line 675
Quoted 4 lines
drivers/staging/rtl8188eu/core/rtw_security.c +0 -0
 			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?

Quoted 5 lines

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

Quoted 6 lines

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

Quoted 1 line

Registration No: 1397386 (Wales)

David Laight

2021-07-24 11:04 UTC #4 lore raw

From: Fabio M. De Francesco

Quoted 7 lines

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.

Quoted 6 lines

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.

...

Quoted 5 lines

Quoted 2 lines
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.

...

Quoted 28 lines

Quoted 1 line
Review u32 rtw_tkip_decrypt(struct adapter *padapter, struct
Quoted 1 line
(quoted diff) +0 -0
@@ -682,7 +669,7 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, struct
recv_frame
Quoted 3 lines
Review u32 rtw_tkip_decrypt(struct adapter *padapter, struct · line 671
Quoted 3 lines
(quoted diff) +0 -0
 *precvframe)>
 			arcfour_init(&mycontext, rc4key, 16);
 			arcfour_encrypt(&mycontext, payload,
payload, length);
Quoted 2 lines
Review u32 rtw_tkip_decrypt(struct adapter *padapter, struct · line 685
Quoted 2 lines
(quoted diff) +0 -1
-			*((__le32 *)crc) = getcrc32(payload, length
- 4);
Quoted 1 line
Review u32 rtw_tkip_decrypt(struct adapter *padapter, struct · line 672
Quoted 1 line
(quoted diff) +1 -0
+			*((__le32 *)crc) = cpu_to_le32(~crc32_le(~0,
payload, length - 4));
Quoted 8 lines
drivers/staging/rtl8188eu/core/rtw_security.c u32 rtw_tkip_decrypt(struct adapter *padapter, struct recv_frame *precvframe) · line 675
Quoted 4 lines
drivers/staging/rtl8188eu/core/rtw_security.c +0 -0
 			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?
Quoted 5 lines
But it is much better to do:
	crc = crc32_le(~0, payload, length);
	if (crc != VALID_CRC32)
		res = _FAIL;

Quoted 4 lines

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)

Fabio M. De Francesco

author

2021-07-21 11:01 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---

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(-)

drivers/staging/rtl8188eu/core/rtw_security.c +0 ~~-36~~

index e9de61afe881..8ec0b897ba3b 100644
@@ -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)
 {

Signature · 1 line

2.32.0