rtw_security: fix cast to restricted __le32

This patch fixes the sparse warning of fix cast to restricted __le32.

Last month, there was a change for replacing private CRC-32 routines with in-kernel ones.
In that patch, we replaced getcrc32 with crc32_le in calling le32_to_cpu. le32_to_cpu accepts __le32 type as arg, but crc32_le returns unsigned int. That how it introduced the sparse warning.

Quoted 7 lines · Jhih-Ming Huang

This patch fixes the sparse warning of fix cast to restricted __le32.

Last month, there was a change for replacing private CRC-32 routines with in-kernel ones.
In that patch, we replaced getcrc32 with crc32_le in calling le32_to_cpu. le32_to_cpu accepts __le32 type as arg, but crc32_le returns unsigned int. That how it introduced the sparse warning.

As crc32_le returns a u32 which is in native-endian format, how can you cast it to le32? Why do you cast it to le32? Isn't that going to be incorrect for big endian systems?

thanks,

greg k-h

Quoted 16 lines · Greg KH

Quoted 7 lines · Jhih-Ming Huang

This patch fixes the sparse warning of fix cast to restricted __le32.

Last month, there was a change for replacing private CRC-32 routines with in-kernel ones.
In that patch, we replaced getcrc32 with crc32_le in calling le32_to_cpu. le32_to_cpu accepts __le32 type as arg, but crc32_le returns unsigned int. That how it introduced the sparse warning.

As crc32_le returns a u32 which is in native-endian format, how can you cast it to le32? Why do you cast it to le32? Isn't that going to be incorrect for big endian systems?

thanks,

Quoted 1 line

greg k-h

Thanks for the fast reply.
Yes, you are right. I did not notice that le32_to_cpu already handles both of the cases.

So it seems the warning from sparse is false positives, am I right?

Quoted 17 lines · Jhih Ming Huang

Quoted 16 lines · Greg KH

Quoted 7 lines · Jhih-Ming Huang

This patch fixes the sparse warning of fix cast to restricted __le32.

Last month, there was a change for replacing private CRC-32 routines with in-kernel ones.
In that patch, we replaced getcrc32 with crc32_le in calling le32_to_cpu. le32_to_cpu accepts __le32 type as arg, but crc32_le returns unsigned int. That how it introduced the sparse warning.

As crc32_le returns a u32 which is in native-endian format, how can you cast it to le32? Why do you cast it to le32? Isn't that going to be incorrect for big endian systems?

thanks,

Quoted 7 lines

Quoted 1 line

greg k-h

Thanks for the fast reply.
Yes, you are right. I did not notice that le32_to_cpu already handles both of the cases.

So it seems the warning from sparse is false positives, am I right?

In a sense that on all architectures we would be ever likely to support le32_to_cpu and cpu_to_le32 do the same bit-shuffling - yes. In a sense of having those used correctly it's not a false positive, though - it's much easier to follow "this variable always hold native-endian, this - little-endian" and watch for conversions done correctly than to count the byteswaps and try to prove that it's either even for all execution histories or odd for all execution histories.

IOW, there's a good reason for keeping separate cpu_to_le32 and le32_to_cpu and not mixing them with each other - it's easier to prove correctness that way *and* easier to look for endianness bugs.

Thanks for your explanation.

To clarify, even though it might be false positives in some senses, following "hold the variable native-endian and check the conversion done correctly"
is much easier than the other way. And it's exactly the current implementation.

So it's better to keep the current implementation and ignore the warnings, right?

This patch fixes the sparse warning of fix cast to restricted __le32.

There was a change for replacing private CRC-32 routines with in kernel ones.
However, the author used le32_to_cpu to convert crc32_le(), and we should cpu_to_le32.

Ths commit also fixes the payload checking by memcmp instead of checking element by element.

Signed-off-by Jhih-Ming Huang <[email protected]>

---
 drivers/staging/rtl8723bs/core/rtw_security.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

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

index a99f439328f1..97a7485f8f58 100644

This patch fixes the sparse warning of fix cast to restricted __le32.

There was a change for replacing private CRC-32 routines with in kernel ones.
However, the author used le32_to_cpu to convert crc32_le(), and we should cpu_to_le32.

Ths commit also fixes the payload checking by memcmp instead of checking element by element and removes the unused variable.

Signed-off-by Jhih-Ming Huang <[email protected]>

---
 drivers/staging/rtl8723bs/core/rtw_security.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

drivers/staging/rtl8723bs/core/rtw_security.c +3 -9

