| Message ID | 1468248832-21969-6-git-send-email-akarwar@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Amitkumar Karwar <akarwar@marvell.com> writes: > It is observed that hostapd fails to setup with management frame > protection mode enabled when using mwifiex. This patch adds > cfg80211_set_default_mgmt_key handler to resolve the problem. > > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > --- > drivers/net/wireless/marvell/mwifiex/cfg80211.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > index 8955f8c..bf95cca 100644 > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > @@ -484,6 +484,18 @@ mwifiex_cfg80211_add_key(struct wiphy *wiphy, struct net_device *netdev, > } > > /* > + * CFG802.11 operation handler to set default mgmt key. > + */ > +static int > +mwifiex_cfg80211_set_default_mgmt_key(struct wiphy *wiphy, > + struct net_device *netdev, > + u8 key_index) > +{ > + wiphy_dbg(wiphy, "set default mgmt key, key index=%d\n", key_index); > + return 0; > +} > + > +/* > * This function sends domain information to the firmware. > * > * The following information are passed to the firmware - > @@ -4002,6 +4014,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = { > .leave_ibss = mwifiex_cfg80211_leave_ibss, > .add_key = mwifiex_cfg80211_add_key, > .del_key = mwifiex_cfg80211_del_key, > + .set_default_mgmt_key = mwifiex_cfg80211_set_default_mgmt_key, > .mgmt_tx = mwifiex_cfg80211_mgmt_tx, > .mgmt_frame_register = mwifiex_cfg80211_mgmt_frame_register, > .remain_on_channel = mwifiex_cfg80211_remain_on_channel, Is it correct to ignore the key index? I see that brcmfmac ignores it as well but I want to still confirm this. Does this mean that with this patcfh mwifiex properly supports MFP?
Hi Kalle, > From: Kalle Valo [mailto:kvalo@codeaurora.org] > Sent: Monday, July 18, 2016 10:49 PM > To: Amitkumar Karwar > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam > Subject: Re: [PATCH 5/9] mwifiex: cfg80211 set_default_mgmt_key handler > > Amitkumar Karwar <akarwar@marvell.com> writes: > > > It is observed that hostapd fails to setup with management frame > > protection mode enabled when using mwifiex. This patch adds > > cfg80211_set_default_mgmt_key handler to resolve the problem. > > > > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > > --- > > drivers/net/wireless/marvell/mwifiex/cfg80211.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c > > b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > > index 8955f8c..bf95cca 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c > > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > > @@ -484,6 +484,18 @@ mwifiex_cfg80211_add_key(struct wiphy *wiphy, > > struct net_device *netdev, } > > > > /* > > + * CFG802.11 operation handler to set default mgmt key. > > + */ > > +static int > > +mwifiex_cfg80211_set_default_mgmt_key(struct wiphy *wiphy, > > + struct net_device *netdev, > > + u8 key_index) > > +{ > > + wiphy_dbg(wiphy, "set default mgmt key, key index=%d\n", > key_index); > > + return 0; > > +} > > + > > +/* > > * This function sends domain information to the firmware. > > * > > * The following information are passed to the firmware - @@ -4002,6 > > +4014,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = { > > .leave_ibss = mwifiex_cfg80211_leave_ibss, > > .add_key = mwifiex_cfg80211_add_key, > > .del_key = mwifiex_cfg80211_del_key, > > + .set_default_mgmt_key = mwifiex_cfg80211_set_default_mgmt_key, > > .mgmt_tx = mwifiex_cfg80211_mgmt_tx, > > .mgmt_frame_register = mwifiex_cfg80211_mgmt_frame_register, > > .remain_on_channel = mwifiex_cfg80211_remain_on_channel, > > Is it correct to ignore the key index? I see that brcmfmac ignores it as > well but I want to still confirm this. > > Does this mean that with this patcfh mwifiex properly supports MFP? > Yes. We do pass MFP tests with this patch. Regards, Amitkumar -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 21, 2016 at 09:18:11AM +0000, Amitkumar Karwar wrote: > > From: Kalle Valo [mailto:kvalo@codeaurora.org] > > Is it correct to ignore the key index? I see that brcmfmac ignores it as > > well but I want to still confirm this. > > > > Does this mean that with this patcfh mwifiex properly supports MFP? > > Yes. We do pass MFP tests with this patch. Did you test IGTK rekeying? This patch looks exactly as broken as it did the last time it was proposed more than a year ago and after the same concern not receiving any reaction.. hostapd will configure two different IGTKs with different Key IDs and change the TX key on the AP once all associated STAs have the new key. If the driver does not support updating the TX key index, either the old or the new STAs associated after rekeying will not have the correct key.
Hi Jouni, > From: Jouni Malinen [mailto:j@w1.fi] > Sent: Thursday, July 21, 2016 9:22 PM > To: Amitkumar Karwar > Cc: Kalle Valo; linux-wireless@vger.kernel.org; Cathy Luo; Nishant > Sarmukadam > Subject: Re: [PATCH 5/9] mwifiex: cfg80211 set_default_mgmt_key handler > > On Thu, Jul 21, 2016 at 09:18:11AM +0000, Amitkumar Karwar wrote: > > > From: Kalle Valo [mailto:kvalo@codeaurora.org] Is it correct to > > > ignore the key index? I see that brcmfmac ignores it as well but I > > > want to still confirm this. > > > > > > Does this mean that with this patcfh mwifiex properly supports MFP? > > > > Yes. We do pass MFP tests with this patch. > > Did you test IGTK rekeying? This patch looks exactly as broken as it did > the last time it was proposed more than a year ago and after the same > concern not receiving any reaction.. hostapd will configure two > different IGTKs with different Key IDs and change the TX key on the AP > once all associated STAs have the new key. If the driver does not > support updating the TX key index, either the old or the new STAs > associated after rekeying will not have the correct key. > Thanks for your feedback and guidance on this. I am trying to understand the problem you mentioned during IGTK rekeying. Today I ran tests with two stations connecting an AP. MFP is enabled on all of them. On hostapd side, my observation is add_key() is always called followed by set_default_mgmt_key(). set_default_mgmt_key() sets the key added by add_key() as default key. We are ignoring set_default_mgmt_key() and updating Tx key index during add_key() itself. Your concerns is we should not update Tx key index during add_key(). Reason is IGTK rekeying is not yet completed with all stations. Right? Regards, Amitkumar -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 22, 2016 at 03:59:47PM +0000, Amitkumar Karwar wrote: > I am trying to understand the problem you mentioned during IGTK rekeying. Today I ran tests with two stations connecting an AP. MFP is enabled on all of them. > > On hostapd side, my observation is add_key() is always called followed by set_default_mgmt_key(). set_default_mgmt_key() sets the key added by add_key() as default key. > > We are ignoring set_default_mgmt_key() and updating Tx key index during add_key() itself. > > Your concerns is we should not update Tx key index during add_key(). Reason is IGTK rekeying is not yet completed with all stations. Right? Correct. set_default_mgmt_key() does not have much effect for the very first IGTK configuration, but whenever doing IGTK rekeying, hostapd behaves just like it does with GTK rekeying. In other words, a different Key ID is selected (alternating between 4 and 5), a random new IGTK is generated, the new IGTK is configured to the local driver (but the old IGTK is still supposed to be used for TX), each associated STA is notified of the new IGTK, the new IGTK is taken into use once the group key handshake has completed with each associated STA. It is that last operation that needs set_default_mgmt_key() to allow this rekeying to work correctly. If you update the TX Key ID on add_key(), you'll risk sending out frames that some of the associated STAs do not yet have a key to validate.
Hi Jouni, > From: Jouni Malinen [mailto:j@w1.fi] > Sent: Friday, July 22, 2016 10:25 PM > To: Amitkumar Karwar > Cc: Kalle Valo; linux-wireless@vger.kernel.org; Cathy Luo; Nishant > Sarmukadam > Subject: Re: [PATCH 5/9] mwifiex: cfg80211 set_default_mgmt_key handler > > On Fri, Jul 22, 2016 at 03:59:47PM +0000, Amitkumar Karwar wrote: > > I am trying to understand the problem you mentioned during IGTK > rekeying. Today I ran tests with two stations connecting an AP. MFP is > enabled on all of them. > > > > On hostapd side, my observation is add_key() is always called followed > by set_default_mgmt_key(). set_default_mgmt_key() sets the key added by > add_key() as default key. > > > > We are ignoring set_default_mgmt_key() and updating Tx key index > during add_key() itself. > > > > Your concerns is we should not update Tx key index during add_key(). > Reason is IGTK rekeying is not yet completed with all stations. Right? > > Correct. set_default_mgmt_key() does not have much effect for the very > first IGTK configuration, but whenever doing IGTK rekeying, hostapd > behaves just like it does with GTK rekeying. In other words, a different > Key ID is selected (alternating between 4 and 5), a random new IGTK is > generated, the new IGTK is configured to the local driver (but the old > IGTK is still supposed to be used for TX), each associated STA is > notified of the new IGTK, the new IGTK is taken into use once the group > key handshake has completed with each associated STA. It is that last > operation that needs set_default_mgmt_key() to allow this rekeying to > work correctly. If you update the TX Key ID on add_key(), you'll risk > sending out frames that some of the associated STAs do not yet have a > key to validate. > Got it. We will implement set_default_mgmt_key() and check if any firmware changes required. Regards, Amitkumar -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jouni Malinen <j@w1.fi> writes: > On Thu, Jul 21, 2016 at 09:18:11AM +0000, Amitkumar Karwar wrote: >> > From: Kalle Valo [mailto:kvalo@codeaurora.org] >> > Is it correct to ignore the key index? I see that brcmfmac ignores it as >> > well but I want to still confirm this. >> > >> > Does this mean that with this patcfh mwifiex properly supports MFP? >> >> Yes. We do pass MFP tests with this patch. > > Did you test IGTK rekeying? This patch looks exactly as broken as it did > the last time it was proposed more than a year ago and after the same > concern not receiving any reaction.. Indeed, I had already forgetten that this patch was submitted February 2015: https://patchwork.kernel.org/patch/5819421/ Please don't do like this and repost patches without earlier comments addressed (or at least document the history).
diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c index 8955f8c..bf95cca 100644 --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c @@ -484,6 +484,18 @@ mwifiex_cfg80211_add_key(struct wiphy *wiphy, struct net_device *netdev, } /* + * CFG802.11 operation handler to set default mgmt key. + */ +static int +mwifiex_cfg80211_set_default_mgmt_key(struct wiphy *wiphy, + struct net_device *netdev, + u8 key_index) +{ + wiphy_dbg(wiphy, "set default mgmt key, key index=%d\n", key_index); + return 0; +} + +/* * This function sends domain information to the firmware. * * The following information are passed to the firmware - @@ -4002,6 +4014,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = { .leave_ibss = mwifiex_cfg80211_leave_ibss, .add_key = mwifiex_cfg80211_add_key, .del_key = mwifiex_cfg80211_del_key, + .set_default_mgmt_key = mwifiex_cfg80211_set_default_mgmt_key, .mgmt_tx = mwifiex_cfg80211_mgmt_tx, .mgmt_frame_register = mwifiex_cfg80211_mgmt_frame_register, .remain_on_channel = mwifiex_cfg80211_remain_on_channel,
It is observed that hostapd fails to setup with management frame protection mode enabled when using mwifiex. This patch adds cfg80211_set_default_mgmt_key handler to resolve the problem. Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> --- drivers/net/wireless/marvell/mwifiex/cfg80211.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)