This patch enables power save processing for encrypted frames even if the
encryption key is not set. This is a requirement when implementing split-MAC
systems like Anyfi.net [1] and CAPWAP [2] on mac80211 using monitor frame
injection and reception. The mac80211 RX handlers are reordered slightly so
that the power save handler is invoked before the decryption handler.
The patch is minimal in the sense that it provides the required functionality
with a minimal change, but I am open to suggestions if this change is too
intrusive. Please let me know what you think.
[1] http://anyfi.net/documentation#architecture
[2] http://tools.ietf.org/html/rfc5416
Signed-off-by: Johan Almbladh <[email protected]>
---
net/mac80211/rx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 6b85f95..0f0017d 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2939,10 +2939,10 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx,
*/
rx->skb = skb;
- CALL_RXH(ieee80211_rx_h_decrypt)
CALL_RXH(ieee80211_rx_h_check_more_data)
CALL_RXH(ieee80211_rx_h_uapsd_and_pspoll)
CALL_RXH(ieee80211_rx_h_sta_process)
+ CALL_RXH(ieee80211_rx_h_decrypt)
CALL_RXH(ieee80211_rx_h_defragment)
CALL_RXH(ieee80211_rx_h_michael_mic_verify)
/* must be after MMIC verify so header is counted in MPDU mic */
--
1.7.9.5
On Mon, Aug 12, 2013 at 06:01:15PM +0200, Johannes Berg wrote:
> On Sun, 2013-08-11 at 22:19 +0200, Johan Almbladh wrote:
> > ieee80211_rx_h_sta_process:
> > The updating of last_rx for an IBSS STA is conditional on the STA
> > being AUTHORIZED. That state is the same regardless of whether the
> > updating is done before or after decryption.
>
> That's true, but my concern here was about the reboot problem:
> * two stations join a secure mesh and communicate
> * one of them reboots, but quickly rejoins the mesh
> In this case sometimes it gets hard to detect the situation and keeping
> the other station alive. OTOH, it already happens for beacon frames
> which aren't encrypted anyway, so I think it shouldn't matter. Antonio?
For the "reboot detection" mechanism only the beacon and the AUTH frames
are involved, therefore (since both are not encrypted) there should be no
problem.
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
This patch decouples the power save processing from the frame decryption
by running the decrypt rx handler after sta_process. In the case where
the decryption failed for some reason, the stack used to not process
the PM and MOREDATA bits for that frame. The stack now always performs
power save processing regardless of the decryption result. That means that
encrypted data frames and NULLFUNC frames are now handled in the same way
regarding power save processing, making the stack more robust.
Tested-by: Johan Almbladh <[email protected]>
Signed-off-by: Johan Almbladh <[email protected]>
---
net/mac80211/rx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 6b85f95..0f0017d 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2939,10 +2939,10 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx,
*/
rx->skb = skb;
- CALL_RXH(ieee80211_rx_h_decrypt)
CALL_RXH(ieee80211_rx_h_check_more_data)
CALL_RXH(ieee80211_rx_h_uapsd_and_pspoll)
CALL_RXH(ieee80211_rx_h_sta_process)
+ CALL_RXH(ieee80211_rx_h_decrypt)
CALL_RXH(ieee80211_rx_h_defragment)
CALL_RXH(ieee80211_rx_h_michael_mic_verify)
/* must be after MMIC verify so header is counted in MPDU mic */
--
1.7.9.5
Right, I got it.
Johan
On Thu, Aug 15, 2013 at 8:08 AM, Kalle Valo <[email protected]> wrote:
> Johan Almbladh <[email protected]> writes:
>
>> This patch decouples the power save processing from the frame decryption
>> by running the decrypt rx handler after sta_process. In the case where
>> the decryption failed for some reason, the stack used to not process
>> the PM and MOREDATA bits for that frame. The stack now always performs
>> power save processing regardless of the decryption result. That means that
>> encrypted data frames and NULLFUNC frames are now handled in the same way
>> regarding power save processing, making the stack more robust.
>>
>> Tested-by: Johan Almbladh <[email protected]>
>> Signed-off-by: Johan Almbladh <[email protected]>
>
> The idea of the Tested-by is that someone else than the patch author has
> also tested the patch, like a bug reporter etc. The patch author should
> always tests his/her code, so adding Tested-by for the author is moot.
>
> --
> Kalle Valo
On Wed, 2013-08-14 at 15:29 +0200, Johan Almbladh wrote:
> This patch changes the location of the rx handler functions to match
> the new order in which they are invoked.
Applied both, but I squashed them into one.
johannes
On Thu, 2013-08-08 at 15:21 +0200, Johan Almbladh wrote:
> This patch enables power save processing for encrypted frames even if the
> encryption key is not set. This is a requirement when implementing split-MAC
> systems like Anyfi.net [1] and CAPWAP [2] on mac80211 using monitor frame
> injection and reception.
I have no idea what these are, nor do I actually want to care much...
You presumably use Felix's active monitor mode?
> The mac80211 RX handlers are reordered slightly so
> that the power save handler is invoked before the decryption handler.
>
> The patch is minimal in the sense that it provides the required functionality
> with a minimal change, but I am open to suggestions if this change is too
> intrusive. Please let me know what you think.
I think you should ask yourself if this makes sense in the normal wifi
context... :-)
It actually seems like it *does* make sense, so it should have an
appropriate description for that, but I'm a bit worried about IBSS in
sta_process.
Also I've tried to keep the code in the file sequential, so this patch
should be moving ieee80211_rx_h_decrypt() itself as well.
johannes
This patch changes the location of the rx handler functions to match
the new order in which they are invoked.
Tested-by: Johan Almbladh <[email protected]>
Signed-off-by: Johan Almbladh <[email protected]>
---
net/mac80211/rx.c | 402 ++++++++++++++++++++++++++---------------------------
1 file changed, 201 insertions(+), 201 deletions(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 0f0017d..a84f319 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1055,207 +1055,6 @@ ieee80211_rx_h_check(struct ieee80211_rx_data *rx)
static ieee80211_rx_result debug_noinline
-ieee80211_rx_h_decrypt(struct ieee80211_rx_data *rx)
-{
- struct sk_buff *skb = rx->skb;
- struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
- struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
- int keyidx;
- int hdrlen;
- ieee80211_rx_result result = RX_DROP_UNUSABLE;
- struct ieee80211_key *sta_ptk = NULL;
- int mmie_keyidx = -1;
- __le16 fc;
-
- /*
- * Key selection 101
- *
- * There are four types of keys:
- * - GTK (group keys)
- * - IGTK (group keys for management frames)
- * - PTK (pairwise keys)
- * - STK (station-to-station pairwise keys)
- *
- * When selecting a key, we have to distinguish between multicast
- * (including broadcast) and unicast frames, the latter can only
- * use PTKs and STKs while the former always use GTKs and IGTKs.
- * Unless, of course, actual WEP keys ("pre-RSNA") are used, then
- * unicast frames can also use key indices like GTKs. Hence, if we
- * don't have a PTK/STK we check the key index for a WEP key.
- *
- * Note that in a regular BSS, multicast frames are sent by the
- * AP only, associated stations unicast the frame to the AP first
- * which then multicasts it on their behalf.
- *
- * There is also a slight problem in IBSS mode: GTKs are negotiated
- * with each station, that is something we don't currently handle.
- * The spec seems to expect that one negotiates the same key with
- * every station but there's no such requirement; VLANs could be
- * possible.
- */
-
- /*
- * No point in finding a key and decrypting if the frame is neither
- * addressed to us nor a multicast frame.
- */
- if (!(status->rx_flags & IEEE80211_RX_RA_MATCH))
- return RX_CONTINUE;
-
- /* start without a key */
- rx->key = NULL;
-
- if (rx->sta)
- sta_ptk = rcu_dereference(rx->sta->ptk);
-
- fc = hdr->frame_control;
-
- if (!ieee80211_has_protected(fc))
- mmie_keyidx = ieee80211_get_mmie_keyidx(rx->skb);
-
- if (!is_multicast_ether_addr(hdr->addr1) && sta_ptk) {
- rx->key = sta_ptk;
- 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;
- } else if (mmie_keyidx >= 0) {
- /* Broadcast/multicast robust management frame / BIP */
- if ((status->flag & RX_FLAG_DECRYPTED) &&
- (status->flag & RX_FLAG_IV_STRIPPED))
- return RX_CONTINUE;
-
- if (mmie_keyidx < NUM_DEFAULT_KEYS ||
- mmie_keyidx >= NUM_DEFAULT_KEYS + NUM_DEFAULT_MGMT_KEYS)
- return RX_DROP_MONITOR; /* unexpected BIP keyidx */
- if (rx->sta)
- rx->key = rcu_dereference(rx->sta->gtk[mmie_keyidx]);
- if (!rx->key)
- rx->key = rcu_dereference(rx->sdata->keys[mmie_keyidx]);
- } else if (!ieee80211_has_protected(fc)) {
- /*
- * The frame was not protected, so skip decryption. However, we
- * need to set rx->key if there is a key that could have been
- * used so that the frame may be dropped if encryption would
- * have been expected.
- */
- struct ieee80211_key *key = NULL;
- struct ieee80211_sub_if_data *sdata = rx->sdata;
- int i;
-
- if (ieee80211_is_mgmt(fc) &&
- is_multicast_ether_addr(hdr->addr1) &&
- (key = rcu_dereference(rx->sdata->default_mgmt_key)))
- rx->key = key;
- else {
- if (rx->sta) {
- for (i = 0; i < NUM_DEFAULT_KEYS; i++) {
- key = rcu_dereference(rx->sta->gtk[i]);
- if (key)
- break;
- }
- }
- if (!key) {
- for (i = 0; i < NUM_DEFAULT_KEYS; i++) {
- key = rcu_dereference(sdata->keys[i]);
- if (key)
- break;
- }
- }
- if (key)
- rx->key = key;
- }
- return RX_CONTINUE;
- } else {
- u8 keyid;
- /*
- * The device doesn't give us the IV so we won't be
- * able to look up the key. That's ok though, we
- * don't need to decrypt the frame, we just won't
- * be able to keep statistics accurate.
- * Except for key threshold notifications, should
- * we somehow allow the driver to tell us which key
- * the hardware used if this flag is set?
- */
- if ((status->flag & RX_FLAG_DECRYPTED) &&
- (status->flag & RX_FLAG_IV_STRIPPED))
- return RX_CONTINUE;
-
- hdrlen = ieee80211_hdrlen(fc);
-
- if (rx->skb->len < 8 + hdrlen)
- return RX_DROP_UNUSABLE; /* TODO: count this? */
-
- /*
- * no need to call ieee80211_wep_get_keyidx,
- * it verifies a bunch of things we've done already
- */
- skb_copy_bits(rx->skb, hdrlen + 3, &keyid, 1);
- keyidx = keyid >> 6;
-
- /* check per-station GTK first, if multicast packet */
- if (is_multicast_ether_addr(hdr->addr1) && rx->sta)
- rx->key = rcu_dereference(rx->sta->gtk[keyidx]);
-
- /* if not found, try default key */
- if (!rx->key) {
- rx->key = rcu_dereference(rx->sdata->keys[keyidx]);
-
- /*
- * RSNA-protected unicast frames should always be
- * sent with pairwise or station-to-station keys,
- * but for WEP we allow using a key index as well.
- */
- if (rx->key &&
- rx->key->conf.cipher != WLAN_CIPHER_SUITE_WEP40 &&
- rx->key->conf.cipher != WLAN_CIPHER_SUITE_WEP104 &&
- !is_multicast_ether_addr(hdr->addr1))
- rx->key = NULL;
- }
- }
-
- if (rx->key) {
- if (unlikely(rx->key->flags & KEY_FLAG_TAINTED))
- return RX_DROP_MONITOR;
-
- rx->key->tx_rx_count++;
- /* TODO: add threshold stuff again */
- } else {
- return RX_DROP_MONITOR;
- }
-
- switch (rx->key->conf.cipher) {
- case WLAN_CIPHER_SUITE_WEP40:
- case WLAN_CIPHER_SUITE_WEP104:
- result = ieee80211_crypto_wep_decrypt(rx);
- break;
- case WLAN_CIPHER_SUITE_TKIP:
- result = ieee80211_crypto_tkip_decrypt(rx);
- break;
- case WLAN_CIPHER_SUITE_CCMP:
- result = ieee80211_crypto_ccmp_decrypt(rx);
- break;
- case WLAN_CIPHER_SUITE_AES_CMAC:
- result = ieee80211_crypto_aes_cmac_decrypt(rx);
- break;
- default:
- /*
- * We can reach here only with HW-only algorithms
- * but why didn't it decrypt the frame?!
- */
- return RX_DROP_UNUSABLE;
- }
-
- /* the hdr variable is invalid after the decrypt handlers */
-
- /* either the frame has been decrypted or will be dropped */
- status->flag |= RX_FLAG_DECRYPTED;
-
- return result;
-}
-
-static ieee80211_rx_result debug_noinline
ieee80211_rx_h_check_more_data(struct ieee80211_rx_data *rx)
{
struct ieee80211_local *local;
@@ -1556,6 +1355,207 @@ ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
return RX_CONTINUE;
} /* ieee80211_rx_h_sta_process */
+static ieee80211_rx_result debug_noinline
+ieee80211_rx_h_decrypt(struct ieee80211_rx_data *rx)
+{
+ struct sk_buff *skb = rx->skb;
+ struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
+ struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+ int keyidx;
+ int hdrlen;
+ ieee80211_rx_result result = RX_DROP_UNUSABLE;
+ struct ieee80211_key *sta_ptk = NULL;
+ int mmie_keyidx = -1;
+ __le16 fc;
+
+ /*
+ * Key selection 101
+ *
+ * There are four types of keys:
+ * - GTK (group keys)
+ * - IGTK (group keys for management frames)
+ * - PTK (pairwise keys)
+ * - STK (station-to-station pairwise keys)
+ *
+ * When selecting a key, we have to distinguish between multicast
+ * (including broadcast) and unicast frames, the latter can only
+ * use PTKs and STKs while the former always use GTKs and IGTKs.
+ * Unless, of course, actual WEP keys ("pre-RSNA") are used, then
+ * unicast frames can also use key indices like GTKs. Hence, if we
+ * don't have a PTK/STK we check the key index for a WEP key.
+ *
+ * Note that in a regular BSS, multicast frames are sent by the
+ * AP only, associated stations unicast the frame to the AP first
+ * which then multicasts it on their behalf.
+ *
+ * There is also a slight problem in IBSS mode: GTKs are negotiated
+ * with each station, that is something we don't currently handle.
+ * The spec seems to expect that one negotiates the same key with
+ * every station but there's no such requirement; VLANs could be
+ * possible.
+ */
+
+ /*
+ * No point in finding a key and decrypting if the frame is neither
+ * addressed to us nor a multicast frame.
+ */
+ if (!(status->rx_flags & IEEE80211_RX_RA_MATCH))
+ return RX_CONTINUE;
+
+ /* start without a key */
+ rx->key = NULL;
+
+ if (rx->sta)
+ sta_ptk = rcu_dereference(rx->sta->ptk);
+
+ fc = hdr->frame_control;
+
+ if (!ieee80211_has_protected(fc))
+ mmie_keyidx = ieee80211_get_mmie_keyidx(rx->skb);
+
+ if (!is_multicast_ether_addr(hdr->addr1) && sta_ptk) {
+ rx->key = sta_ptk;
+ 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;
+ } else if (mmie_keyidx >= 0) {
+ /* Broadcast/multicast robust management frame / BIP */
+ if ((status->flag & RX_FLAG_DECRYPTED) &&
+ (status->flag & RX_FLAG_IV_STRIPPED))
+ return RX_CONTINUE;
+
+ if (mmie_keyidx < NUM_DEFAULT_KEYS ||
+ mmie_keyidx >= NUM_DEFAULT_KEYS + NUM_DEFAULT_MGMT_KEYS)
+ return RX_DROP_MONITOR; /* unexpected BIP keyidx */
+ if (rx->sta)
+ rx->key = rcu_dereference(rx->sta->gtk[mmie_keyidx]);
+ if (!rx->key)
+ rx->key = rcu_dereference(rx->sdata->keys[mmie_keyidx]);
+ } else if (!ieee80211_has_protected(fc)) {
+ /*
+ * The frame was not protected, so skip decryption. However, we
+ * need to set rx->key if there is a key that could have been
+ * used so that the frame may be dropped if encryption would
+ * have been expected.
+ */
+ struct ieee80211_key *key = NULL;
+ struct ieee80211_sub_if_data *sdata = rx->sdata;
+ int i;
+
+ if (ieee80211_is_mgmt(fc) &&
+ is_multicast_ether_addr(hdr->addr1) &&
+ (key = rcu_dereference(rx->sdata->default_mgmt_key)))
+ rx->key = key;
+ else {
+ if (rx->sta) {
+ for (i = 0; i < NUM_DEFAULT_KEYS; i++) {
+ key = rcu_dereference(rx->sta->gtk[i]);
+ if (key)
+ break;
+ }
+ }
+ if (!key) {
+ for (i = 0; i < NUM_DEFAULT_KEYS; i++) {
+ key = rcu_dereference(sdata->keys[i]);
+ if (key)
+ break;
+ }
+ }
+ if (key)
+ rx->key = key;
+ }
+ return RX_CONTINUE;
+ } else {
+ u8 keyid;
+ /*
+ * The device doesn't give us the IV so we won't be
+ * able to look up the key. That's ok though, we
+ * don't need to decrypt the frame, we just won't
+ * be able to keep statistics accurate.
+ * Except for key threshold notifications, should
+ * we somehow allow the driver to tell us which key
+ * the hardware used if this flag is set?
+ */
+ if ((status->flag & RX_FLAG_DECRYPTED) &&
+ (status->flag & RX_FLAG_IV_STRIPPED))
+ return RX_CONTINUE;
+
+ hdrlen = ieee80211_hdrlen(fc);
+
+ if (rx->skb->len < 8 + hdrlen)
+ return RX_DROP_UNUSABLE; /* TODO: count this? */
+
+ /*
+ * no need to call ieee80211_wep_get_keyidx,
+ * it verifies a bunch of things we've done already
+ */
+ skb_copy_bits(rx->skb, hdrlen + 3, &keyid, 1);
+ keyidx = keyid >> 6;
+
+ /* check per-station GTK first, if multicast packet */
+ if (is_multicast_ether_addr(hdr->addr1) && rx->sta)
+ rx->key = rcu_dereference(rx->sta->gtk[keyidx]);
+
+ /* if not found, try default key */
+ if (!rx->key) {
+ rx->key = rcu_dereference(rx->sdata->keys[keyidx]);
+
+ /*
+ * RSNA-protected unicast frames should always be
+ * sent with pairwise or station-to-station keys,
+ * but for WEP we allow using a key index as well.
+ */
+ if (rx->key &&
+ rx->key->conf.cipher != WLAN_CIPHER_SUITE_WEP40 &&
+ rx->key->conf.cipher != WLAN_CIPHER_SUITE_WEP104 &&
+ !is_multicast_ether_addr(hdr->addr1))
+ rx->key = NULL;
+ }
+ }
+
+ if (rx->key) {
+ if (unlikely(rx->key->flags & KEY_FLAG_TAINTED))
+ return RX_DROP_MONITOR;
+
+ rx->key->tx_rx_count++;
+ /* TODO: add threshold stuff again */
+ } else {
+ return RX_DROP_MONITOR;
+ }
+
+ switch (rx->key->conf.cipher) {
+ case WLAN_CIPHER_SUITE_WEP40:
+ case WLAN_CIPHER_SUITE_WEP104:
+ result = ieee80211_crypto_wep_decrypt(rx);
+ break;
+ case WLAN_CIPHER_SUITE_TKIP:
+ result = ieee80211_crypto_tkip_decrypt(rx);
+ break;
+ case WLAN_CIPHER_SUITE_CCMP:
+ result = ieee80211_crypto_ccmp_decrypt(rx);
+ break;
+ case WLAN_CIPHER_SUITE_AES_CMAC:
+ result = ieee80211_crypto_aes_cmac_decrypt(rx);
+ break;
+ default:
+ /*
+ * We can reach here only with HW-only algorithms
+ * but why didn't it decrypt the frame?!
+ */
+ return RX_DROP_UNUSABLE;
+ }
+
+ /* the hdr variable is invalid after the decrypt handlers */
+
+ /* either the frame has been decrypted or will be dropped */
+ status->flag |= RX_FLAG_DECRYPTED;
+
+ return result;
+}
+
static inline struct ieee80211_fragment_entry *
ieee80211_reassemble_add(struct ieee80211_sub_if_data *sdata,
unsigned int frag, unsigned int seq, int rx_queue,
--
1.7.9.5
I would prefer my original solution that puts the decryption handler
after the sta_process handler. The code is cleaner since we avoid the
extra flag and the coupling between decrypt and sta_process. My
conclusion is that the change is correct, see below.
ieee80211_rx_h_check_more_data, ieee80211_rx_h_uapsd_and_pspoll:
The MOREDATA and PM bits are not protected by the encryption MIC. It
should be valid to process those bits regardless of the decryption
outcome.
ieee80211_rx_h_sta_process:
The updating of last_rx for an IBSS STA is conditional on the STA
being AUTHORIZED. That state is the same regardless of whether the
updating is done before or after decryption.
The main argument that the MOREDATA and PM bits are not protected by
the encryption and are therefore independent. You can always send a
spoofed NULLFUNC frame to an RSN AP or STA and have the PM and
MOREDATA bits processed accordingly. My change allows data frames that
could not be decrypted to be processed similar to NULLFUNC frames.
I have run mac80211 in STA and AP mode with this change for quite some
time now without any problems. I will run it in IBSS mode also as you
suggest.
Provided that my IBSS tests pass, should I send you a final patch that
changes the RX handler order and their locations in the file?
Johan
On Sat, Aug 10, 2013 at 1:31 PM, Johan Almbladh <[email protected]> wrote:
> On Fri, Aug 9, 2013 at 3:07 PM, Johannes Berg <[email protected]> wrote:
>>
>> On Thu, 2013-08-08 at 15:21 +0200, Johan Almbladh wrote:
>> > This patch enables power save processing for encrypted frames even if the
>> > encryption key is not set. This is a requirement when implementing split-MAC
>> > systems like Anyfi.net [1] and CAPWAP [2] on mac80211 using monitor frame
>> > injection and reception.
>>
>> I have no idea what these are, nor do I actually want to care much...
>
> Anyfi.net is a dynamic split-mac system where the security part of the
> 802.11 stack is located in your home router and the realtime part is
> handled by any router or AP that happens to be near your current
> location. The two parts are connected dynamically via a UDP tunnel
> that carries raw encrypted 802.11 frames, forming a complete 802.11
> stack that provides your home Wi-Fi wherever you are. In a community
> Wi-Fi deployment, the users get secure Wi-Fi access with automatic
> sign-on via their home Wi-Fi network which is simply available
> everywhere.
>
> Should you find the concept interesting there is quite extensive
> technical documentation at http://anyfi.net/documentation#architecture
> :-)
>
>
>> You presumably use Felix's active monitor mode?
>
> I use a monitor socket in a userspace daemon. The daemon receives
> encrypted 802.11 frames with radiotap encapsulation on this monitor
> socket. It also injects encrypted 802.11 frames with radiotap
> encapsulation by transmitting them on the same socket. I believe this
> is the way hostapd used to handle transmission of management and EAPOL
> frames before they switched to nl80211.
>
>
>> > The mac80211 RX handlers are reordered slightly so
>> > that the power save handler is invoked before the decryption handler.
>> >
>> > The patch is minimal in the sense that it provides the required functionality
>> > with a minimal change, but I am open to suggestions if this change is too
>> > intrusive. Please let me know what you think.
>>
>> I think you should ask yourself if this makes sense in the normal wifi
>> context... :-)
>
> You are right about that, but I think this little feature can be added
> without affecting the normal operation :-) To be honest, mac80211 has
> all the interfaces required for any split-mac implementation, thanks
> to the mac80211/hostapd partitioning. The *only* thing missing is the
> ability to handle AP power save processing without handling the
> encryption...
>
>
>> It actually seems like it *does* make sense, so it should have an
>> appropriate description for that, but I'm a bit worried about IBSS in
>> sta_process.
>
> The patch enables power save processing even if there is no unicast
> key set, but *also* if key is set but the decryption failed. This is
> what I meant with "intrusive". The IBSS updating in sta_process will
> also run in this case, but the STA is still required to be authorized.
>
> It's possible to narrow it down to only affect the case where no
> encryption key is set:
>
> * Keep the RX handlers in their original order
> * Don't drop frames where rx->key is NULL in ieee80211_rx_h_decrypt.
> Instead, mark the frame with a flag
> * Drop any marked frames at the end of ieee80211_rx_h_sta_process with
> RX_DROP_MONITOR
>
> I can prepare a new patch if you prefer this solution.
>
>> Also I've tried to keep the code in the file sequential, so this patch
>> should be moving ieee80211_rx_h_decrypt() itself as well.
>
> I'll make sure to put them in the right order.
>
> Johan
On Sun, 2013-08-11 at 22:19 +0200, Johan Almbladh wrote:
> I would prefer my original solution that puts the decryption handler
> after the sta_process handler. The code is cleaner since we avoid the
> extra flag and the coupling between decrypt and sta_process. My
> conclusion is that the change is correct, see below.
I actually agree and think that it makes our implementation of 802.11
more robust, but I think that should be said in the changelog and be
carefully analysed (as I guess you've done below)
> ieee80211_rx_h_check_more_data, ieee80211_rx_h_uapsd_and_pspoll:
> The MOREDATA and PM bits are not protected by the encryption MIC. It
> should be valid to process those bits regardless of the decryption
> outcome.
Agree, those seem pretty clear.
> ieee80211_rx_h_sta_process:
> The updating of last_rx for an IBSS STA is conditional on the STA
> being AUTHORIZED. That state is the same regardless of whether the
> updating is done before or after decryption.
That's true, but my concern here was about the reboot problem:
* two stations join a secure mesh and communicate
* one of them reboots, but quickly rejoins the mesh
In this case sometimes it gets hard to detect the situation and keeping
the other station alive. OTOH, it already happens for beacon frames
which aren't encrypted anyway, so I think it shouldn't matter. Antonio?
> The main argument that the MOREDATA and PM bits are not protected by
> the encryption and are therefore independent. You can always send a
> spoofed NULLFUNC frame to an RSN AP or STA and have the PM and
> MOREDATA bits processed accordingly. My change allows data frames that
> could not be decrypted to be processed similar to NULLFUNC frames.
Right, of course.
> I have run mac80211 in STA and AP mode with this change for quite some
> time now without any problems. I will run it in IBSS mode also as you
> suggest.
>
> Provided that my IBSS tests pass, should I send you a final patch that
> changes the RX handler order and their locations in the file?
I think the IBSS test would be rather difficult, but yes, I think you've
convinced me that the patch is fine - please do resend it.
johannes
Johan Almbladh <[email protected]> writes:
> This patch decouples the power save processing from the frame decryption
> by running the decrypt rx handler after sta_process. In the case where
> the decryption failed for some reason, the stack used to not process
> the PM and MOREDATA bits for that frame. The stack now always performs
> power save processing regardless of the decryption result. That means that
> encrypted data frames and NULLFUNC frames are now handled in the same way
> regarding power save processing, making the stack more robust.
>
> Tested-by: Johan Almbladh <[email protected]>
> Signed-off-by: Johan Almbladh <[email protected]>
The idea of the Tested-by is that someone else than the patch author has
also tested the patch, like a bug reporter etc. The patch author should
always tests his/her code, so adding Tested-by for the author is moot.
--
Kalle Valo
On Fri, Aug 9, 2013 at 3:07 PM, Johannes Berg <[email protected]> wrote:
>
> On Thu, 2013-08-08 at 15:21 +0200, Johan Almbladh wrote:
> > This patch enables power save processing for encrypted frames even if the
> > encryption key is not set. This is a requirement when implementing split-MAC
> > systems like Anyfi.net [1] and CAPWAP [2] on mac80211 using monitor frame
> > injection and reception.
>
> I have no idea what these are, nor do I actually want to care much...
Anyfi.net is a dynamic split-mac system where the security part of the
802.11 stack is located in your home router and the realtime part is
handled by any router or AP that happens to be near your current
location. The two parts are connected dynamically via a UDP tunnel
that carries raw encrypted 802.11 frames, forming a complete 802.11
stack that provides your home Wi-Fi wherever you are. In a community
Wi-Fi deployment, the users get secure Wi-Fi access with automatic
sign-on via their home Wi-Fi network which is simply available
everywhere.
Should you find the concept interesting there is quite extensive
technical documentation at http://anyfi.net/documentation#architecture
:-)
> You presumably use Felix's active monitor mode?
I use a monitor socket in a userspace daemon. The daemon receives
encrypted 802.11 frames with radiotap encapsulation on this monitor
socket. It also injects encrypted 802.11 frames with radiotap
encapsulation by transmitting them on the same socket. I believe this
is the way hostapd used to handle transmission of management and EAPOL
frames before they switched to nl80211.
> > The mac80211 RX handlers are reordered slightly so
> > that the power save handler is invoked before the decryption handler.
> >
> > The patch is minimal in the sense that it provides the required functionality
> > with a minimal change, but I am open to suggestions if this change is too
> > intrusive. Please let me know what you think.
>
> I think you should ask yourself if this makes sense in the normal wifi
> context... :-)
You are right about that, but I think this little feature can be added
without affecting the normal operation :-) To be honest, mac80211 has
all the interfaces required for any split-mac implementation, thanks
to the mac80211/hostapd partitioning. The *only* thing missing is the
ability to handle AP power save processing without handling the
encryption...
> It actually seems like it *does* make sense, so it should have an
> appropriate description for that, but I'm a bit worried about IBSS in
> sta_process.
The patch enables power save processing even if there is no unicast
key set, but *also* if key is set but the decryption failed. This is
what I meant with "intrusive". The IBSS updating in sta_process will
also run in this case, but the STA is still required to be authorized.
It's possible to narrow it down to only affect the case where no
encryption key is set:
* Keep the RX handlers in their original order
* Don't drop frames where rx->key is NULL in ieee80211_rx_h_decrypt.
Instead, mark the frame with a flag
* Drop any marked frames at the end of ieee80211_rx_h_sta_process with
RX_DROP_MONITOR
I can prepare a new patch if you prefer this solution.
> Also I've tried to keep the code in the file sequential, so this patch
> should be moving ieee80211_rx_h_decrypt() itself as well.
I'll make sure to put them in the right order.
Johan