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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Gaston,

A couple of minor style comments:

On Wed, May 13, 2015 at 8:23 AM, Gaston Gonzalez <gascoar@xxxxxxxxx> 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@xxxxxxxxx
Profile: http://www.google.com/profiles/julian.calaby/
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux