diff mbox series

[1/2] mac80211: enable TKIP when using encapsulation offloading

Message ID 1587720951-9222-2-git-send-email-murugana@codeaurora.org (mailing list archive)
State Rejected
Delegated to: Johannes Berg
Headers show
Series mac80211: enable TKIP when using encapsulation offloading | expand

Commit Message

Sathishkumar Muruganandam April 24, 2020, 9:35 a.m. UTC
TKIP was disabled on encap offload mode since if TKIP MIC error
reporting is not capable by the driver using encap offload mode,
then TKIP countermeasures wont work.

Enabling back TKIP bring-up in encap offload mode to leave the
control with the driver to disable hw encap when it is not capable
of TKIP MIC error reporting.

Fixes: 50ff477a8639 ("mac80211: add 802.11 encapsulation offloading support")
Signed-off-by: Sathishkumar Muruganandam <murugana@codeaurora.org>
---
 net/mac80211/iface.c | 7 -------
 net/mac80211/key.c   | 7 -------
 2 files changed, 14 deletions(-)

Comments

Johannes Berg May 26, 2020, 12:44 p.m. UTC | #1
On Fri, 2020-04-24 at 15:05 +0530, Sathishkumar Muruganandam wrote:
> TKIP was disabled on encap offload mode since if TKIP MIC error
> reporting is not capable by the driver using encap offload mode,
> then TKIP countermeasures wont work.
> 
> Enabling back TKIP bring-up in encap offload mode to leave the
> control with the driver to disable hw encap when it is not capable
> of TKIP MIC error reporting.

So how can the driver do TKIP MIC error reporting?

I don't think it can. It doesn't have all the necessary information to
call cfg80211_michael_mic_failure(), as far as I can tell.

I'm not applying this until I see at least the *ability* for drivers to
do it. I mean, I don't really want to worry about your driver's
correctness, but if you then turn around and realize that you *cannot*
actually implement the right behaviour, that's no good either.

johannes
Sathishkumar Muruganandam May 29, 2020, 12:08 p.m. UTC | #2
On 2020-05-26 18:14, Johannes Berg wrote:
> On Fri, 2020-04-24 at 15:05 +0530, Sathishkumar Muruganandam wrote:
>> TKIP was disabled on encap offload mode since if TKIP MIC error
>> reporting is not capable by the driver using encap offload mode,
>> then TKIP countermeasures wont work.
>> 
>> Enabling back TKIP bring-up in encap offload mode to leave the
>> control with the driver to disable hw encap when it is not capable
>> of TKIP MIC error reporting.
> 
> So how can the driver do TKIP MIC error reporting?
> 
> I don't think it can. It doesn't have all the necessary information to
> call cfg80211_michael_mic_failure(), as far as I can tell.

Ath11k HW has capability of reporting TKIP MIC error when hw encap is 
enabled as well, so TKIP encryption shall be enabled.

> 
> I'm not applying this until I see at least the *ability* for drivers to
> do it. I mean, I don't really want to worry about your driver's
> correctness, but if you then turn around and realize that you *cannot*
> actually implement the right behaviour, that's no good either.
> 

Shall we have a ieee80211_hw_flags for the driver to advertise TKIP MIC 
error reporting capability when hw encap is enabled ?
TKIP bring-up will be disabled for the drivers which doesn't have the 
capability.

--
Sathishkumar
Johannes Berg May 29, 2020, 12:10 p.m. UTC | #3
On Fri, 2020-05-29 at 17:38 +0530, Sathishkumar Muruganandam wrote:

> > I don't think it can. It doesn't have all the necessary information to
> > call cfg80211_michael_mic_failure(), as far as I can tell.
> 
> Ath11k HW has capability of reporting TKIP MIC error when hw encap is 
> enabled as well, so TKIP encryption shall be enabled.

Maybe so. But it cannot tell wpa_supplicant about MIC errors, and so
that cannot do countermeasures, so something isn't right.

> Shall we have a ieee80211_hw_flags for the driver to advertise TKIP MIC 
> error reporting capability when hw encap is enabled ?
> TKIP bring-up will be disabled for the drivers which doesn't have the 
> capability.

That would be better, since for TKIP it's actually more tricky and
requires more work.

But mostly I was thinking that you should make it possible for ath11k to
actually report the MIC errors up to userspace. Right now I don't think
it can, because it doesn't have the netdev pointer?

johannes
Sathishkumar Muruganandam May 29, 2020, 12:29 p.m. UTC | #4
On 2020-05-29 17:40, Johannes Berg wrote:
> On Fri, 2020-05-29 at 17:38 +0530, Sathishkumar Muruganandam wrote:
> 
>> > I don't think it can. It doesn't have all the necessary information to
>> > call cfg80211_michael_mic_failure(), as far as I can tell.
>> 
>> Ath11k HW has capability of reporting TKIP MIC error when hw encap is
>> enabled as well, so TKIP encryption shall be enabled.
> 
> Maybe so. But it cannot tell wpa_supplicant about MIC errors, and so
> that cannot do countermeasures, so something isn't right.
> 
>> Shall we have a ieee80211_hw_flags for the driver to advertise TKIP 
>> MIC
>> error reporting capability when hw encap is enabled ?
>> TKIP bring-up will be disabled for the drivers which doesn't have the
>> capability.
> 
> That would be better, since for TKIP it's actually more tricky and
> requires more work.
> 
> But mostly I was thinking that you should make it possible for ath11k 
> to
> actually report the MIC errors up to userspace. Right now I don't think
> it can, because it doesn't have the netdev pointer?
> 

