[PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Gaston Gonzalez]

Silence the following sparse warning:

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c:184:16: warning: cast to restricted __le16

Signed-off-by: Gaston Gonzalez <gascoar@gmail.com>
---
 .../rtl8192u/ieee80211/ieee80211_crypt_tkip.c      | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
index e815c81..efa6983 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
@@ -179,1 +179,1 @@ static inline u16 Mk16(u8 hi, u8 lo)
 }


-static inline u16 Mk16_le(u16 *v)
+static inline u16 Mk16_le(__le16 *v)
 {
 	return le16_to_cpu(*v);
 }
@@ -270,1 +270,1 @@ static void tkip_mixing_phase2(u8 *WEPSeed, const u8 *TK, const u16 *TTAK,
 	PPK[5] = TTAK[4] + IV16;

 	/* Step 2 - 96-bit bijective mixing using S-box */
-	PPK[0] += _S_(PPK[5] ^ Mk16_le((u16 *) &TK[0]));
-	PPK[1] += _S_(PPK[0] ^ Mk16_le((u16 *) &TK[2]));
-	PPK[2] += _S_(PPK[1] ^ Mk16_le((u16 *) &TK[4]));
-	PPK[3] += _S_(PPK[2] ^ Mk16_le((u16 *) &TK[6]));
-	PPK[4] += _S_(PPK[3] ^ Mk16_le((u16 *) &TK[8]));
-	PPK[5] += _S_(PPK[4] ^ Mk16_le((u16 *) &TK[10]));
-
-	PPK[0] += RotR1(PPK[5] ^ Mk16_le((u16 *) &TK[12]));
-	PPK[1] += RotR1(PPK[0] ^ Mk16_le((u16 *) &TK[14]));
+	PPK[0] += _S_(PPK[5] ^ Mk16_le((__le16 *) &TK[0]));
+	PPK[1] += _S_(PPK[0] ^ Mk16_le((__le16 *) &TK[2]));
+	PPK[2] += _S_(PPK[1] ^ Mk16_le((__le16 *) &TK[4]));
+	PPK[3] += _S_(PPK[2] ^ Mk16_le((__le16 *) &TK[6]));
+	PPK[4] += _S_(PPK[3] ^ Mk16_le((__le16 *) &TK[8]));
+	PPK[5] += _S_(PPK[4] ^ Mk16_le((__le16 *) &TK[10]));
+
+	PPK[0] += RotR1(PPK[5] ^ Mk16_le((__le16 *) &TK[12]));
+	PPK[1] += RotR1(PPK[0] ^ Mk16_le((__le16 *) &TK[14]));
 	PPK[2] += RotR1(PPK[1]);
 	PPK[3] += RotR1(PPK[2]);
 	PPK[4] += RotR1(PPK[3]);
@@ -289,4 +289,4 @@ static void tkip_mixing_phase2(u8 *WEPSeed, const u8 *TK, const u16 *TTAK,
 	WEPSeed[0] = Hi8(IV16);
 	WEPSeed[1] = (Hi8(IV16) | 0x20) & 0x7F;
 	WEPSeed[2] = Lo8(IV16);
-	WEPSeed[3] = Lo8((PPK[5] ^ Mk16_le((u16 *) &TK[0])) >> 1);
+	WEPSeed[3] = Lo8((PPK[5] ^ Mk16_le((__le16 *) &TK[0])) >> 1);

 #ifdef __BIG_ENDIAN
 	{
--
2.1.4

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Dan Carpenter]

On Sun, Apr 19, 2015 at 05:43:51PM -0300, Gaston Gonzalez wrote:
> Silence the following sparse warning:
> 
> drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c:184:16: warning: cast to restricted __le16
> 

We have a tkip_mixing_phase2() function in net/mac80211/tkip.c.  That's
probably the better thing.

> Signed-off-by: Gaston Gonzalez <gascoar@gmail.com>
> ---
>  .../rtl8192u/ieee80211/ieee80211_crypt_tkip.c      | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
> index e815c81..efa6983 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
> @@ -179,1 +179,1 @@ static inline u16 Mk16(u8 hi, u8 lo)
>  }
> 
> 
> -static inline u16 Mk16_le(u16 *v)
> +static inline u16 Mk16_le(__le16 *v)
>  {
>  	return le16_to_cpu(*v);
>  }


Mk16_le() is a bad function name and as we can see from tkip.c it just
duplicates get_unaligned_le16().  Better to make TK void pointer instead
of a u8 pointer (because it doesn't point to u8s so we have to cast it
every time we use it).  This is another trick I learned from tkip.c.

regards,
dan carpenter

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Gaston Gonzalez]

On 20/04/15 05:24, Dan Carpenter wrote:
> Mk16_le() is a bad function name and as we can see from tkip.c it just
> duplicates get_unaligned_le16().  Better to make TK void pointer instead
> of a u8 pointer (because it doesn't point to u8s so we have to cast it
> every time we use it).  This is another trick I learned from tkip.c.
Ok. Could I just remove Mk16_le() and use get_unaligned_le16() instead?

I built rtl8192u/ using get_unaligned_le16() for the following lines and
I didn't get any warning:

+#include <asm/unaligned.h>

