2010-08-10 07:51:03

by Johannes Berg

[permalink] [raw]
Subject: [PATCH 3/5] mac80211: remove unused status flag checks

From: Johannes Berg <[email protected]>

The decryption code verifies whether or not
a given frame was decrypted and verified by
hardware. This is unnecessary, as the crypto
RX handler already does it long before the
decryption code is even invoked, so remove
that code to avoid confusion.

Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/wpa.c | 26 ++++++--------------------
1 file changed, 6 insertions(+), 20 deletions(-)

--- wireless-testing.orig/net/mac80211/wpa.c 2010-08-08 10:38:47.000000000 +0200
+++ wireless-testing/net/mac80211/wpa.c 2010-08-08 10:40:03.000000000 +0200
@@ -221,19 +221,13 @@ ieee80211_crypto_tkip_decrypt(struct iee
if (!rx->sta || skb->len - hdrlen < 12)
return RX_DROP_UNUSABLE;

- if (status->flag & RX_FLAG_DECRYPTED) {
- if (status->flag & RX_FLAG_IV_STRIPPED) {
- /*
- * Hardware took care of all processing, including
- * replay protection, and stripped the ICV/IV so
- * we cannot do any checks here.
- */
- return RX_CONTINUE;
- }
-
- /* let TKIP code verify IV, but skip decryption */
+ /*
+ * Let TKIP code verify IV, but skip decryption.
+ * In the case where hardware checks the IV as well,
+ * we don't even get here, see ieee80211_rx_h_decrypt()
+ */
+ if (status->flag & RX_FLAG_DECRYPTED)
hwaccel = 1;
- }

res = ieee80211_tkip_decrypt_data(rx->local->wep_rx_tfm,
key, skb->data + hdrlen,
@@ -447,10 +441,6 @@ ieee80211_crypto_ccmp_decrypt(struct iee
if (!rx->sta || data_len < 0)
return RX_DROP_UNUSABLE;

- if ((status->flag & RX_FLAG_DECRYPTED) &&
- (status->flag & RX_FLAG_IV_STRIPPED))
- return RX_CONTINUE;
-
ccmp_hdr2pn(pn, skb->data + hdrlen);

queue = ieee80211_is_mgmt(hdr->frame_control) ?
@@ -564,10 +554,6 @@ ieee80211_crypto_aes_cmac_decrypt(struct
if (!ieee80211_is_mgmt(hdr->frame_control))
return RX_CONTINUE;

- if ((status->flag & RX_FLAG_DECRYPTED) &&
- (status->flag & RX_FLAG_IV_STRIPPED))
- return RX_CONTINUE;
-
if (skb->len < 24 + sizeof(*mmie))
return RX_DROP_UNUSABLE;





2010-08-12 04:58:02

by Juuso Oikarinen

[permalink] [raw]
Subject: Re: [PATCH 3/5] mac80211: remove unused status flag checks

On Wed, 2010-08-11 at 14:39 +0200, ext Johannes Berg wrote:
> On Wed, 2010-08-11 at 15:12 +0300, Juuso Oikarinen wrote:
>
> > > > The decryption code verifies whether or not
> > > > a given frame was decrypted and verified by
> > > > hardware. This is unnecessary, as the crypto
> > > > RX handler already does it long before the
> > > > decryption code is even invoked, so remove
> > > > that code to avoid confusion.
> > > >
> > > > Signed-off-by: Johannes Berg <[email protected]>
> > > > ---
> > > > net/mac80211/wpa.c | 26 ++++++--------------------
> > > > 1 file changed, 6 insertions(+), 20 deletions(-)
> > > >
> > >
> > > This patch for some reason seems to break wl1271 WPA - association
> > > succeeds but encrypted data transfer fails.
> > >
> > > I still don't know why, but I'm looking into it.
> > >
> >
> > It appears, that in function ieee80211_rx_h_decrypt we go here:
> >
> > if (!is_multicast_ether_addr(hdr->addr1) && stakey) {
> > rx->key = stakey;
> > /* Skip decryption if the frame is not protected. */
> > if (!ieee80211_has_protected(hdr->frame_control))
> > return RX_CONTINUE;
> >
> > And here, as the frame is protected, we go out of the if, and end up in
> > tkip_decrypt, which with this patch no longer checks whether the frame
> > is already decrypted.
> >
> > The frame then ends up dropped.
>
> Err, you're right, sorry about that. There are too many paths here. How
> about this patch?
>
> johannes
>
> --- wireless-testing.orig/net/mac80211/rx.c 2010-08-11 14:37:13.000000000 +0200
> +++ wireless-testing/net/mac80211/rx.c 2010-08-11 14:38:13.000000000 +0200
> @@ -873,6 +873,9 @@ ieee80211_rx_h_decrypt(struct ieee80211_
>
> if (!is_multicast_ether_addr(hdr->addr1) && stakey) {
> rx->key = stakey;
> + if ((status->flag & RX_FLAG_DECRYPTED) &&
> + (status->flag & RX_FLAG_IV_STRIPPED))
> + return RX_CONTINUE;
> /* Skip decryption if the frame is not protected. */
> if (!ieee80211_has_protected(fc))
> return RX_CONTINUE;
>
>

This appears to work.

-Juus


2010-08-11 12:13:54

by Juuso Oikarinen

[permalink] [raw]
Subject: Re: [PATCH 3/5] mac80211: remove unused status flag checks

On Wed, 2010-08-11 at 13:51 +0200, Oikarinen Juuso (Nokia-MS/Tampere)
wrote:
> On Tue, 2010-08-10 at 09:46 +0200, ext Johannes Berg wrote:
> > From: Johannes Berg <[email protected]>
> >
> > The decryption code verifies whether or not
> > a given frame was decrypted and verified by
> > hardware. This is unnecessary, as the crypto
> > RX handler already does it long before the
> > decryption code is even invoked, so remove
> > that code to avoid confusion.
> >
> > Signed-off-by: Johannes Berg <[email protected]>
> > ---
> > net/mac80211/wpa.c | 26 ++++++--------------------
> > 1 file changed, 6 insertions(+), 20 deletions(-)
> >
>
> This patch for some reason seems to break wl1271 WPA - association
> succeeds but encrypted data transfer fails.
>
> I still don't know why, but I'm looking into it.
>

It appears, that in function ieee80211_rx_h_decrypt we go here:

if (!is_multicast_ether_addr(hdr->addr1) && stakey) {
rx->key = stakey;
/* Skip decryption if the frame is not protected. */
if (!ieee80211_has_protected(hdr->frame_control))
return RX_CONTINUE;

And here, as the frame is protected, we go out of the if, and end up in
tkip_decrypt, which with this patch no longer checks whether the frame
is already decrypted.

The frame then ends up dropped.


> -Juuso
>
> --
> 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



2010-08-11 12:39:21

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 3/5] mac80211: remove unused status flag checks

On Wed, 2010-08-11 at 15:12 +0300, Juuso Oikarinen wrote:

> > > The decryption code verifies whether or not
> > > a given frame was decrypted and verified by
> > > hardware. This is unnecessary, as the crypto
> > > RX handler already does it long before the
> > > decryption code is even invoked, so remove
> > > that code to avoid confusion.
> > >
> > > Signed-off-by: Johannes Berg <[email protected]>
> > > ---
> > > net/mac80211/wpa.c | 26 ++++++--------------------
> > > 1 file changed, 6 insertions(+), 20 deletions(-)
> > >
> >
> > This patch for some reason seems to break wl1271 WPA - association
> > succeeds but encrypted data transfer fails.
> >
> > I still don't know why, but I'm looking into it.
> >
>
> It appears, that in function ieee80211_rx_h_decrypt we go here:
>
> if (!is_multicast_ether_addr(hdr->addr1) && stakey) {
> rx->key = stakey;
> /* Skip decryption if the frame is not protected. */
> if (!ieee80211_has_protected(hdr->frame_control))
> return RX_CONTINUE;
>
> And here, as the frame is protected, we go out of the if, and end up in
> tkip_decrypt, which with this patch no longer checks whether the frame
> is already decrypted.
>
> The frame then ends up dropped.

Err, you're right, sorry about that. There are too many paths here. How
about this patch?

johannes

--- wireless-testing.orig/net/mac80211/rx.c 2010-08-11 14:37:13.000000000 +0200
+++ wireless-testing/net/mac80211/rx.c 2010-08-11 14:38:13.000000000 +0200
@@ -873,6 +873,9 @@ ieee80211_rx_h_decrypt(struct ieee80211_

if (!is_multicast_ether_addr(hdr->addr1) && stakey) {
rx->key = stakey;
+ if ((status->flag & RX_FLAG_DECRYPTED) &&
+ (status->flag & RX_FLAG_IV_STRIPPED))
+ return RX_CONTINUE;
/* Skip decryption if the frame is not protected. */
if (!ieee80211_has_protected(fc))
return RX_CONTINUE;



2010-08-11 11:52:15

by Juuso Oikarinen

[permalink] [raw]
Subject: Re: [PATCH 3/5] mac80211: remove unused status flag checks

On Tue, 2010-08-10 at 09:46 +0200, ext Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> The decryption code verifies whether or not
> a given frame was decrypted and verified by
> hardware. This is unnecessary, as the crypto
> RX handler already does it long before the
> decryption code is even invoked, so remove
> that code to avoid confusion.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> net/mac80211/wpa.c | 26 ++++++--------------------
> 1 file changed, 6 insertions(+), 20 deletions(-)
>

This patch for some reason seems to break wl1271 WPA - association
succeeds but encrypted data transfer fails.

I still don't know why, but I'm looking into it.

-Juuso