Yes, currently only tx encap support is added and rx decap support is in
progress to do TKIP MIC error reporting to userspace via 
cfg80211_michael_mic_failure().
With NL80211_CMD_MICHAEL_MIC_FAILURE, hostapd will be able to do TKIP 
counter-measures.

Thanks,
Sathishkumar
Johannes Berg May 29, 2020, 12:46 p.m. UTC | #5
On Fri, 2020-05-29 at 17:59 +0530, Sathishkumar Muruganandam wrote:
> 
> Yes, currently only tx encap support is added and rx decap support is in
> progress to do TKIP MIC error reporting to userspace via 
> cfg80211_michael_mic_failure().

Yes, but can you actually call that? It requires a netdev. I don't think
you can easily get it from the vif?

johannes
Sathishkumar Muruganandam May 29, 2020, 2:40 p.m. UTC | #6
On 2020-05-29 18:16, Johannes Berg wrote:
> On Fri, 2020-05-29 at 17:59 +0530, Sathishkumar Muruganandam wrote:
>> 
>> Yes, currently only tx encap support is added and rx decap support is 
>> in
>> progress to do TKIP MIC error reporting to userspace via
>> cfg80211_michael_mic_failure().
> 
> Yes, but can you actually call that? It requires a netdev. I don't 
> think
> you can easily get it from the vif?
> 

Yes, that's right. Currently the plan is to add new mac80211 api for rx 
decap where
we can call cfg80211_michael_mic_failure() and not called directly from 
driver.

Whether we can expose netdev to driver for doing such cfg80211 call ?

Thanks,
Sathishkumar
Johannes Berg May 29, 2020, 2:44 p.m. UTC | #7
On Fri, 2020-05-29 at 20:10 +0530, Sathishkumar Muruganandam wrote:
> On 2020-05-29 18:16, Johannes Berg wrote:
> > On Fri, 2020-05-29 at 17:59 +0530, Sathishkumar Muruganandam wrote:
> > > Yes, currently only tx encap support is added and rx decap support is 
> > > in
> > > progress to do TKIP MIC error reporting to userspace via
> > > cfg80211_michael_mic_failure().
> > 
> > Yes, but can you actually call that? It requires a netdev. I don't 
> > think
> > you can easily get it from the vif?
> > 
> 
> Yes, that's right. Currently the plan is to add new mac80211 api for rx 
> decap where
> we can call cfg80211_michael_mic_failure() and not called directly from 
> driver.

Sure, that sounds fine.

Really what I was saying is that we should have that together, not just
this patch that allows TKIP offload, but then nothing that actually
makes that working properly/safely.

> Whether we can expose netdev to driver for doing such cfg80211 call ?

I've always worried that if we expose the netdev we'll just get all
kinds of messy things happening in the driver :)

But maybe not. I guess it doesn't make a big difference.

johannes
Sathishkumar Muruganandam May 29, 2020, 2:51 p.m. UTC | #8
On 2020-05-29 20:14, Johannes Berg wrote:
> On Fri, 2020-05-29 at 20:10 +0530, Sathishkumar Muruganandam wrote:
>> On 2020-05-29 18:16, Johannes Berg wrote:
>> > On Fri, 2020-05-29 at 17:59 +0530, Sathishkumar Muruganandam wrote:
>> > > Yes, currently only tx encap support is added and rx decap support is
>> > > in
>> > > progress to do TKIP MIC error reporting to userspace via
>> > > cfg80211_michael_mic_failure().
>> >
>> > Yes, but can you actually call that? It requires a netdev. I don't
>> > think
>> > you can easily get it from the vif?
>> >
>> 
>> Yes, that's right. Currently the plan is to add new mac80211 api for 
>> rx
>> decap where
>> we can call cfg80211_michael_mic_failure() and not called directly 
>> from
>> driver.
> 
> Sure, that sounds fine.
> 
> Really what I was saying is that we should have that together, not just
> this patch that allows TKIP offload, but then nothing that actually
> makes that working properly/safely.

Sure, thanks Johannes !

Will include this along with rx decap patch.

> 
>> Whether we can expose netdev to driver for doing such cfg80211 call ?
> 
> I've always worried that if we expose the netdev we'll just get all
> kinds of messy things happening in the driver :)
> 
> But maybe not. I guess it doesn't make a big difference.
> 

Sure, will fallback to mac80211 approach for rx decap. Thanks for your 
review !

Thanks,
Sathishkumar
diff mbox series

Patch

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index d069825705d6..68af8acde0f0 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1257,13 +1257,6 @@  bool ieee80211_set_hw_80211_encap(struct ieee80211_vif *vif, bool enable)
 	    (local->hw.wiphy->frag_threshold != (u32)-1))
 		enable = false;
 
-	mutex_lock(&sdata->local->key_mtx);
-	list_for_each_entry(key, &sdata->key_list, list) {
-		if (key->conf.cipher == WLAN_CIPHER_SUITE_TKIP)
-			enable = false;
-	}
-	mutex_unlock(&sdata->local->key_mtx);
-
 	__ieee80211_set_hw_80211_encap(sdata, enable);
 
 	return enable;
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 8f403c1bb908..25cab6fc127c 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -177,13 +177,6 @@  static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
 		}
 	}
 
-	/* TKIP countermeasures don't work in encap offload mode */
-	if (key->conf.cipher == WLAN_CIPHER_SUITE_TKIP &&
-	    sdata->hw_80211_encap) {
-		sdata_dbg(sdata, "TKIP is not allowed in hw 80211 encap mode\n");
-		return -EINVAL;
-	}
-
 	ret = drv_set_key(key->local, SET_KEY, sdata,
 			  sta ? &sta->sta : NULL, &key->conf);