* Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.
2012-07-04 13:00 ` Nicolas Cavallari
@ 2012-07-04 13:29 ` Johannes Berg
2012-07-04 16:10 ` [PATCH 1/2] mac80211: restructure key selection Nicolas Cavallari
2012-07-04 13:45 ` [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas Nicolas Cavallari
2012-07-04 17:44 ` Jouni Malinen
2 siblings, 1 reply; 12+ messages in thread
From: Johannes Berg @ 2012-07-04 13:29 UTC (permalink / raw)
To: Nicolas Cavallari; +Cc: John W. Linville, linux-wireless, Jouni Malinen
On Wed, 2012-07-04 at 15:00 +0200, Nicolas Cavallari wrote:
> > + else if (ieee80211_is_robust_mgmt_frame(hdr) &&
> > + (!ieee80211_is_action(hdr->frame_control) ||
> > + !tx->sta || !test_sta_flag(tx->sta, WLAN_STA_MFP)))
> > + tx->key = NULL;
>
> I would have written that more like
>
> is_robust_mgmt && !(is_action && tx->sta && test_sta_flag(mfp))
>
> So it's more clear that we drop action frames to mfp stas, as per the spec.
Yeah, makes sense. I was translating it directly.
> > + else {
> > +
> > I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
> > return TX_DROP;
>
> is that blank line needed ?
Err, no :-)
> >> Do you want to apply it yourself ? or should i include it in a series ?
> >
> > Then ... probably easiest for you to include it so you can work on top.
> > Here's a version with commit log:
> > http://p.sipsolutions.net/17ea0ebece544a59.txt
> >
>
> Ok. will send a series with this patch modified once i test it and my
> new patch version.
Great, thanks.
> >>> The whole drop_unencrypted seems strange to start with though...
> >>
> >> what to you find strange ? that it is not used in a ESS, or how it works
> >> ? or both ?
> >
> > The way it's used/works/isn't used.
>
> From what i understand, data frames to unauthorized stations are dropped
> well before selecting the encryption key. as stations are authorized
> after or at the same time their encryption key are set, it somewhat
> works. But for MFP, i'm not sure. I think that drop_unencrypted is
> mistaken for "drop_unencrypted_management" there.
> But i'm not an MFP expert.
>
> I'm not sure if we should just add a separate
> drop_unencrypted_management, or just replace drop_unencrypted with
> drop_unencrypted_management. But in a IBSS with RSN, if wpa_supplicant
> isn't recent enough, stations are always authorized by default. so
> drop_encrypted is required in this case.
Ok ...
I guess I'll have to let Jouni comment on this, right now I'm not much
less confused than before :-)
johannes
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] mac80211: restructure key selection
2012-07-04 13:29 ` Johannes Berg
@ 2012-07-04 16:10 ` Nicolas Cavallari
2012-07-04 16:10 ` [PATCHv3 2/2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas Nicolas Cavallari
2012-07-10 16:07 ` [PATCH 1/2] mac80211: restructure key selection Johannes Berg
0 siblings, 2 replies; 12+ messages in thread
From: Nicolas Cavallari @ 2012-07-04 16:10 UTC (permalink / raw)
To: John W. Linville, Johannes Berg; +Cc: linux-wireless
From: Johannes Berg <johannes.berg@intel.com>
The "no key" case in key selection that decides
whether to drop the frame or not is impossible
to understand, restructure the code.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
[cavallar@lri.fr: removed blank line and restructured action frame clause]
Signed-off-by: Nicolas Cavallari <cavallar@lri.fr>
---
net/mac80211/tx.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 4990f4f..cdd4ac6 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -523,7 +523,7 @@ ieee80211_tx_h_check_control_port_protocol(struct ieee80211_tx_data *tx)
static ieee80211_tx_result debug_noinline
ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
{
- struct ieee80211_key *key = NULL;
+ struct ieee80211_key *key;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
@@ -542,16 +542,20 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
else if (!is_multicast_ether_addr(hdr->addr1) &&
(key = rcu_dereference(tx->sdata->default_unicast_key)))
tx->key = key;
- else if (tx->sdata->drop_unencrypted &&
- (tx->skb->protocol != tx->sdata->control_port_protocol) &&
- !(info->flags & IEEE80211_TX_CTL_INJECTED) &&
- (!ieee80211_is_robust_mgmt_frame(hdr) ||
- (ieee80211_is_action(hdr->frame_control) &&
- tx->sta && test_sta_flag(tx->sta, WLAN_STA_MFP)))) {
+ else if (info->flags & IEEE80211_TX_CTL_INJECTED)
+ tx->key = NULL;
+ else if (!tx->sdata->drop_unencrypted)
+ tx->key = NULL;
+ else if (tx->skb->protocol == tx->sdata->control_port_protocol)
+ tx->key = NULL;
+ else if (ieee80211_is_robust_mgmt_frame(hdr) &&
+ !(ieee80211_is_action(hdr->frame_control) &&
+ tx->sta && test_sta_flag(tx->sta, WLAN_STA_MFP)))
+ tx->key = NULL;
+ else {
I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
return TX_DROP;
- } else
- tx->key = NULL;
+ }
if (tx->key) {
bool skip_hw = false;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCHv3 2/2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.
2012-07-04 16:10 ` [PATCH 1/2] mac80211: restructure key selection Nicolas Cavallari
@ 2012-07-04 16:10 ` Nicolas Cavallari
2012-07-10 16:07 ` [PATCH 1/2] mac80211: restructure key selection Johannes Berg
1 sibling, 0 replies; 12+ messages in thread
From: Nicolas Cavallari @ 2012-07-04 16:10 UTC (permalink / raw)
To: John W. Linville, Johannes Berg; +Cc: linux-wireless
When drop_unencrypted is enabled and MFP is disabled,
non-robust management frames for not-yet associated STA are dropped.
This isn't visible as many management frames sent from the kernel
have TX_INTFL_DONT_ENCRYPT set and management frames injected
from a monitor vif have TX_CTL_INJECTED so aren't dropped.
But management frames sent from userspace via NL80211_CMD_FRAME
do not have this flag set, so are dropped.
This patch make it always accept non-robust management frames.
Signed-off-by: Nicolas Cavallari <cavallar@lri.fr>
---
changes :
v2->v3 : now depends on johannes's cleanup, even clearer code.
v1->v2 : thinier, clearer, more correct code. migrated from RFC to PATCH.
---
net/mac80211/tx.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index cdd4ac6..691aa6c 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -552,6 +552,9 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
!(ieee80211_is_action(hdr->frame_control) &&
tx->sta && test_sta_flag(tx->sta, WLAN_STA_MFP)))
tx->key = NULL;
+ else if (ieee80211_is_mgmt(hdr->frame_control) &&
+ !ieee80211_is_robust_mgmt_frame(hdr))
+ tx->key = NULL;
else {
I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
return TX_DROP;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/2] mac80211: restructure key selection
2012-07-04 16:10 ` [PATCH 1/2] mac80211: restructure key selection Nicolas Cavallari
2012-07-04 16:10 ` [PATCHv3 2/2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas Nicolas Cavallari
@ 2012-07-10 16:07 ` Johannes Berg
1 sibling, 0 replies; 12+ messages in thread
From: Johannes Berg @ 2012-07-10 16:07 UTC (permalink / raw)
To: Nicolas Cavallari; +Cc: John W. Linville, linux-wireless
Applied both. I think we need to figure out more of the story with
drop_unencrypted and why we say we don't want to encrypt BA action
frames?!
johannes
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.
2012-07-04 13:00 ` Nicolas Cavallari
2012-07-04 13:29 ` Johannes Berg
@ 2012-07-04 13:45 ` Nicolas Cavallari
2012-07-04 17:44 ` Jouni Malinen
2 siblings, 0 replies; 12+ messages in thread
From: Nicolas Cavallari @ 2012-07-04 13:45 UTC (permalink / raw)
To: Johannes Berg; +Cc: John W. Linville, linux-wireless, Jouni Malinen
On 04/07/2012 15:00, Nicolas Cavallari wrote:
But in a IBSS with RSN, if wpa_supplicant
> isn't recent enough, stations are always authorized by default. so
> drop_encrypted is required in this case.
drop_unencrypted, my bad.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.
2012-07-04 13:00 ` Nicolas Cavallari
2012-07-04 13:29 ` Johannes Berg
2012-07-04 13:45 ` [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas Nicolas Cavallari
@ 2012-07-04 17:44 ` Jouni Malinen
2012-07-05 7:55 ` Johannes Berg
2 siblings, 1 reply; 12+ messages in thread
From: Jouni Malinen @ 2012-07-04 17:44 UTC (permalink / raw)
To: Nicolas Cavallari; +Cc: Johannes Berg, John W. Linville, linux-wireless
On Wed, Jul 04, 2012 at 03:00:03PM +0200, Nicolas Cavallari wrote:
> From what i understand, data frames to unauthorized stations are dropped
> well before selecting the encryption key. as stations are authorized
> after or at the same time their encryption key are set, it somewhat
> works. But for MFP, i'm not sure. I think that drop_unencrypted is
> mistaken for "drop_unencrypted_management" there.
> But i'm not an MFP expert.
drop_unencrypted was originally (i.e., way before MFP) added as an extra
protection for some corner cases where keys may not have been set. In
theory, the PAE (authorized vs. unauthorized) should have covered those
cases, but there were some multi-SSID AP cases that were not obviously
clear. Consequently, it felt safer to add an extra protection for BSSes
that are known to use encryption for data frames.
As far as MFP is concerned, we have the WLAN_STA_MFP flag that should be
more reliable way of determining whether robust management frames have
to be protected.
> I'm not sure if we should just add a separate
> drop_unencrypted_management, or just replace drop_unencrypted with
> drop_unencrypted_management.
I hope that neither of those would be needed.
> But in a IBSS with RSN, if wpa_supplicant
> isn't recent enough, stations are always authorized by default. so
> drop_encrypted is required in this case.
For a BSS that uses RSN, we could maintain a new flag that indicates
that (non-nullfunc) Data frames are not to be transmitted or received
without protected. Though, this would be quite similar to
drop_unencrypted in practice.
As far as the new patch is concerned, it would look like this is
extending the fix in commit e0463f501fb945c1fde536d98eefc5ba156ff497.
The commit log for that change seems to claim that the goal was to avoid
dropping any management frames to a STA that does not use MFP, but the
change does not seem to do that.
As far as drop_unencrypted not being used in AP/managed mode is
concerned, that sounds like an additional bug.. This code is supposed to
drop Action frames from STA/AP before 4-way handshake. If we want to get
rid of drop_unencrypted, this function may need another condition to
drop the frame based on WLAN_STA_MFP flag. I have clearly assumed that
drop_unencrypted was set here (and maybe that was indeed the case in
early 2009 or maybe I did testing with WEXT at the time based on commit
0c7c10c7cc6bc890d23c8c62b81b4feccd92124b).
--
Jouni Malinen PGP id EFC895FA
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.
2012-07-04 17:44 ` Jouni Malinen
@ 2012-07-05 7:55 ` Johannes Berg
0 siblings, 0 replies; 12+ messages in thread
From: Johannes Berg @ 2012-07-05 7:55 UTC (permalink / raw)
To: Jouni Malinen; +Cc: Nicolas Cavallari, John W. Linville, linux-wireless
On Wed, 2012-07-04 at 20:44 +0300, Jouni Malinen wrote:
> drop_unencrypted was originally (i.e., way before MFP) added as an extra
> protection for some corner cases where keys may not have been set. In
> theory, the PAE (authorized vs. unauthorized) should have covered those
> cases, but there were some multi-SSID AP cases that were not obviously
> clear. Consequently, it felt safer to add an extra protection for BSSes
> that are known to use encryption for data frames.
Hmm, ok.
> As far as MFP is concerned, we have the WLAN_STA_MFP flag that should be
> more reliable way of determining whether robust management frames have
> to be protected.
Right.
> > But in a IBSS with RSN, if wpa_supplicant
> > isn't recent enough, stations are always authorized by default. so
> > drop_encrypted is required in this case.
>
> For a BSS that uses RSN, we could maintain a new flag that indicates
> that (non-nullfunc) Data frames are not to be transmitted or received
> without protected. Though, this would be quite similar to
> drop_unencrypted in practice.
>
>
> As far as the new patch is concerned, it would look like this is
> extending the fix in commit e0463f501fb945c1fde536d98eefc5ba156ff497.
> The commit log for that change seems to claim that the goal was to avoid
> dropping any management frames to a STA that does not use MFP, but the
> change does not seem to do that.
Yeah, it's a bit confusing, especially since the drop_unencrypted is in
there.
> As far as drop_unencrypted not being used in AP/managed mode is
> concerned, that sounds like an additional bug.. This code is supposed to
> drop Action frames from STA/AP before 4-way handshake. If we want to get
> rid of drop_unencrypted, this function may need another condition to
> drop the frame based on WLAN_STA_MFP flag. I have clearly assumed that
> drop_unencrypted was set here (and maybe that was indeed the case in
> early 2009 or maybe I did testing with WEXT at the time based on commit
> 0c7c10c7cc6bc890d23c8c62b81b4feccd92124b).
It looks a bit it got lost years ago in commit
f21293549f60f88c74fcb9944737f11048896dc4, but I can't tell you why. We
also never added nl80211 API for it. Did we just miss it?
I guess what we should do now is figure out what should be going on, do
we even need drop_unencrypted still or are we ok with only MFP?
johannes
^ permalink raw reply [flat|nested] 12+ messages in thread