-    PPK[0] += _S_(PPK[5] ^ Mk16_le((u16 *) &TK[0]));
-    PPK[0] += _S_(PPK[5] ^ Mk16_le((u16 *) &TK[0]));
-    PPK[1] += _S_(PPK[0] ^ Mk16_le((u16 *) &TK[2]));
-    PPK[2] += _S_(PPK[1] ^ Mk16_le((u16 *) &TK[4]));
-    PPK[3] += _S_(PPK[2] ^ Mk16_le((u16 *) &TK[6]));
-    PPK[4] += _S_(PPK[3] ^ Mk16_le((u16 *) &TK[8]));
-    PPK[5] += _S_(PPK[4] ^ Mk16_le((u16 *) &TK[10]));
-
-    PPK[0] += RotR1(PPK[5] ^ Mk16_le((u16 *) &TK[12]));
-    PPK[1] += RotR1(PPK[0] ^ Mk16_le((u16 *) &TK[14]));
+    PPK[0] += _S_(PPK[5] ^ get_unaligned_le16(TK + 0));
+    PPK[1] += _S_(PPK[0] ^ get_unaligned_le16(TK + 2));
+    PPK[2] += _S_(PPK[1] ^ get_unaligned_le16(TK + 4));
+    PPK[3] += _S_(PPK[2] ^ get_unaligned_le16(TK + 6));
+    PPK[4] += _S_(PPK[3] ^ get_unaligned_le16(TK + 8));
+    PPK[5] += _S_(PPK[4] ^ get_unaligned_le16(TK + 10));
+
+    PPK[0] += RotR1(PPK[5] ^ get_unaligned_le16(TK + 12));
+    PPK[1] += RotR1(PPK[0] ^ get_unaligned_le16(TK + 14));

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Dan Carpenter]

Can't we just export the tkip.c function?

regards,
dan carpenter

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Gaston Gonzalez]

On 27/04/15 07:12, Dan Carpenter wrote:
> Can't we just export the tkip.c function?
>
> regards,
> dan carpenter
>
Hi Dan,

(sorry for the delayed response)

The inputs of the two implementations of tkip_mixing_phase2() differ in
one parameter:

 - ieee80211_crypt_tkip.c expects 'const u16 *TTAK'
 - tkip.c expects 'struct tkip_ctx *ctx'

tkip_mixing_phase2() is called two times in ieee80211_crypt_tkip.c:

git grep tkip_mixing_phase2  drivers/staging/rtl8192u/ieee80211/

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c:static void
tkip_mixing_phase2(u8 *WEPSeed, const u8 *TK, const u16 *TTAK,

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c:             
tkip_mixing_phase2(rc4key, tkey->key, tkey->tx_ttak, tkey->tx_iv16);

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c:             
tkip_mixing_phase2(rc4key, tkey->key, tkey->rx_ttak, iv16);


tkey is a 'struct ieee80211_tkip_data', which differs from tkip_ctx
structure. So in the case of exporting the tkip.c function, we would
need to add 'struct tkip_ctx' definition, and not just change the
function input definition.

The only member of tkip_ctx structure used in tkip_mixing_phase2() is
p1k, which is the equivalent of TTAK array in ieee80211_crypt_tkip.c .
Thus - I'm new to this so I may be missing something - one way would be
to just add the tkip_ctx structure definition in ieee80211_crypt_tkip.c.
The down side of doing this is that there would be some parameters
defined twice in two different structures: u32 iv32, u16 iv16, u16
p1k[5], u32 p1k_iv32

Another way would be split ieee80211_tkip_data structure in struct
tkip_ctx on one side, and leave the rest of the members on the original
structure. This would avoid to duplicate the members definitions, but I
guess that approach could broke other things...

As I said, I'm new to this so I may be missing something, maybe a
different approach?

One detail: although not used in the function, 'enum
ieee80211_internal_tkip_state state' is not defined in 'struct
ieee80211_tkip_data', thus in any case that definition would have to be
added.

regards,

Gaston

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Dan Carpenter]

To be honest, I'm a little bit a newbie myself when it comes to
linux-wireless so keep that in mind.  ;)  Changing the parameters seems
simple enough.  But it needs to an EXPORT_SYMBOL() and to be declared in
a header file and I'm not sure what else.  But we have four
implementations of this function now.

diff --git a/net/mac80211/tkip.c b/net/mac80211/tkip.c
index 0ae2077..f149593 100644
--- a/net/mac80211/tkip.c
+++ b/net/mac80211/tkip.c
@@ -105,11 +105,9 @@ static void tkip_mixing_phase1(const u8 *tk, struct tkip_ctx *ctx,
 	ctx->p1k_iv32 = tsc_IV32;
 }
 
-static void tkip_mixing_phase2(const u8 *tk, struct tkip_ctx *ctx,
-			       u16 tsc_IV16, u8 *rc4key)
+void tkip_mixing_phase2(const u8 *tk, const u16 *p1k, u16 tsc_IV16, u8 *rc4key)
 {
 	u16 ppk[6];
-	const u16 *p1k = ctx->p1k;
 	int i;
 
 	ppk[0] = p1k[0];
@@ -210,7 +208,7 @@ void ieee80211_get_tkip_p2k(struct ieee80211_key_conf *keyconf,
 
 	spin_lock(&key->u.tkip.txlock);
 	ieee80211_compute_tkip_p1k(key, iv32);
-	tkip_mixing_phase2(tk, ctx, iv16, p2k);
+	tkip_mixing_phase2(tk, ctx->p1k, iv16, p2k);
 	spin_unlock(&key->u.tkip.txlock);
 }
 EXPORT_SYMBOL(ieee80211_get_tkip_p2k);
@@ -295,7 +293,7 @@ int ieee80211_tkip_decrypt_data(struct crypto_cipher *tfm,
 		key->u.tkip.rx[queue].state = TKIP_STATE_PHASE1_HW_UPLOADED;
 	}
 
-	tkip_mixing_phase2(tk, &key->u.tkip.rx[queue], iv16, rc4key);
+	tkip_mixing_phase2(tk, key->u.tkip.rx[queue].p1k, iv16, rc4key);
 
 	res = ieee80211_wep_decrypt_data(tfm, rc4key, 16, pos, payload_len - 12);
  done:

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Gaston Gonzalez]

