| Message ID | 5553F48E.3050008@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;