2010-10-10 16:52:10

by Johannes Berg

Subject: [PATCH] mac80211: don't kmalloc 16 bytes

From: Johannes Berg <[email protected]>

Since this small buffer isn't used for DMA,
we can simply allocate it on the stack, it
just needs to be 16 bytes of which only 8
will be used for WEP40 keys.

Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/wep.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)

--- wireless-testing.orig/net/mac80211/wep.c 2010-10-08 14:50:35.000000000 +0200
+++ wireless-testing/net/mac80211/wep.c 2010-10-08 14:51:41.000000000 +0200
@@ -222,7 +222,7 @@ static int ieee80211_wep_decrypt(struct
struct ieee80211_key *key)
{
u32 klen;
- u8 *rc4key;
+ u8 rc4key[3 + WLAN_KEY_LEN_WEP104];
u8 keyidx;
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
unsigned int hdrlen;
@@ -245,10 +245,6 @@ static int ieee80211_wep_decrypt(struct

klen = 3 + key->conf.keylen;

- rc4key = kmalloc(klen, GFP_ATOMIC);
- if (!rc4key)
- return -1;
-
/* Prepend 24-bit IV to RC4 key */
memcpy(rc4key, skb->data + hdrlen, 3);

@@ -260,8 +256,6 @@ static int ieee80211_wep_decrypt(struct
len))
ret = -1;

- kfree(rc4key);
-
/* Trim ICV */
skb_trim(skb, skb->len - WEP_ICV_LEN);

2010-10-10 17:50:35

by Michael Büsch

Subject: Re: [PATCH] mac80211: don't kmalloc 16 bytes

On Sun, 2010-10-10 at 18:52 +0200, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> Since this small buffer isn't used for DMA,
> we can simply allocate it on the stack, it
> just needs to be 16 bytes of which only 8
> will be used for WEP40 keys.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> net/mac80211/wep.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> --- wireless-testing.orig/net/mac80211/wep.c 2010-10-08 14:50:35.000000000 +0200
> +++ wireless-testing/net/mac80211/wep.c 2010-10-08 14:51:41.000000000 +0200
> @@ -222,7 +222,7 @@ static int ieee80211_wep_decrypt(struct
> struct ieee80211_key *key)
> {
> u32 klen;
> - u8 *rc4key;
> + u8 rc4key[3 + WLAN_KEY_LEN_WEP104];
> u8 keyidx;
> struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> unsigned int hdrlen;
> @@ -245,10 +245,6 @@ static int ieee80211_wep_decrypt(struct
>
> klen = 3 + key->conf.keylen;

What about
if (WARN_ON(klen > sizeof(rc4key)))
return -1;
to harden this a bit for accidental stack overflows?

>
> - rc4key = kmalloc(klen, GFP_ATOMIC);
> - if (!rc4key)
> - return -1;
> -
> /* Prepend 24-bit IV to RC4 key */
> memcpy(rc4key, skb->data + hdrlen, 3);
>
> @@ -260,8 +256,6 @@ static int ieee80211_wep_decrypt(struct
> len))
> ret = -1;
>
> - kfree(rc4key);
> -
> /* Trim ICV */
> skb_trim(skb, skb->len - WEP_ICV_LEN);
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>


--
Greetings Michael.

2010-10-10 17:52:28

by Johannes Berg

Subject: Re: [PATCH] mac80211: don't kmalloc 16 bytes

On Sun, 2010-10-10 at 19:50 +0200, Michael Büsch wrote:
> On Sun, 2010-10-10 at 18:52 +0200, Johannes Berg wrote:
> > From: Johannes Berg <[email protected]>
> >
> > Since this small buffer isn't used for DMA,
> > we can simply allocate it on the stack, it
> > just needs to be 16 bytes of which only 8
> > will be used for WEP40 keys.
> >
> > Signed-off-by: Johannes Berg <[email protected]>
> > ---
> > net/mac80211/wep.c | 8 +-------
> > 1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > --- wireless-testing.orig/net/mac80211/wep.c 2010-10-08 14:50:35.000000000 +0200
> > +++ wireless-testing/net/mac80211/wep.c 2010-10-08 14:51:41.000000000 +0200
> > @@ -222,7 +222,7 @@ static int ieee80211_wep_decrypt(struct
> > struct ieee80211_key *key)
> > {
> > u32 klen;
> > - u8 *rc4key;
> > + u8 rc4key[3 + WLAN_KEY_LEN_WEP104];
> > u8 keyidx;
> > struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> > unsigned int hdrlen;
> > @@ -245,10 +245,6 @@ static int ieee80211_wep_decrypt(struct
> >
> > klen = 3 + key->conf.keylen;
>
> What about
> if (WARN_ON(klen > sizeof(rc4key)))
> return -1;
> to harden this a bit for accidental stack overflows?

Not sure, it doesn't really seem worth it -- if somebody really wanted
to extend mac80211 to support WEP256 he'd also have to find and change
the other places that already contain similar code. Not that it's really
a hotpath though (since 11n doesn't support WEP).

johannes