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:
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
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
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
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
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
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
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
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
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
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
Hi Gaston,
A couple of minor style comments:
On Wed, May 13, 2015 at 8:23 AM, Gaston Gonzalez <[email protected]> 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: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/