On 08/05/15 08:03, Dan Carpenter wrote:
> To be honest, I'm a little bit a newbie myself when it comes to
> linux-wireless so keep that in mind.  ;)  Changing the parameters seems
> simple enough.  But it needs to an EXPORT_SYMBOL() and to be declared in
> a header file and I'm not sure what else.  But we have four
> implementations of this function now.
Ok Dan, I did the test exporting tkip_mixing_phase2(). Below is how the
patch would look.
So far I didn't get any warnings.

Summary and comments:

 - Goal: use tkip_mixing_phase2() from /net/mac80211/tkip.c in
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c

 - Approach: export  tkip_mixing_phase2(), adding tkip_ctx structure
definition without toucing original structures from
ieee80211_crypt_tkip.c. As you pointed out, this is done by adding
EXPORT_SYMBOL() and declaring the function in the destined file.

 - As commented in the previous email, adding tkip_ctx structure
duplicates some members. Only one of them is used in the function: p1k,
so it is copied from one structure to the other.

 - Please let me know if the implementation is useful or is better
another approach or if it needs changes.

 - In the case this is well oriented, the submission form would be a new
patch or v2 of the original one?

regards,

Gaston


---
 .../rtl8192u/ieee80211/ieee80211_crypt_tkip.c      | 79
++++++++--------------
 net/mac80211/tkip.c                                |  3 +-
 2 files changed, 32 insertions(+), 50 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
index 1f80c52..51e2034 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
@@ -18,3 +18,4 @@
 #include <linux/if_ether.h>
 #include <linux/if_arp.h>
 #include <linux/string.h>
+#include <net/mac80211.h>

 #include "ieee80211.h"

@@ -61,2 +62,2 @@ struct ieee80211_tkip_data {
     u8 rx_hdr[16], tx_hdr[16];
 };

+enum ieee80211_internal_tkip_state {
+    TKIP_STATE_NOT_INIT,
+    TKIP_STATE_PHASE1_DONE,
+    TKIP_STATE_PHASE1_HW_UPLOADED,
+};
+
+struct tkip_ctx {
+    u32 iv32;    /* current iv32 */
+    u16 iv16;    /* current iv16 */
+    u16 p1k[5];    /* p1k cache */
+    u32 p1k_iv32;    /* iv32 for which p1k computed */
+    enum ieee80211_internal_tkip_state state;
+};
+
+void tkip_mixing_phase2(const u8 *tk, struct tkip_ctx *ctx,
+            u16 tsc_IV16, u8 *rc4key);
+
 static void *ieee80211_tkip_init(int key_idx)
 {
     struct ieee80211_tkip_data *priv;
@@ -254,1 +272,1 @@ static void tkip_mixing_phase1(u16 *TTAK, const u8
*TK, const u8 *TA, u32 IV32)
 }


-static void tkip_mixing_phase2(u8 *WEPSeed, const u8 *TK, const u16 *TTAK,
-                   u16 IV16)
-{
-    /* Make temporary area overlap WEP seed so that the final copy can be
-     * avoided on little endian hosts. */
-    u16 *PPK = (u16 *) &WEPSeed[4];
-
-    /* Step 1 - make copy of TTAK and bring in TSC */
-    PPK[0] = TTAK[0];
-    PPK[1] = TTAK[1];
-    PPK[2] = TTAK[2];
-    PPK[3] = TTAK[3];
-    PPK[4] = TTAK[4];
-    PPK[5] = TTAK[4] + IV16;
-
-    /* Step 2 - 96-bit bijective mixing using S-box */
-    PPK[0] += _S_(PPK[5] ^ Mk16_le((u16 *) &TK[0]));
-    PPK[1] += _S_(PPK[0] ^ Mk16_le((u16 *) &TK[2]));
-    PPK[2] += _S_(PPK[1] ^ Mk16_le((u16 *) &TK[4]));
-    PPK[3] += _S_(PPK[2] ^ Mk16_le((u16 *) &TK[6]));
-    PPK[4] += _S_(PPK[3] ^ Mk16_le((u16 *) &TK[8]));
-    PPK[5] += _S_(PPK[4] ^ Mk16_le((u16 *) &TK[10]));
-
-    PPK[0] += RotR1(PPK[5] ^ Mk16_le((u16 *) &TK[12]));
-    PPK[1] += RotR1(PPK[0] ^ Mk16_le((u16 *) &TK[14]));
-    PPK[2] += RotR1(PPK[1]);
-    PPK[3] += RotR1(PPK[2]);
-    PPK[4] += RotR1(PPK[3]);
-    PPK[5] += RotR1(PPK[4]);
-
-    /* Step 3 - bring in last of TK bits, assign 24-bit WEP IV value
-     * WEPSeed[0..2] is transmitted as WEP IV */
-    WEPSeed[0] = Hi8(IV16);
-    WEPSeed[1] = (Hi8(IV16) | 0x20) & 0x7F;
-    WEPSeed[2] = Lo8(IV16);
-    WEPSeed[3] = Lo8((PPK[5] ^ Mk16_le((u16 *) &TK[0])) >> 1);
-
-#ifdef __BIG_ENDIAN
-    {
-        int i;
-        for (i = 0; i < 6; i++)
-            PPK[i] = (PPK[i] << 8) | (PPK[i] >> 8);
-    }
-#endif
-}
-
-
 static int ieee80211_tkip_encrypt(struct sk_buff *skb, int hdr_len,
void *priv)
 {
     struct ieee80211_tkip_data *tkey = priv;
         int len;
     u8 *pos;
+    struct tkip_ctx *tx = NULL;
+    size_t p1k_len;
     struct rtl_80211_hdr_4addr *hdr;
     cb_desc *tcb_desc = (cb_desc *)(skb->cb + MAX_DEV_ADDR_SIZE);
     struct blkcipher_desc desc = {.tfm = tkey->tx_tfm_arc4};
@@ -314,2 +287,2 @@ static int ieee80211_tkip_encrypt(struct sk_buff
*skb, int hdr_len, void *priv)
     u32 crc;
     struct scatterlist sg;

+    p1k_len = sizeof(tkey->tx_ttak);
+    memcpy(&tx->p1k, &tkey->tx_ttak, p1k_len);
+
     if (skb_headroom(skb) < 8 || skb_tailroom(skb) < 4 ||
         skb->len < hdr_len)
         return -1;
@@ -327,7 +303,7 @@ static int ieee80211_tkip_encrypt(struct sk_buff
*skb, int hdr_len, void *priv)
                     tkey->tx_iv32);
             tkey->tx_phase1_done = 1;
         }