index a99f439328f1..8dc6a976b487 100644
@@ -92,7 +92,6 @@ void rtw_wep_encrypt(struct adapter *padapter, u8 *pxmitframe)
 void rtw_wep_decrypt(struct adapter  *padapter, u8 *precvframe)
 {
 	/*  exclude ICV */
-	u8 crc[4];
 	signed int	length;
 	u32 keylength;
 	u8 *pframe, *payload, *iv, wepkey[16];

@@ -119,10 +118,6 @@ void rtw_wep_decrypt(struct adapter  *padapter, u8 *precvframe)
 		/* decrypt payload include icv */
 		arc4_setkey(ctx, wepkey, 3 + keylength);
 		arc4_crypt(ctx, payload, payload,  length);
-
-		/* calculate icv and compare the icv */
-		*((u32 *)crc) = le32_to_cpu(~crc32_le(~0, payload, length - 4));
-
 	}
 }
 

@@ -537,7 +532,7 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, u8 *precvframe)
 	u32 pnh;
 	u8   rc4key[16];
 	u8   ttkey[16];
-	u8 crc[4];
+	__le32 crc;
 	signed int			length;
 
 	u8 *pframe, *payload, *iv, *prwskey;

@@ -618,10 +613,9 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, u8 *precvframe)
 			arc4_setkey(ctx, rc4key, 16);
 			arc4_crypt(ctx, payload, payload, length);
 
-			*((u32 *)crc) = le32_to_cpu(~crc32_le(~0, payload, length - 4));
+			crc = cpu_to_le32(~crc32_le(~0, payload, length - 4));
 
-			if (crc[3] != payload[length - 1] || crc[2] != payload[length - 2] ||
-			    crc[1] != payload[length - 3] || crc[0] != payload[length - 4])
+			if (memcmp(&crc, payload + length - 4, 4) != 0)
 				res = _FAIL;
 		} else {
 			res = _FAIL;

Signature · 1 line

2.32.0

Sent: 21 June 2021 09:19

This patch fixes the sparse warning of fix cast to restricted __le32.

There was a change for replacing private CRC-32 routines with in kernel ones.
However, the author used le32_to_cpu to convert crc32_le(), and we should cpu_to_le32.

Ths commit also fixes the payload checking by memcmp instead of checking element by element and removes the unused variable.

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

index a99f439328f1..8dc6a976b487 100644

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

@@ -537,7 +532,7 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, u8 *precvframe)
 	u32 pnh;
 	u8   rc4key[16];
 	u8   ttkey[16];
-	u8 crc[4];
+	__le32 crc;
 	signed int			length;
 
 	u8 *pframe, *payload, *iv, *prwskey;

This patch fixes the sparse warning of fix cast to restricted __le32.

There was a change for replacing private CRC-32 routines with in kernel ones.
However, the author used le32_to_cpu to convert crc32_le(), and we should use cpu_to_le32.

Reported-by kernel test robot <[email protected]>

Signed-off-by Jhih-Ming Huang <[email protected]>

---
 drivers/staging/rtl8723bs/core/rtw_security.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

drivers/staging/rtl8723bs/core/rtw_security.c +3 -9

index a99f439328f1..ff79e1aacd1a 100644
@@ -92,7 +92,6 @@ void rtw_wep_encrypt(struct adapter *padapter, u8 *pxmitframe)
 void rtw_wep_decrypt(struct adapter  *padapter, u8 *precvframe)
 {
 	/*  exclude ICV */
-	u8 crc[4];
 	signed int	length;
 	u32 keylength;
 	u8 *pframe, *payload, *iv, wepkey[16];

@@ -119,10 +118,6 @@ void rtw_wep_decrypt(struct adapter  *padapter, u8 *precvframe)
 		/* decrypt payload include icv */
 		arc4_setkey(ctx, wepkey, 3 + keylength);
 		arc4_crypt(ctx, payload, payload,  length);
-
-		/* calculate icv and compare the icv */
-		*((u32 *)crc) = le32_to_cpu(~crc32_le(~0, payload, length - 4));
-
 	}
 }
 

@@ -537,7 +532,7 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, u8 *precvframe)
 	u32 pnh;
 	u8   rc4key[16];
 	u8   ttkey[16];
-	u8 crc[4];
+	__le32 crc;
 	signed int			length;
 
 	u8 *pframe, *payload, *iv, *prwskey;

@@ -618,10 +613,9 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, u8 *precvframe)
 			arc4_setkey(ctx, rc4key, 16);
 			arc4_crypt(ctx, payload, payload, length);
 
-			*((u32 *)crc) = le32_to_cpu(~crc32_le(~0, payload, length - 4));
+			crc = cpu_to_le32(~crc32_le(~0, payload, length - 4));
 
