Implement Broadcast/Multicast Integrity Protocol for management frame
protection. This patch adds the needed definitions for the new
information element (MMIE) and implementation for the new "encryption"
type (though, BIP is actually not encrypting data, it provides only
integrity protection). These routines will be used by a follow-on patch
that enables BIP for multicast/broadcast robust management frames.
Signed-off-by: Jouni Malinen <[email protected]>
Index: wireless-testing/include/linux/ieee80211.h
===================================================================
--- wireless-testing.orig/include/linux/ieee80211.h
+++ wireless-testing/include/linux/ieee80211.h
@@ -590,6 +590,15 @@ struct ieee80211_mgmt {
} __attribute__ ((packed));
+/* Management MIC information element (IEEE 802.11w) */
+struct ieee80211_mmie {
+ u8 element_id;
+ u8 length;
+ u8 key_id[2]; /* little endian, but may be unaligned */
+ u8 sequence_number[6];
+ u8 mic[8];
+} __attribute__ ((packed));
+
/* Control frames */
struct ieee80211_rts {
__le16 frame_control;
@@ -860,6 +869,7 @@ enum ieee80211_eid {
WLAN_EID_HT_EXTRA_INFO = 61,
/* 802.11i */
WLAN_EID_RSN = 48,
+ WLAN_EID_MMIE = 76 /* 802.11w */,
WLAN_EID_WPA = 221,
WLAN_EID_GENERIC = 221,
WLAN_EID_VENDOR_SPECIFIC = 221,
Index: wireless-testing/net/mac80211/key.h
===================================================================
--- wireless-testing.orig/net/mac80211/key.h
+++ wireless-testing/net/mac80211/key.h
@@ -109,6 +109,16 @@ struct ieee80211_key {
u8 tx_crypto_buf[6 * AES_BLOCK_LEN];
u8 rx_crypto_buf[6 * AES_BLOCK_LEN];
} ccmp;
+ struct {
+ u8 tx_pn[6];
+ u8 rx_pn[6];
+ struct crypto_cipher *tfm;
+ u32 replays; /* dot11RSNAStatsCMACReplays */
+ u32 icverrors; /* dot11RSNAStatsCMACICVErrors */
+ /* scratch buffers for virt_to_page() (crypto API) */
+ u8 tx_crypto_buf[2 * AES_BLOCK_LEN];
+ u8 rx_crypto_buf[2 * AES_BLOCK_LEN];
+ } aes_cmac;
} u;
/* number of times this key has been used */
Index: wireless-testing/net/mac80211/wpa.c
===================================================================
--- wireless-testing.orig/net/mac80211/wpa.c
+++ wireless-testing/net/mac80211/wpa.c
@@ -1,5 +1,6 @@
/*
* Copyright 2002-2004, Instant802 Networks, Inc.
+ * Copyright 2008, Jouni Malinen <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -11,12 +12,14 @@
#include <linux/slab.h>
#include <linux/skbuff.h>
#include <linux/compiler.h>
+#include <asm/unaligned.h>
#include <net/mac80211.h>
#include "ieee80211_i.h"
#include "michael.h"
#include "tkip.h"
#include "aes_ccm.h"
+#include "aes_cmac.h"
#include "wpa.h"
static int ieee80211_get_hdr_info(const struct sk_buff *skb, u8 **sa, u8 **da,
@@ -558,3 +561,129 @@ ieee80211_crypto_ccmp_decrypt(struct iee
return RX_CONTINUE;
}
+
+
+static void bip_aad(struct sk_buff *skb, u8 *aad)
+{
+ /* BIP AAD: FC(masked) || A1 || A2 || A3 */
+
+ /* FC type/subtype */
+ aad[0] = skb->data[0];
+ /* Mask FC Retry, PwrMgt, MoreData flags to zero */
+ aad[1] = skb->data[1] & ~(BIT(4) | BIT(5) | BIT(6));
+ /* A1 || A2 || A3 */
+ memcpy(aad + 2, skb->data + 4, 3 * ETH_ALEN);
+}
+
+
+static inline void bip_ipn_swap(u8 *d, const u8 *s)
+{
+ *d++ = s[5];
+ *d++ = s[4];
+ *d++ = s[3];
+ *d++ = s[2];
+ *d++ = s[1];
+ *d = s[0];
+}
+
+
+ieee80211_tx_result
+ieee80211_crypto_aes_cmac_encrypt(struct ieee80211_tx_data *tx)
+{
+ struct sk_buff *skb = tx->skb;
+ struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+ struct ieee80211_key *key = tx->key;
+ struct ieee80211_mmie *mmie;
+ u8 *pn, aad[20];
+ int i;
+
+ if ((tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)) {
+ /* hwaccel */
+ info->control.hw_key = &tx->key->conf;
+ return 0;
+ }
+
+ if (skb_tailroom(skb) < sizeof(*mmie)) {
+ if (pskb_expand_head(skb, skb_headroom(skb),
+ skb_tailroom(skb) + sizeof((*mmie)),
+ GFP_ATOMIC) < 0)
+ return TX_DROP;
+ }
+
+ mmie = (struct ieee80211_mmie *) skb_put(skb, sizeof(*mmie));
+ mmie->element_id = WLAN_EID_MMIE;
+ mmie->length = sizeof(*mmie) - 2;
+ put_unaligned_le16(key->conf.keyidx, mmie->key_id);
+
+ /* PN = PN + 1 */
+ pn = key->u.aes_cmac.tx_pn;
+
+ for (i = sizeof(key->u.aes_cmac.tx_pn) - 1; i >= 0; i--) {
+ pn[i]++;
+ if (pn[i])
+ break;
+ }
+ bip_ipn_swap(mmie->sequence_number, pn);
+
+ bip_aad(skb, aad);
+
+ /*
+ * MIC = AES-128-CMAC(IGTK, AAD || Management Frame Body || MMIE, 64)
+ */
+ ieee80211_aes_cmac(key->u.aes_cmac.tfm, key->u.aes_cmac.tx_crypto_buf,
+ aad, skb->data + 24, skb->len - 24, mmie->mic);
+
+ return TX_CONTINUE;
+}
+
+
+ieee80211_rx_result
+ieee80211_crypto_aes_cmac_decrypt(struct ieee80211_rx_data *rx)
+{
+ struct sk_buff *skb = rx->skb;
+ struct ieee80211_key *key = rx->key;
+ struct ieee80211_mmie *mmie;
+ u8 aad[20], mic[8], ipn[6];
+
+ if ((rx->fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_MGMT)
+ return RX_CONTINUE;
+
+ if ((rx->status->flag & RX_FLAG_DECRYPTED) &&
+ (rx->status->flag & RX_FLAG_IV_STRIPPED))
+ return RX_CONTINUE;
+
+ if (skb->len < 24 + sizeof(*mmie))
+ return RX_DROP_UNUSABLE;
+
+ mmie = (struct ieee80211_mmie *)
+ (skb->data + skb->len - sizeof(*mmie));
+ if (mmie->element_id != WLAN_EID_MMIE ||
+ mmie->length != sizeof(*mmie) - 2)
+ return RX_DROP_UNUSABLE; /* Invalid MMIE */
+
+ bip_ipn_swap(ipn, mmie->sequence_number);
+
+ if (memcmp(ipn, key->u.aes_cmac.rx_pn, 6) <= 0) {
+ key->u.aes_cmac.replays++;
+ return RX_DROP_UNUSABLE;
+ }
+
+ if (!(rx->status->flag & RX_FLAG_DECRYPTED)) {
+ /* hardware didn't decrypt/verify MIC */
+ bip_aad(skb, aad);
+ ieee80211_aes_cmac(key->u.aes_cmac.tfm,
+ key->u.aes_cmac.rx_crypto_buf, aad,
+ skb->data + 24, skb->len - 24, mic);
+ if (memcmp(mic, mmie->mic, sizeof(mmie->mic)) != 0) {
+ key->u.aes_cmac.icverrors++;
+ return RX_DROP_UNUSABLE;
+ }
+ }
+
+ memcpy(key->u.aes_cmac.rx_pn, ipn, 6);
+
+ /* Remove MMIE */
+ skb_trim(skb, skb->len - sizeof(*mmie));
+
+ return RX_CONTINUE;
+}
Index: wireless-testing/net/mac80211/wpa.h
===================================================================
--- wireless-testing.orig/net/mac80211/wpa.h
+++ wireless-testing/net/mac80211/wpa.h
@@ -28,4 +28,9 @@ ieee80211_crypto_ccmp_encrypt(struct iee
ieee80211_rx_result
ieee80211_crypto_ccmp_decrypt(struct ieee80211_rx_data *rx);
+ieee80211_tx_result
+ieee80211_crypto_aes_cmac_encrypt(struct ieee80211_tx_data *tx);
+ieee80211_rx_result
+ieee80211_crypto_aes_cmac_decrypt(struct ieee80211_rx_data *rx);
+
#endif /* WPA_H */
Index: wireless-testing/net/mac80211/aes_cmac.c
===================================================================
--- /dev/null
+++ wireless-testing/net/mac80211/aes_cmac.c
@@ -0,0 +1,135 @@
+/*
+ * AES-128-CMAC with TLen 16 for IEEE 802.11w BIP
+ * Copyright 2008, Jouni Malinen <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/crypto.h>
+#include <linux/err.h>
+
+#include <net/mac80211.h>
+#include "key.h"
+#include "aes_cmac.h"
+
+#define AES_BLOCK_SIZE 16
+#define AES_CMAC_KEY_LEN 16
+#define CMAC_TLEN 8 /* CMAC TLen = 64 bits (8 octets) */
+#define AAD_LEN 20
+
+
+static void gf_mulx(u8 *pad)
+{
+ int i, carry;
+
+ carry = pad[0] & 0x80;
+ for (i = 0; i < AES_BLOCK_SIZE - 1; i++)
+ pad[i] = (pad[i] << 1) | (pad[i + 1] >> 7);
+ pad[AES_BLOCK_SIZE - 1] <<= 1;
+ if (carry)
+ pad[AES_BLOCK_SIZE - 1] ^= 0x87;
+}
+
+
+static void aes_128_cmac_vector(struct crypto_cipher *tfm, u8 *scratch,
+ size_t num_elem,
+ const u8 *addr[], const size_t *len, u8 *mac)
+{
+ u8 *cbc, *pad;
+ const u8 *pos, *end;
+ size_t i, e, left, total_len;
+
+ cbc = scratch;
+ pad = scratch + AES_BLOCK_SIZE;
+
+ memset(cbc, 0, AES_BLOCK_SIZE);
+
+ total_len = 0;
+ for (e = 0; e < num_elem; e++)
+ total_len += len[e];
+ left = total_len;
+
+ e = 0;
+ pos = addr[0];
+ end = pos + len[0];
+
+ while (left >= AES_BLOCK_SIZE) {
+ for (i = 0; i < AES_BLOCK_SIZE; i++) {
+ cbc[i] ^= *pos++;
+ if (pos >= end) {
+ e++;
+ pos = addr[e];
+ end = pos + len[e];
+ }
+ }
+ if (left > AES_BLOCK_SIZE)
+ crypto_cipher_encrypt_one(tfm, cbc, cbc);
+ left -= AES_BLOCK_SIZE;
+ }
+
+ memset(pad, 0, AES_BLOCK_SIZE);
+ crypto_cipher_encrypt_one(tfm, pad, pad);
+ gf_mulx(pad);
+
+ if (left || total_len == 0) {
+ for (i = 0; i < left; i++) {
+ cbc[i] ^= *pos++;
+ if (pos >= end) {
+ e++;
+ pos = addr[e];
+ end = pos + len[e];
+ }
+ }
+ cbc[left] ^= 0x80;
+ gf_mulx(pad);
+ }
+
+ for (i = 0; i < AES_BLOCK_SIZE; i++)
+ pad[i] ^= cbc[i];
+ crypto_cipher_encrypt_one(tfm, pad, pad);
+ memcpy(mac, pad, CMAC_TLEN);
+}
+
+
+void ieee80211_aes_cmac(struct crypto_cipher *tfm, u8 *scratch, const u8 *aad,
+ const u8 *data, size_t data_len, u8 *mic)
+{
+ const u8 *addr[3];
+ size_t len[3];
+ u8 zero[CMAC_TLEN];
+
+ memset(zero, 0, CMAC_TLEN);
+ addr[0] = aad;
+ len[0] = AAD_LEN;
+ addr[1] = data;
+ len[1] = data_len - CMAC_TLEN;
+ addr[2] = zero;
+ len[2] = CMAC_TLEN;
+
+ aes_128_cmac_vector(tfm, scratch, 3, addr, len, mic);
+}
+
+
+struct crypto_cipher * ieee80211_aes_cmac_key_setup(const u8 key[])
+{
+ struct crypto_cipher *tfm;
+
+ tfm = crypto_alloc_cipher("aes", 0, CRYPTO_ALG_ASYNC);
+ if (IS_ERR(tfm))
+ return NULL;
+
+ crypto_cipher_setkey(tfm, key, AES_CMAC_KEY_LEN);
+
+ return tfm;
+}
+
+
+void ieee80211_aes_cmac_key_free(struct crypto_cipher *tfm)
+{
+ if (tfm)
+ crypto_free_cipher(tfm);
+}
Index: wireless-testing/net/mac80211/aes_cmac.h
===================================================================
--- /dev/null
+++ wireless-testing/net/mac80211/aes_cmac.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright 2008, Jouni Malinen <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef AES_CMAC_H
+#define AES_CMAC_H
+
+#include <linux/crypto.h>
+
+struct crypto_cipher * ieee80211_aes_cmac_key_setup(const u8 key[]);
+void ieee80211_aes_cmac(struct crypto_cipher *tfm, u8 *scratch, const u8 *aad,
+ const u8 *data, size_t data_len, u8 *mic);
+void ieee80211_aes_cmac_key_free(struct crypto_cipher *tfm);
+
+#endif /* AES_CMAC_H */
Index: wireless-testing/net/mac80211/Makefile
===================================================================
--- wireless-testing.orig/net/mac80211/Makefile
+++ wireless-testing/net/mac80211/Makefile
@@ -21,6 +21,7 @@ mac80211-y := \
michael.o \
tkip.o \
aes_ccm.o \
+ aes_cmac.o \
cfg.o \
rx.o \
tx.o \
--
--
Jouni Malinen PGP id EFC895FA
> +/* Management MIC information element (IEEE 802.11w) */
> +struct ieee80211_mmie {
> + u8 element_id;
> + u8 length;
> + u8 key_id[2]; /* little endian, but may be unaligned */
Since you say the struct is packed you should be able to use __le16 just
fine.
> + if ((tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)) {
I think one set of parentheses suffices ;)
> + if (skb_tailroom(skb) < sizeof(*mmie)) {
> + if (pskb_expand_head(skb, skb_headroom(skb),
> + skb_tailroom(skb) + sizeof((*mmie)),
> + GFP_ATOMIC) < 0)
> + return TX_DROP;
> + }
I tried ensure pskb_expand_head is only called at most once when the
frame is handed to master_start_xmit to avoid problems with skb truesize
and such. Could you add the necessary space at that point already,
possibly simply reserving max(mmic-len, mmie-len) or so instead of the
current mmic-len (I think)? I'd hate to add back calls to
pskb_expand_head at other places, and it's only 18 bytes so not really
huge.
> + if ((rx->fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_MGMT)
> + return RX_CONTINUE;
Harvey just added a bunch of helper inlines to include/linux/ieee80211.h
for stuff like that, I think you could use one of them here.
> + mmie = (struct ieee80211_mmie *)
> + (skb->data + skb->len - sizeof(*mmie));
> + if (mmie->element_id != WLAN_EID_MMIE ||
> + mmie->length != sizeof(*mmie) - 2)
> + return RX_DROP_UNUSABLE; /* Invalid MMIE */
Is that what the draft says? Because iterating the IEs would be
different, this means you could potentially have something like a vendor
IE last that encapsulates the MMIE including type/len fields, which
should probably not be used?
> + /* Remove MMIE */
> + skb_trim(skb, skb->len - sizeof(*mmie));
Is that actually necessary? Since it's an IE, it should be ignored by
all other code, no? Not that it matters though.
johannes
> > Yeah, true, and we actually have that in another place too. If we then
> > remove the MMIE, the IE sanity checks should catch the bad frame anyway,
> > when/if it is parsed. Except we removed those because APs were sending
> > bogus information. I'm fine with this, but we should be aware of the
> > consequence.
>
> As long as we get the RX path implemented properly, this will only hit
> if there is a bug in an MFP-enabled AP or someone is trying to attack
> the network and both cases are very good candidates for dropping the
> frame anyway. The key selection is supposed to pick BIP key only if the
> sender (AP) has negotiated MFP and as such, all valid broadcast robust
> management frames are guaranteed to have MMIE in the end.
True. I was more thinking of somebody intentionally doing it in the AP
to implement "802.11w in vendor IEs" or something like that but I guess
that's unlikely to happen. And yeah, an attack won't work anyway since
those frames would be rejected based on the wrong MIC.
johannes
> Aah.. That's a copy-paste from something old.. I think I removed the
> IEEE80211_KEY_FLAG_GENERATE_IV flag from here for BIP.. I'm not
> completely sure yet, how we should have this, i.e., whether that flag
> should be used here or not. It is unclear what type of hwaccel, if any,
> vendors are going to implement for BIP.
Yeah, good point, I guess we should just not implement hwaccel until we
find hardware that can handle it.
> > I tried ensure pskb_expand_head is only called at most once when the
> > frame is handed to master_start_xmit to avoid problems with skb truesize
> > and such. Could you add the necessary space at that point already,
> > possibly simply reserving max(mmic-len, mmie-len) or so instead of the
> > current mmic-len (I think)? I'd hate to add back calls to
> > pskb_expand_head at other places, and it's only 18 bytes so not really
> > huge.
>
> I thought about that for a moment, but then decided that it would be
> okay to just do this here since MMIE is larger than any other tailroom
> we have and there is next to no real use for multicast/broadcast
> management frames, so there is no need to optimize this. I don't see how
> this would require any other place to use pskb_expand_head.
Well the thing is that you can't just call pskb_expand_head without
orphaning the SKB first and all that truesize adjusting, because of
truesize accounting, because it might now or later belong to a userspace
socket.
> Anyway, I would assume it would be possible to do this by changing
> IEEE80211_ENCRYPT_TAILROOM from 12 to 22 which would waste 10 bytes
> extra for every frame (well, not waste for BIP-protected frames but
> there are next to none of them).
22? Is there something else on the frame in addition to the MMIE?
> I converted some of the places to use the new helpers, but did not go
> through all places. I would assume there is still places that can be
> made to use them, but as long as FC is available in host byte order, it
> is easier and faster to use it. Sure, if rx->fc were to disappear, that
> may make it more likely that these get changed ;-).
Heh, ok, that's fine, I didn't realise that it was using the CPU order
FC.
> > Is that what the draft says? Because iterating the IEs would be
> > different, this means you could potentially have something like a vendor
> > IE last that encapsulates the MMIE including type/len fields, which
> > should probably not be used?
>
> MMIE has to be the last "IE" in the frame. Sure, it would, in theory, be
> possible to receive a frame that does not have MMIE, but has "MMIE like"
> data in another IE. As long as we can make sure that this is reached
> only for frames that are received from MFP enabled AP, the simple
> solution of pointing to the end of the frame is enough. Otherwise, we
> would need to parse all the IEs and know about the start of IEs in all
> different types of Action frames.. That's not really something I would
> like to do.
Yeah, true, and we actually have that in another place too. If we then
remove the MMIE, the IE sanity checks should catch the bad frame anyway,
when/if it is parsed. Except we removed those because APs were sending
bogus information. I'm fine with this, but we should be aware of the
consequence.
> > > + /* Remove MMIE */
> > > + skb_trim(skb, skb->len - sizeof(*mmie));
> >
> > Is that actually necessary? Since it's an IE, it should be ignored by
> > all other code, no? Not that it matters though.
>
> I don't think it is necessary in the sense that leaving the "IE" in
> would break anything. However, I do think this is the correct thing to
> do here and matches with what we do with TKIP/CCMP. MMIE is not really
> an IE, it is just a frame component that is made to look like one. The
> current IEEE 802.11w draft is bit confused about what MMIE is at places,
> but anyway, I would compare it to TKIP ICV and CCMP MIC in the end of
> the frames.
Works for me.
johannes
On Tuesday 17 June 2008 20:06:10 Jouni Malinen wrote:
> > Since you say the struct is packed you should be able to use __le16 just
> > fine.
>
> Is that guaranteed to force all accesses of the field to not use 16-bit
> read/write?
Yes.
--
Greetings Michael.
On Tue, 2008-06-17 at 18:55 +0200, Johannes Berg wrote:
> > + if ((rx->fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_MGMT)
> > + return RX_CONTINUE;
>
> Harvey just added a bunch of helper inlines to include/linux/ieee80211.h
> for stuff like that, I think you could use one of them here.
Afraid not, rx->fc is a u16. The helpers are all for __le16.
Harvey
On Tue, Jun 17, 2008 at 06:55:14PM +0200, Johannes Berg wrote:
> > +/* Management MIC information element (IEEE 802.11w) */
> > +struct ieee80211_mmie {
> > + u8 element_id;
> > + u8 length;
> > + u8 key_id[2]; /* little endian, but may be unaligned */
>
> Since you say the struct is packed you should be able to use __le16 just
> fine.
Is that guaranteed to force all accesses of the field to not use 16-bit
read/write?
> > + if ((tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)) {
>
> I think one set of parentheses suffices ;)
Aah.. That's a copy-paste from something old.. I think I removed the
IEEE80211_KEY_FLAG_GENERATE_IV flag from here for BIP.. I'm not
completely sure yet, how we should have this, i.e., whether that flag
should be used here or not. It is unclear what type of hwaccel, if any,
vendors are going to implement for BIP.
> > + if (skb_tailroom(skb) < sizeof(*mmie)) {
> > + if (pskb_expand_head(skb, skb_headroom(skb),
> > + skb_tailroom(skb) + sizeof((*mmie)),
> > + GFP_ATOMIC) < 0)
> > + return TX_DROP;
> > + }
>
> I tried ensure pskb_expand_head is only called at most once when the
> frame is handed to master_start_xmit to avoid problems with skb truesize
> and such. Could you add the necessary space at that point already,
> possibly simply reserving max(mmic-len, mmie-len) or so instead of the
> current mmic-len (I think)? I'd hate to add back calls to
> pskb_expand_head at other places, and it's only 18 bytes so not really
> huge.
I thought about that for a moment, but then decided that it would be
okay to just do this here since MMIE is larger than any other tailroom
we have and there is next to no real use for multicast/broadcast
management frames, so there is no need to optimize this. I don't see how
this would require any other place to use pskb_expand_head.
Anyway, I would assume it would be possible to do this by changing
IEEE80211_ENCRYPT_TAILROOM from 12 to 22 which would waste 10 bytes
extra for every frame (well, not waste for BIP-protected frames but
there are next to none of them).
> > + if ((rx->fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_MGMT)
> > + return RX_CONTINUE;
>
> Harvey just added a bunch of helper inlines to include/linux/ieee80211.h
> for stuff like that, I think you could use one of them here.
I converted some of the places to use the new helpers, but did not go
through all places. I would assume there is still places that can be
made to use them, but as long as FC is available in host byte order, it
is easier and faster to use it. Sure, if rx->fc were to disappear, that
may make it more likely that these get changed ;-).
> > + mmie = (struct ieee80211_mmie *)
> > + (skb->data + skb->len - sizeof(*mmie));
> > + if (mmie->element_id != WLAN_EID_MMIE ||
> > + mmie->length != sizeof(*mmie) - 2)
> > + return RX_DROP_UNUSABLE; /* Invalid MMIE */
>
> Is that what the draft says? Because iterating the IEs would be
> different, this means you could potentially have something like a vendor
> IE last that encapsulates the MMIE including type/len fields, which
> should probably not be used?
MMIE has to be the last "IE" in the frame. Sure, it would, in theory, be
possible to receive a frame that does not have MMIE, but has "MMIE like"
data in another IE. As long as we can make sure that this is reached
only for frames that are received from MFP enabled AP, the simple
solution of pointing to the end of the frame is enough. Otherwise, we
would need to parse all the IEs and know about the start of IEs in all
different types of Action frames.. That's not really something I would
like to do.
> > + /* Remove MMIE */
> > + skb_trim(skb, skb->len - sizeof(*mmie));
>
> Is that actually necessary? Since it's an IE, it should be ignored by
> all other code, no? Not that it matters though.
I don't think it is necessary in the sense that leaving the "IE" in
would break anything. However, I do think this is the correct thing to
do here and matches with what we do with TKIP/CCMP. MMIE is not really
an IE, it is just a frame component that is made to look like one. The
current IEEE 802.11w draft is bit confused about what MMIE is at places,
but anyway, I would compare it to TKIP ICV and CCMP MIC in the end of
the frames.
--
Jouni Malinen PGP id EFC895FA
On Tue, Jun 17, 2008 at 08:19:13PM +0200, Johannes Berg wrote:
> Well the thing is that you can't just call pskb_expand_head without
> orphaning the SKB first and all that truesize adjusting, because of
> truesize accounting, because it might now or later belong to a userspace
> socket.
OK. I don't want to do that here, so I'll see what can be done with
IEEE80211_ENCRYPT_TAILROOM.
> > Anyway, I would assume it would be possible to do this by changing
> > IEEE80211_ENCRYPT_TAILROOM from 12 to 22 which would waste 10 bytes
> > extra for every frame (well, not waste for BIP-protected frames but
> > there are next to none of them).
>
> 22? Is there something else on the frame in addition to the MMIE?
Uhm.. Maybe I just cannot count anymore.. Or well, I did not remember
where the 12 comes from and decided to add 4 because of that. Anyway,
yes, now that I see that 12 is 8(MIC)+4(ICV) for TKIP, this 12 would
indeed change to 18.
> Yeah, true, and we actually have that in another place too. If we then
> remove the MMIE, the IE sanity checks should catch the bad frame anyway,
> when/if it is parsed. Except we removed those because APs were sending
> bogus information. I'm fine with this, but we should be aware of the
> consequence.
As long as we get the RX path implemented properly, this will only hit
if there is a bug in an MFP-enabled AP or someone is trying to attack
the network and both cases are very good candidates for dropping the
frame anyway. The key selection is supposed to pick BIP key only if the
sender (AP) has negotiated MFP and as such, all valid broadcast robust
management frames are guaranteed to have MMIE in the end.
--
Jouni Malinen PGP id EFC895FA