-        tkip_mixing_phase2(rc4key, tkey->key, tkey->tx_ttak,
tkey->tx_iv16);
+        tkip_mixing_phase2(tkey->key, tx, tkey->tx_iv16, rc4key);
     }
     else
     tkey->tx_phase1_done = 1;
@@ -387,6 +363,8 @@ static int ieee80211_tkip_encrypt(struct sk_buff
*skb, int hdr_len, void *priv)
 static int ieee80211_tkip_decrypt(struct sk_buff *skb, int hdr_len,
void *priv)
 {
     struct ieee80211_tkip_data *tkey = priv;
+    struct tkip_ctx *rx = NULL;
+    size_t p1k_len;
     u8 keyidx, *pos;
     u32 iv32;
     u16 iv16;
@@ -401,2 +379,2 @@ static int ieee80211_tkip_decrypt(struct sk_buff
*skb, int hdr_len, void *priv)
     if (skb->len < hdr_len + 8 + 4)
         return -1;

+    p1k_len = sizeof(tkey->rx_ttak);
+    memcpy(&rx->p1k, &tkey->rx_ttak, p1k_len);
+
     hdr = (struct rtl_80211_hdr_4addr *) skb->data;
     pos = skb->data + hdr_len;
     keyidx = pos[3];
@@ -447,4 +428,4 @@ static int ieee80211_tkip_decrypt(struct sk_buff
*skb, int hdr_len, void *priv)
             tkip_mixing_phase1(tkey->rx_ttak, tkey->key, hdr->addr2, iv32);
             tkey->rx_phase1_done = 1;
         }
-        tkip_mixing_phase2(rc4key, tkey->key, tkey->rx_ttak, iv16);
+        tkip_mixing_phase2(tkey->key, rx, tkey->rx_iv16, rc4key);

         plen = skb->len - hdr_len - 12;

diff --git a/net/mac80211/tkip.c b/net/mac80211/tkip.c
index 0ae2077..1f74866 100644
--- a/net/mac80211/tkip.c
+++ b/net/mac80211/tkip.c
@@ -105,2 +105,2 @@ static void tkip_mixing_phase1(const u8 *tk, struct
tkip_ctx *ctx,
     ctx->p1k_iv32 = tsc_IV32;
 }