-			if (crc[3] != payload[length - 1] || crc[2] != payload[length - 2] ||
-			    crc[1] != payload[length - 3] || crc[0] != payload[length - 4])
+			if (crc != get_unaligned_le32(payload + length - 4))
 				res = _FAIL;
 		} else {
 			res = _FAIL;

Signature · 1 line

2.32.0

This patch fixes the sparse warning of fix cast to restricted __le32.

There was a change for replacing private CRC-32 routines with in kernel ones.
However, the author used le32_to_cpu to convert crc32_le(), and we should use cpu_to_le32.

Reported-by kernel test robot <[email protected]>

Signed-off-by Jhih-Ming Huang <[email protected]>

---
changes from v6:
using get_unaligned_le32 to get the value and comparing it with crc, instead of using memcmp with raw pointers.

 drivers/staging/rtl8723bs/core/rtw_security.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

Quoted 22 lines · Jhih-Ming Huang

This patch fixes the sparse warning of fix cast to restricted __le32.

There was a change for replacing private CRC-32 routines with in kernel ones.
However, the author used le32_to_cpu to convert crc32_le(), and we should cpu_to_le32.

Ths commit also fixes the payload checking by memcmp instead of checking element by element.

Signed-off-by Jhih-Ming Huang <[email protected]>

---
 drivers/staging/rtl8723bs/core/rtw_security.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

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

index a99f439328f1..97a7485f8f58 100644
@@ -121,7 +121,7 @@ void rtw_wep_decrypt(struct adapter  *padapter, u8 *precvframe)
 		arc4_crypt(ctx, payload, payload,  length);
 

drivers/staging/rtl8723bs/core/rtw_security.c void rtw_wep_decrypt(struct adapter *padapter, u8 *precvframe) · line 124

Quoted 3 lines

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

 		/* calculate icv and compare the icv */
-		*((u32 *)crc) = le32_to_cpu(~crc32_le(~0, payload, length - 4));
+		*crc = cpu_to_le32(~crc32_le(~0, payload, length - 4));

Huh? crc is u8[4]; that assignment will truncate that le32 to u8 and store it in the first byte of your 4-element array. How the hell does sparse *not* complain on that?

Either make crc __le32 (and turn assignment into crc = cpu_to_le32(...)), or make that *(__le32 *)crc = ...

drivers/staging/rtl8723bs/core/rtw_security.c u32 rtw_tkip_decrypt(struct adapter *padapter, u8 *precvframe) · line 621

Quoted 6 lines

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

@@ -618,10 +618,9 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, u8 *precvframe)
 			arc4_setkey(ctx, rc4key, 16);
 			arc4_crypt(ctx, payload, payload, length);
 
-			*((u32 *)crc) = le32_to_cpu(~crc32_le(~0, payload, length - 4));
+			*crc = cpu_to_le32(~crc32_le(~0, payload, length - 4));

Ditto. Declare crc as __le32 and use

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

here.

Quoted 9 lines · Jhih Ming Huang

Thanks for your explanation.

To clarify, even though it might be false positives in some senses, following "hold the variable native-endian and check the conversion done correctly"
is much easier than the other way. And it's exactly the current implementation.

So it's better to keep the current implementation and ignore the warnings, right?

Umm... If that's the case, the warnings should go away if you use cpu_to_le32() for conversions from native to l-e and le32_to_cpu() for conversions from l-e to native.

IOW, the choice between those should annotate what's going on.

In your case doing

        *((u32 *)crc) = le32_to_cpu((__force __le32)~crc32_le(~0, payload, length - 4));

is wrong - you have
crc32_le(...) native-endian
~crc32_le(...) - ditto
le32_to_cpu(~crc32_le(...)) - byteswapped native-endian on b-e, unchanged on l-e. So result will be little-endian representation of ~crc32(...) in all cases. IOW, it's cpu_to_le32(~crc32_le(...)), misannotated as native-endian instead of little-endian it actually is.

Then you store that value (actually __le32) into *(u32 *)crc. Seeing that crc is u8[4] there, that *(u32 *) is misleading - you are actually storing __le32 there (and, AFAICS, doing noting with the result). The same story in rtw_tkip_decrypt(), only there you do use the result later.

So just make it __le32 crc and

        crc = cpu_to_le32(~crc32_le(~0, payload, length - 4));
with
                        if (crc[3] != payload[length - 1] || crc[2] != payload[length - 2] ||
                            crc[1] != payload[length - 3] || crc[0] != payload[length - 4])
turned into
                        if (memcmp(&crc, payload + length - 4, 4) != 0)

(or (crc != get_unaligned((__le32 *)(payload + length - 4))), for that matter, to document what's going on and let the damn thing pick the optimal implementation for given architecture).

Incidentally, your secmicgetuint32() is simply get_unaligned_le32() and secmicputuint32() - put_unaligned_le32(). No need to reinvent that wheel...