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.
This patch uses __force to fix the warning.
Signed-off-by: Jhih-Ming Huang <[email protected]>
---
drivers/staging/rtl8723bs/core/rtw_security.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
index a99f439328f1..2f4da67e31c6 100644
--- a/drivers/staging/rtl8723bs/core/rtw_security.c
+++ b/drivers/staging/rtl8723bs/core/rtw_security.c
@@ -121,7 +121,7 @@ void rtw_wep_decrypt(struct adapter *padapter, u8 *precvframe)
arc4_crypt(ctx, payload, payload, length);
/* calculate icv and compare the icv */
- *((u32 *)crc) = le32_to_cpu(~crc32_le(~0, payload, length - 4));
+ *((u32 *)crc) = le32_to_cpu((__force __le32)~crc32_le(~0, payload, length - 4));
}
}
@@ -618,7 +618,8 @@ 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));
+ *((u32 *)crc) = le32_to_cpu((__force __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])
--
2.25.1
On Sun, Jun 13, 2021 at 08:28:58PM +0800, Jhih-Ming Huang wrote:
> 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
On Sun, Jun 13, 2021 at 8:34 PM Greg KH <[email protected]> wrote:
>
> On Sun, Jun 13, 2021 at 08:28:58PM +0800, Jhih-Ming Huang wrote:
> > 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
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?
thanks
--jmhuang
On Mon, Jun 14, 2021 at 12:40:27AM +0800, Jhih Ming Huang wrote:
> On Sun, Jun 13, 2021 at 8:34 PM Greg KH <[email protected]> wrote:
> >
> > On Sun, Jun 13, 2021 at 08:28:58PM +0800, Jhih-Ming Huang wrote:
> > > 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
>
> 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.
On Mon, Jun 14, 2021 at 10:14 PM Al Viro <[email protected]> wrote:
>
> On Mon, Jun 14, 2021 at 12:40:27AM +0800, Jhih Ming Huang wrote:
> > On Sun, Jun 13, 2021 at 8:34 PM Greg KH <[email protected]> wrote:
> > >
> > > On Sun, Jun 13, 2021 at 08:28:58PM +0800, Jhih-Ming Huang wrote:
> > > > 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
> >
> > 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?
Thanks. Regards
--jmhuang
On Mon, Jun 14, 2021 at 11:27:03PM +0800, Jhih Ming Huang wrote:
> 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...
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(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
index a99f439328f1..97a7485f8f58 100644
--- a/drivers/staging/rtl8723bs/core/rtw_security.c
+++ b/drivers/staging/rtl8723bs/core/rtw_security.c
@@ -121,7 +121,7 @@ void rtw_wep_decrypt(struct adapter *padapter, u8 *precvframe)
arc4_crypt(ctx, payload, payload, length);
/* 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));
}
}
@@ -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));
- 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;
--
2.32.0.288
On Tue, Jun 15, 2021 at 1:03 AM Al Viro <[email protected]> wrote:
>
> On Mon, Jun 14, 2021 at 11:27:03PM +0800, Jhih Ming Huang wrote:
>
> > 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...
>
Thanks for your comprehensive explanation.
I just sent the v3 PATCH, but I replied to this thread.
Should I create the other thread?
For the secmicgetuint32(), I am not the author of this function,
but you are right we should not reinvent the wheel.
Let's focus on sparse warning fixing in this commit.
thanks.
--jmhuang
On Sat, Jun 19, 2021 at 02:17:51AM +0800, Jhih-Ming Huang wrote:
> 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(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
> index a99f439328f1..97a7485f8f58 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_security.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_security.c
> @@ -121,7 +121,7 @@ void rtw_wep_decrypt(struct adapter *padapter, u8 *precvframe)
> arc4_crypt(ctx, payload, payload, length);
>
> /* 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 = ...
> @@ -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.
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 | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
index a99f439328f1..3a2711e21a0f 100644
--- a/drivers/staging/rtl8723bs/core/rtw_security.c
+++ b/drivers/staging/rtl8723bs/core/rtw_security.c
@@ -92,7 +92,7 @@ void rtw_wep_encrypt(struct adapter *padapter, u8 *pxmitframe)
void rtw_wep_decrypt(struct adapter *padapter, u8 *precvframe)
{
/* exclude ICV */
- u8 crc[4];
+ __le32 crc;
signed int length;
u32 keylength;
u8 *pframe, *payload, *iv, wepkey[16];
@@ -121,7 +121,7 @@ void rtw_wep_decrypt(struct adapter *padapter, u8 *precvframe)
arc4_crypt(ctx, payload, payload, length);
/* 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));
}
}
@@ -537,7 +537,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 +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));
- 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;
--
2.32.0.288.g62a8d224e6-goog
On Fri, Jun 18, 2021 at 07:29:16PM +0000, Al Viro wrote:
> On Sat, Jun 19, 2021 at 02:17:51AM +0800, Jhih-Ming Huang wrote:
> > 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(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
> > index a99f439328f1..97a7485f8f58 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_security.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_security.c
> > @@ -121,7 +121,7 @@ void rtw_wep_decrypt(struct adapter *padapter, u8 *precvframe)
> > arc4_crypt(ctx, payload, payload, length);
> >
> > /* 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?
facepalm... fixed in v4 PATCH.
thanks for your help.
>
> Either make crc __le32 (and turn assignment into crc = cpu_to_le32(...)), or
> make that *(__le32 *)crc = ...
>
> > @@ -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.
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(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
index a99f439328f1..8dc6a976b487 100644
--- a/drivers/staging/rtl8723bs/core/rtw_security.c
+++ b/drivers/staging/rtl8723bs/core/rtw_security.c
@@ -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;
--
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 cpu_to_le32.
Ths commit also fixes the payload checking by memcmp instead of checking element
by element and removes the unused variable.
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(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
index a99f439328f1..8dc6a976b487 100644
--- a/drivers/staging/rtl8723bs/core/rtw_security.c
+++ b/drivers/staging/rtl8723bs/core/rtw_security.c
@@ -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;
--
2.32.0
On Mon, Jun 21, 2021 at 04:19:28PM +0800, Jhih-Ming Huang wrote:
> 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]>
I forgot to add the Reported-by tag.
already added in PATCH v6
thanks.
jmhuang
> ---
> drivers/staging/rtl8723bs/core/rtw_security.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
> index a99f439328f1..8dc6a976b487 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_security.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_security.c
> @@ -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;
> --
> 2.32.0
>
From: Jhih-Ming Huang
> 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.
...
> diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c
> b/drivers/staging/rtl8723bs/core/rtw_security.c
> index a99f439328f1..8dc6a976b487 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_security.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_security.c
...
> @@ -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)
Shouldn't this be using (IIRC) get_unaligned_le32() ?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
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(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
index a99f439328f1..ff79e1aacd1a 100644
--- a/drivers/staging/rtl8723bs/core/rtw_security.c
+++ b/drivers/staging/rtl8723bs/core/rtw_security.c
@@ -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;
--
2.32.0
On Sun, Jul 04, 2021 at 06:31:12PM +0800, Jhih-Ming Huang wrote:
> 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(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
> index a99f439328f1..ff79e1aacd1a 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_security.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_security.c
> @@ -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;
> --
> 2.32.0
>
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/SubmittingPatches for what needs to be done
here to properly describe this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
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(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
index a99f439328f1..ff79e1aacd1a 100644
--- a/drivers/staging/rtl8723bs/core/rtw_security.c
+++ b/drivers/staging/rtl8723bs/core/rtw_security.c
@@ -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;
--
2.32.0
On Sun, Aug 01, 2021 at 11:51:52PM +0800, Jhih-Ming Huang wrote:
> 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(-)
Does not apply to the tree :(