-static void tkip_mixing_phase2(const u8 *tk, struct tkip_ctx *ctx,
+void tkip_mixing_phase2(const u8 *tk, struct tkip_ctx *ctx,
                    u16 tsc_IV16, u8 *rc4key)
 {
     u16 ppk[6];
@@ -138,3 +138,4 @@ static void tkip_mixing_phase2(const u8 *tk, struct
tkip_ctx *ctx,
     for (i = 0; i < 6; i++)
         put_unaligned_le16(ppk[i], rc4key + 2 * i);
 }
+EXPORT_SYMBOL(tkip_mixing_phase2);

 /* Add TKIP IV and Ext. IV at @pos. @iv0, @iv1, and @iv2 are the first
octets
  * of the IV. Returns pointer to the octet following IVs (i.e.,
beginning of
--
2.1.4

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Julian Calaby]

Hi Gaston,

A couple of minor style comments:

On Wed, May 13, 2015 at 8:23 AM, Gaston Gonzalez <gascoar@gmail.com> wrote:
> On 08/05/15 08:03, Dan Carpenter wrote:
>> To be honest, I'm a little bit a newbie myself when it comes to
>> linux-wireless so keep that in mind.  ;)  Changing the parameters seems
>> simple enough.  But it needs to an EXPORT_SYMBOL() and to be declared in
>> a header file and I'm not sure what else.  But we have four
>> implementations of this function now.
> Ok Dan, I did the test exporting tkip_mixing_phase2(). Below is how the
> patch would look.
> So far I didn't get any warnings.
>
> Summary and comments:
>
>  - Goal: use tkip_mixing_phase2() from /net/mac80211/tkip.c in
> drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
>
>  - Approach: export  tkip_mixing_phase2(), adding tkip_ctx structure
> definition without toucing original structures from
> ieee80211_crypt_tkip.c. As you pointed out, this is done by adding
> EXPORT_SYMBOL() and declaring the function in the destined file.
>
>  - As commented in the previous email, adding tkip_ctx structure
> duplicates some members. Only one of them is used in the function: p1k,
> so it is copied from one structure to the other.
>
>  - Please let me know if the implementation is useful or is better
> another approach or if it needs changes.
>
>  - In the case this is well oriented, the submission form would be a new
> patch or v2 of the original one?
>
> regards,
>
> Gaston
>
>
> ---
>  .../rtl8192u/ieee80211/ieee80211_crypt_tkip.c      | 79
> ++++++++--------------
>  net/mac80211/tkip.c                                |  3 +-
>  2 files changed, 32 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
> b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
> index 1f80c52..51e2034 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
> @@ -18,3 +18,4 @@
>  #include <linux/if_ether.h>
>  #include <linux/if_arp.h>
>  #include <linux/string.h>
> +#include <net/mac80211.h>
>
>  #include "ieee80211.h"
>
> @@ -61,2 +62,2 @@ struct ieee80211_tkip_data {
>      u8 rx_hdr[16], tx_hdr[16];
>  };
>
> +enum ieee80211_internal_tkip_state {
> +    TKIP_STATE_NOT_INIT,
> +    TKIP_STATE_PHASE1_DONE,
> +    TKIP_STATE_PHASE1_HW_UPLOADED,
> +};
> +
> +struct tkip_ctx {
> +    u32 iv32;    /* current iv32 */
> +    u16 iv16;    /* current iv16 */
> +    u16 p1k[5];    /* p1k cache */
> +    u32 p1k_iv32;    /* iv32 for which p1k computed */
> +    enum ieee80211_internal_tkip_state state;
> +};
> +
> +void tkip_mixing_phase2(const u8 *tk, struct tkip_ctx *ctx,
> +            u16 tsc_IV16, u8 *rc4key);
> +

I'm guessing you copied all of this from mac80211/tkip.c. It should
get stuffed into a header somewhere, not copied to the top of the
file.

>  static void *ieee80211_tkip_init(int key_idx)
>  {
>      struct ieee80211_tkip_data *priv;
> @@ -254,1 +272,1 @@ static void tkip_mixing_phase1(u16 *TTAK, const u8
> *TK, const u8 *TA, u32 IV32)
>  }
>
>
> -static void tkip_mixing_phase2(u8 *WEPSeed, const u8 *TK, const u16 *TTAK,
> -                   u16 IV16)
> -{
> -    /* Make temporary area overlap WEP seed so that the final copy can be
> -     * avoided on little endian hosts. */
> -    u16 *PPK = (u16 *) &WEPSeed[4];
> -
> -    /* Step 1 - make copy of TTAK and bring in TSC */
> -    PPK[0] = TTAK[0];
> -    PPK[1] = TTAK[1];
> -    PPK[2] = TTAK[2];
> -    PPK[3] = TTAK[3];
> -    PPK[4] = TTAK[4];
> -    PPK[5] = TTAK[4] + IV16;
> -
> -    /* Step 2 - 96-bit bijective mixing using S-box */
> -    PPK[0] += _S_(PPK[5] ^ Mk16_le((u16 *) &TK[0]));
> -    PPK[1] += _S_(PPK[0] ^ Mk16_le((u16 *) &TK[2]));
> -    PPK[2] += _S_(PPK[1] ^ Mk16_le((u16 *) &TK[4]));
> -    PPK[3] += _S_(PPK[2] ^ Mk16_le((u16 *) &TK[6]));
> -    PPK[4] += _S_(PPK[3] ^ Mk16_le((u16 *) &TK[8]));
> -    PPK[5] += _S_(PPK[4] ^ Mk16_le((u16 *) &TK[10]));
> -
> -    PPK[0] += RotR1(PPK[5] ^ Mk16_le((u16 *) &TK[12]));
> -    PPK[1] += RotR1(PPK[0] ^ Mk16_le((u16 *) &TK[14]));
> -    PPK[2] += RotR1(PPK[1]);
> -    PPK[3] += RotR1(PPK[2]);
> -    PPK[4] += RotR1(PPK[3]);
> -    PPK[5] += RotR1(PPK[4]);
> -
> -    /* Step 3 - bring in last of TK bits, assign 24-bit WEP IV value
> -     * WEPSeed[0..2] is transmitted as WEP IV */
> -    WEPSeed[0] = Hi8(IV16);
> -    WEPSeed[1] = (Hi8(IV16) | 0x20) & 0x7F;
> -    WEPSeed[2] = Lo8(IV16);
> -    WEPSeed[3] = Lo8((PPK[5] ^ Mk16_le((u16 *) &TK[0])) >> 1);
> -
> -#ifdef __BIG_ENDIAN
> -    {
> -        int i;
> -        for (i = 0; i < 6; i++)
> -            PPK[i] = (PPK[i] << 8) | (PPK[i] >> 8);
> -    }
> -#endif
> -}
> -
> -
>  static int ieee80211_tkip_encrypt(struct sk_buff *skb, int hdr_len,
> void *priv)
>  {
>      struct ieee80211_tkip_data *tkey = priv;
>          int len;
>      u8 *pos;
> +    struct tkip_ctx *tx = NULL;
> +    size_t p1k_len;
>      struct rtl_80211_hdr_4addr *hdr;
>      cb_desc *tcb_desc = (cb_desc *)(skb->cb + MAX_DEV_ADDR_SIZE);
>      struct blkcipher_desc desc = {.tfm = tkey->tx_tfm_arc4};
> @@ -314,2 +287,2 @@ static int ieee80211_tkip_encrypt(struct sk_buff
> *skb, int hdr_len, void *priv)
>      u32 crc;
>      struct scatterlist sg;
>
> +    p1k_len = sizeof(tkey->tx_ttak);
> +    memcpy(&tx->p1k, &tkey->tx_ttak, p1k_len);
> +
>      if (skb_headroom(skb) < 8 || skb_tailroom(skb) < 4 ||
>          skb->len < hdr_len)
>          return -1;
> @@ -327,7 +303,7 @@ static int ieee80211_tkip_encrypt(struct sk_buff
> *skb, int hdr_len, void *priv)
>                      tkey->tx_iv32);
>              tkey->tx_phase1_done = 1;
>          }
> -        tkip_mixing_phase2(rc4key, tkey->key, tkey->tx_ttak,
> tkey->tx_iv16);
> +        tkip_mixing_phase2(tkey->key, tx, tkey->tx_iv16, rc4key);

You've done the exact same thing (define a struct tkip_ctx, p1k_len,
the sizeof() and memcpy() calls) in a few places just to produce the
second argument to tkip_mixing_phase2(). Why not make a helper (say
ieee80211_tkip_mixing_phase2()) that does all of this based on the
struct ieee80211_tkip_data and rc4key arguments? This would also
reduce the amount of stuff that needs to be exported.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Dan Carpenter]

What?  No, that patch can't work at all.  It will just oops directly
when you do:

> +    memcpy(&tx->p1k, &tkey->tx_ttak, p1k_len);

How come you didn't use my patch as a base to work from, you shouldn't
be passing tkip_ctx at all.

regards,
dan carpenter

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Gaston Gonzalez]

On 13/05/15 05:36, Dan Carpenter wrote:
> How come you didn't use my patch as a base to work from, you shouldn't
> be passing tkip_ctx at all.
Hi Dan, my mistake. To be honest I assumed that the idea was not to
touch tkip.c at all, that's why I had to pass tkip_ctx... sorry about
that :-(

Coming back to your patch: below is the implementation using it. Taking
the advice of Julian Calaby, tkip_mixing_phase2() was stuffed into a
header (other  tkip.c functions are in include/net/mac80211.h so I put
there, not sure if it is the right place, if it is I'll added the
documentation lines to it)

Please let me know if it is well oriented and what changes need to be done.

thanks a lot,

Gaston

---
 .../rtl8192u/ieee80211/ieee80211_crypt_tkip.c      | 53
++--------------------
 include/net/mac80211.h                             |  3 ++
 net/mac80211/tkip.c                                |  9 ++--
 3 files changed, 10 insertions(+), 55 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
index 1f80c52..b7d0b06 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
@@ -18,3 +18,4 @@
 #include <linux/if_ether.h>
 #include <linux/if_arp.h>
 #include <linux/string.h>
+#include <net/mac80211.h>

 #include "ieee80211.h"

@@ -253,2 +254,2 @@ static void tkip_mixing_phase1(u16 *TTAK, const u8
*TK, const u8 *TA, u32 IV32)
     }
 }

-
-static void tkip_mixing_phase2(u8 *WEPSeed, const u8 *TK, const u16 *TTAK,
-                   u16 IV16)
-{
-    /* Make temporary area overlap WEP seed so that the final copy can be
-     * avoided on little endian hosts. */
-    u16 *PPK = (u16 *) &WEPSeed[4];
-
-    /* Step 1 - make copy of TTAK and bring in TSC */
-    PPK[0] = TTAK[0];
-    PPK[1] = TTAK[1];
-    PPK[2] = TTAK[2];
-    PPK[3] = TTAK[3];
-    PPK[4] = TTAK[4];
-    PPK[5] = TTAK[4] + IV16;
-
-    /* Step 2 - 96-bit bijective mixing using S-box */
-    PPK[0] += _S_(PPK[5] ^ Mk16_le((u16 *) &TK[0]));
-    PPK[1] += _S_(PPK[0] ^ Mk16_le((u16 *) &TK[2]));
-    PPK[2] += _S_(PPK[1] ^ Mk16_le((u16 *) &TK[4]));
-    PPK[3] += _S_(PPK[2] ^ Mk16_le((u16 *) &TK[6]));
-    PPK[4] += _S_(PPK[3] ^ Mk16_le((u16 *) &TK[8]));
-    PPK[5] += _S_(PPK[4] ^ Mk16_le((u16 *) &TK[10]));
-
-    PPK[0] += RotR1(PPK[5] ^ Mk16_le((u16 *) &TK[12]));
-    PPK[1] += RotR1(PPK[0] ^ Mk16_le((u16 *) &TK[14]));
-    PPK[2] += RotR1(PPK[1]);
-    PPK[3] += RotR1(PPK[2]);
-    PPK[4] += RotR1(PPK[3]);
-    PPK[5] += RotR1(PPK[4]);
-
-    /* Step 3 - bring in last of TK bits, assign 24-bit WEP IV value
-     * WEPSeed[0..2] is transmitted as WEP IV */
-    WEPSeed[0] = Hi8(IV16);
-    WEPSeed[1] = (Hi8(IV16) | 0x20) & 0x7F;
-    WEPSeed[2] = Lo8(IV16);
-    WEPSeed[3] = Lo8((PPK[5] ^ Mk16_le((u16 *) &TK[0])) >> 1);
-
-#ifdef __BIG_ENDIAN
-    {
-        int i;
-        for (i = 0; i < 6; i++)
-            PPK[i] = (PPK[i] << 8) | (PPK[i] >> 8);
-    }
-#endif
-}
-
-
 static int ieee80211_tkip_encrypt(struct sk_buff *skb, int hdr_len,
void *priv)
 {
     struct ieee80211_tkip_data *tkey = priv;
@@ -327,7 +280,7 @@ static int ieee80211_tkip_encrypt(struct sk_buff
*skb, int hdr_len, void *priv)
                     tkey->tx_iv32);
             tkey->tx_phase1_done = 1;
         }
-        tkip_mixing_phase2(rc4key, tkey->key, tkey->tx_ttak,
tkey->tx_iv16);
+        tkip_mixing_phase2(tkey->key, tkey->tx_ttak, tkey->tx_iv16,
rc4key);
     }
     else
     tkey->tx_phase1_done = 1;
@@ -447,4 +400,4 @@ static int ieee80211_tkip_decrypt(struct sk_buff
*skb, int hdr_len, void *priv)
             tkip_mixing_phase1(tkey->rx_ttak, tkey->key, hdr->addr2, iv32);
             tkey->rx_phase1_done = 1;
         }
-        tkip_mixing_phase2(rc4key, tkey->key, tkey->rx_ttak, iv16);
+        tkip_mixing_phase2(tkey->key, tkey->rx_ttak, iv16, rc4key);

        plen = skb->len - hdr_len - 12;

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index b4bef11..62934c3 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -4238,2 +4238,2 @@ void ieee80211_get_tkip_rx_p1k(struct
ieee80211_key_conf *keyconf,
 void ieee80211_get_tkip_p2k(struct ieee80211_key_conf *keyconf,
                 struct sk_buff *skb, u8 *p2k);

+void tkip_mixing_phase2(const u8 *tk, const u16 *p1k, u16 tsc_IV16,
+            u8 *rc4key);
+
 /**
  * ieee80211_aes_cmac_calculate_k1_k2 - calculate the AES-CMAC sub keys
  *
diff --git a/net/mac80211/tkip.c b/net/mac80211/tkip.c
index 0ae2077..a6fb85d3 100644
--- a/net/mac80211/tkip.c
+++ b/net/mac80211/tkip.c
@@ -105,2 +105,2 @@ static void tkip_mixing_phase1(const u8 *tk, struct
tkip_ctx *ctx,
     ctx->p1k_iv32 = tsc_IV32;
 }

-static void tkip_mixing_phase2(const u8 *tk, struct tkip_ctx *ctx,
-                   u16 tsc_IV16, u8 *rc4key)
+void tkip_mixing_phase2(const u8 *tk, const u16 *p1k, u16 tsc_IV16, u8
*rc4key)
 {
     u16 ppk[6];
-    const u16 *p1k = ctx->p1k;
     int i;

     ppk[0] = p1k[0];
@@ -138,3 +136,4 @@ static void tkip_mixing_phase2(const u8 *tk, struct
tkip_ctx *ctx,
     for (i = 0; i < 6; i++)
         put_unaligned_le16(ppk[i], rc4key + 2 * i);
 }
+EXPORT_SYMBOL(tkip_mixing_phase2);

 /* Add TKIP IV and Ext. IV at @pos. @iv0, @iv1, and @iv2 are the first
octets
  * of the IV. Returns pointer to the octet following IVs (i.e.,
beginning of
@@ -210,0 +209,0 @@ void ieee80211_get_tkip_p2k(struct
ieee80211_key_conf *keyconf,

     spin_lock(&key->u.tkip.txlock);
     ieee80211_compute_tkip_p1k(key, iv32);
-    tkip_mixing_phase2(tk, ctx, iv16, p2k);
+    tkip_mixing_phase2(tk, ctx->p1k, iv16, p2k);
     spin_unlock(&key->u.tkip.txlock);
 }
 EXPORT_SYMBOL(ieee80211_get_tkip_p2k);
@@ -295,2 +294,2 @@ int ieee80211_tkip_decrypt_data(struct crypto_cipher
*tfm,
         key->u.tkip.rx[queue].state = TKIP_STATE_PHASE1_HW_UPLOADED;
     }

-    tkip_mixing_phase2(tk, &key->u.tkip.rx[queue], iv16, rc4key);
+    tkip_mixing_phase2(tk, key->u.tkip.rx[queue].p1k, iv16, rc4key);

     res = ieee80211_wep_decrypt_data(tfm, rc4key, 16, pos, payload_len
- 12);
  done:
--
2.1.4

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Dan Carpenter]

On Wed, May 13, 2015 at 10:04:14PM -0300, Gaston Gonzalez wrote:
> @@ -327,7 +280,7 @@ static int ieee80211_tkip_encrypt(struct sk_buff
> *skb, int hdr_len, void *priv)
>                      tkey->tx_iv32);
>              tkey->tx_phase1_done = 1;
>          }
> -        tkip_mixing_phase2(rc4key, tkey->key, tkey->tx_ttak,
> tkey->tx_iv16);
> +        tkip_mixing_phase2(tkey->key, tkey->tx_ttak, tkey->tx_iv16,
> rc4key);
>      }
>      else
>      tkey->tx_phase1_done = 1;
> @@ -447,4 +400,4 @@ static int ieee80211_tkip_decrypt(struct sk_buff
> *skb, int hdr_len, void *priv)
>              tkip_mixing_phase1(tkey->rx_ttak, tkey->key, hdr->addr2, iv32);
>              tkey->rx_phase1_done = 1;
>          }
> -        tkip_mixing_phase2(rc4key, tkey->key, tkey->rx_ttak, iv16);
> +        tkip_mixing_phase2(tkey->key, tkey->rx_ttak, iv16, rc4key);
> 
>         plen = skb->len - hdr_len - 12;
> 

This patch got corrupted over email, so I can't review it inline, but it
looks much more reasonable.

regards,
dan carpenter

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Johannes Berg]

On Wed, 2015-05-13 at 22:04 -0300, Gaston Gonzalez wrote:

>  .../rtl8192u/ieee80211/ieee80211_crypt_tkip.c      | 53
> ++--------------------
>  include/net/mac80211.h                             |  3 ++
>  net/mac80211/tkip.c                                |  9 ++--
>  3 files changed, 10 insertions(+), 55 deletions(-)

That doesn't seem right to me. Clearly, that's a staging driver that
ships its own 802.11 subsystem, and bears no existing relation to
mac80211. Exporting a mac80211 internal function to make such a driver
happy makes me very suspicious. That function might not be very mac80211
specific, but exporting it from mac80211 doesn't seem right, that
introduces a massive and mostly artificial dependency into the driver.

Now arguably that driver is in staging and that doesn't matter, but then
why even bother cleaning this up? It seems likely that a well-written
driver for this would use mac80211, and not have to bother with this at
all since it could then use ieee80211_get_tkip_rx_p1k() or
ieee80211_get_tkip_p2k() or the update_tkip_key() method.

So I'm not fond of this patch and without a *very very* good reason and
explanation I'm not going to merge the mac80211 part. It certainly
bothers me much less that a crappy staging driver has a few lines of
duplicated code than it would to export mac80211 internals that real
mac80211 drivers don't need.

johannes

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Dan Carpenter]

Thanks Johannes.

I'm a total newbie to networking so I don't get offended when the
experts tell me I'm wrong.  :)  We'll look at the things you suggested.

regards,
dan carpenter

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Larry Finger]

On 05/14/2015 02:35 PM, Johannes Berg wrote:
> On Wed, 2015-05-13 at 22:04 -0300, Gaston Gonzalez wrote:
>
>>   .../rtl8192u/ieee80211/ieee80211_crypt_tkip.c      | 53
>> ++--------------------
>>   include/net/mac80211.h                             |  3 ++
>>   net/mac80211/tkip.c                                |  9 ++--
>>   3 files changed, 10 insertions(+), 55 deletions(-)
>
> That doesn't seem right to me. Clearly, that's a staging driver that
> ships its own 802.11 subsystem, and bears no existing relation to
> mac80211. Exporting a mac80211 internal function to make such a driver
> happy makes me very suspicious. That function might not be very mac80211
> specific, but exporting it from mac80211 doesn't seem right, that
> introduces a massive and mostly artificial dependency into the driver.
>
> Now arguably that driver is in staging and that doesn't matter, but then
> why even bother cleaning this up? It seems likely that a well-written
> driver for this would use mac80211, and not have to bother with this at
> all since it could then use ieee80211_get_tkip_rx_p1k() or
> ieee80211_get_tkip_p2k() or the update_tkip_key() method.
>
> So I'm not fond of this patch and without a *very very* good reason and
> explanation I'm not going to merge the mac80211 part. It certainly
> bothers me much less that a crappy staging driver has a few lines of
> duplicated code than it would to export mac80211 internals that real
> mac80211 drivers don't need.

I agree. No staging driver should ever require changes in any part of mac80211!

If you want to get credit for kernel patches, you are welcome to fuss with the 
code, but none of your changes should touch any part of the kernel other than 
the driver you are working on. That is an absolute requirement.

One other comment is that I have never seen this hardware. I wonder if anyone 
has it anymore.

Larry

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Gaston Gonzalez]

On 14/05/15 16:35, Johannes Berg wrote:
> On Wed, 2015-05-13 at 22:04 -0300, Gaston Gonzalez wrote:
>
>>  .../rtl8192u/ieee80211/ieee80211_crypt_tkip.c      | 53
>> ++--------------------
>>  include/net/mac80211.h                             |  3 ++
>>  net/mac80211/tkip.c                                |  9 ++--
>>  3 files changed, 10 insertions(+), 55 deletions(-)
> That doesn't seem right to me. Clearly, that's a staging driver that
> ships its own 802.11 subsystem, and bears no existing relation to
> mac80211. Exporting a mac80211 internal function to make such a driver
> happy makes me very suspicious. That function might not be very mac80211
> specific, but exporting it from mac80211 doesn't seem right, that
> introduces a massive and mostly artificial dependency into the driver.
>
> Now arguably that driver is in staging and that doesn't matter, but then
> why even bother cleaning this up? It seems likely that a well-written
> driver for this would use mac80211, and not have to bother with this at
> all since it could then use ieee80211_get_tkip_rx_p1k() or
> ieee80211_get_tkip_p2k() or the update_tkip_key() method.
>
> So I'm not fond of this patch and without a *very very* good reason and
> explanation I'm not going to merge the mac80211 part. It certainly
> bothers me much less that a crappy staging driver has a few lines of
> duplicated code than it would to export mac80211 internals that real
> mac80211 drivers don't need.
>
> johannes
>
Johannes,

If Dan is a newbie to this, I would be a pre-under-newbie or something
below that. That being said, understood your explication, I'll look for
another way to deal with this warning.
Thanks, and sorry for the noise.

Regards,

Gaston

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Johannes Berg]

On Thu, 2015-05-14 at 19:03 -0300, Gaston Gonzalez wrote:

> If Dan is a newbie to this, I would be a pre-under-newbie or something
> below that. That being said, understood your explication, I'll look for
> another way to deal with this warning.

I don't even see your original patch, so I have no idea what you're
trying to do :)

johannes

Re: [PATCH] staging: rtl8192u: ieee80211: Silence sparse warning

[Gaston Gonzalez]

On 15/05/15 04:26, Johannes Berg wrote:
> On Thu, 2015-05-14 at 19:03 -0300, Gaston Gonzalez wrote:
>
>> If Dan is a newbie to this, I would be a pre-under-newbie or something
>> below that. That being said, understood your explication, I'll look for
>> another way to deal with this warning.
> I don't even see your original patch, so I have no idea what you're
> trying to do :)
>
> johannes
>
The original patch started as sparse warning fix:

http://thread.gmane.org/gmane.linux.drivers.driver-project.devel/67254

regards,

Gaston