diff mbox series

[2/6] brcmfmac: send port authorized event for 802.1X 4-way handshake offload

Message ID 1546582221-143220-2-git-send-email-chi-hsien.lin@cypress.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series [1/6] nl80211: add NL80211_ATTR_IFINDEX to port authorized event | expand

Commit Message

Chi-Hsien Lin Jan. 4, 2019, 6:11 a.m. UTC 
From: Chung-Hsien Hsu <stanley.hsu@cypress.com>

With 4-way handshake offload for 802.1X authentication, a port
authorized event should be sent to user space after the completion of
4-way handshake. It is used to indicate that a connection is authorized
and 802.1X authentication is no longer required.

Signed-off-by: Chung-Hsien Hsu <stanley.hsu@cypress.com>
Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 23 +++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Comments

Arend van Spriel Jan. 7, 2019, 9:44 a.m. UTC | #1 
On 1/4/2019 7:11 AM, Chi-Hsien Lin wrote:
> From: Chung-Hsien Hsu <stanley.hsu@cypress.com>
> 
> With 4-way handshake offload for 802.1X authentication, a port
> authorized event should be sent to user space after the completion of
> 4-way handshake. It is used to indicate that a connection is authorized
> and 802.1X authentication is no longer required.

It had been a while since I had looked at our offload code (basically 
since the initial implementation for the nl80211 work) so I was unsure 
why this would be needed.

So initially we added a PORT_AUTHORIZED *attribute* in the nl80211 api 
and later on the PORT_AUTHORIZED *event* was introduced and 4-way hs 
offload support in wpa_supplicant is ignoring the *attribute* and only 
handling the *event*. I think this information is important enough to 
add to this commit message with a reference to commit 503c1fb98ba3 
("cfg80211/nl80211: add a port authorized event") which "broke" the 
functionality in brcmfmac.

Some specific comments below...

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Chung-Hsien Hsu <stanley.hsu@cypress.com>
> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
> ---
>   .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 23 +++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 35301237d435..ad0d775a1244 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -5266,10 +5266,13 @@ static bool brcmf_is_linkup(struct brcmf_cfg80211_vif *vif,
>   	u32 event = e->event_code;
>   	u32 status = e->status;
>   
> -	if (vif->profile.use_fwsup == BRCMF_PROFILE_FWSUP_PSK &&
> -	    event == BRCMF_E_PSK_SUP &&
> -	    status == BRCMF_E_STATUS_FWSUP_COMPLETED)
> +	if (event == BRCMF_E_PSK_SUP &&
> +	    status == BRCMF_E_STATUS_FWSUP_COMPLETED) {
>   		set_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state);
> +		if (vif->profile.use_fwsup == BRCMF_PROFILE_FWSUP_1X)
> +			return true;
> +	}
> +

Here things get a bit tricky I think. The old behaviour was to wait for 
both PSK_SUP and SET_SSID events before calling cfg80211_connect_done(). 
If I recall correctly I did that because they can arrive in different 
order depending on the type of offload (1x or psk) but also depends on 
firmware build, so ....

>   	if (event == BRCMF_E_SET_SSID && status == BRCMF_E_STATUS_SUCCESS) {
>   		brcmf_dbg(CONN, "Processing set ssid\n");
>   		memcpy(vif->profile.bssid, e->addr, ETH_ALEN);
> @@ -5280,11 +5283,10 @@ static bool brcmf_is_linkup(struct brcmf_cfg80211_vif *vif,
>   	}
>   
>   	if (test_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state) &&
> -	    test_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &vif->sme_state)) {
> -		clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state);
> -		clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &vif->sme_state);
> +	    test_and_clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS,
> +			       &vif->sme_state))
>   		return true;
> -	}
> +
>   	return false;
>   }
>   
> @@ -5501,6 +5503,13 @@ brcmf_bss_connect_done(struct brcmf_cfg80211_info *cfg,
>   		brcmf_dbg(CONN, "Report connect result - connection %s\n",
>   			  completed ? "succeeded" : "failed");
>   	}
> +
> +	if (test_and_clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS,
> +			       &ifp->vif->sme_state) &&
> +	    profile->use_fwsup == BRCMF_PROFILE_FWSUP_1X) {
> +		cfg80211_port_authorized(ndev, profile->bssid, GFP_KERNEL);
> +		brcmf_dbg(CONN, "Report port authorized\n");
> +	}

I would leave the code in brcmf_is_linkup() as before and only check 
profile->use_fwsup here to determine whether cfg80211_port_authorized() 
should be called here.

>   	brcmf_dbg(TRACE, "Exit\n");
>   	return 0;
>   }
>
Chung-Hsien Hsu May 9, 2019, 8:58 a.m. UTC | #2 
On Mon, Jan 07, 2019 at 10:44:01AM +0100, Arend Van Spriel wrote:
> On 1/4/2019 7:11 AM, Chi-Hsien Lin wrote:
> >From: Chung-Hsien Hsu <stanley.hsu@cypress.com>
> >
> >With 4-way handshake offload for 802.1X authentication, a port
> >authorized event should be sent to user space after the completion of
> >4-way handshake. It is used to indicate that a connection is authorized
> >and 802.1X authentication is no longer required.
>
> It had been a while since I had looked at our offload code
> (basically since the initial implementation for the nl80211 work) so
> I was unsure why this would be needed.
>
> So initially we added a PORT_AUTHORIZED *attribute* in the nl80211
> api and later on the PORT_AUTHORIZED *event* was introduced and
> 4-way hs offload support in wpa_supplicant is ignoring the
> *attribute* and only handling the *event*. I think this information
> is important enough to add to this commit message with a reference
> to commit 503c1fb98ba3 ("cfg80211/nl80211: add a port authorized
> event") which "broke" the functionality in brcmfmac.

Thanks a lot for the feedback.
After looking further, it is observed that the connection state will be
set to WPA_COMPLETED in wpa_supplicant after it sets PMK to the driver.
So no need to have this change. Let's drop it form the series.

Regards,
Chung-Hsien

This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
Arend van Spriel May 9, 2019, 11:54 a.m. UTC | #3 
+ Jouni

On 5/9/2019 10:58 AM, Stanley Hsu wrote:
> On Mon, Jan 07, 2019 at 10:44:01AM +0100, Arend Van Spriel wrote:
>> On 1/4/2019 7:11 AM, Chi-Hsien Lin wrote:
>>> From: Chung-Hsien Hsu <stanley.hsu@cypress.com>
>>>
>>> With 4-way handshake offload for 802.1X authentication, a port
>>> authorized event should be sent to user space after the completion of
>>> 4-way handshake. It is used to indicate that a connection is authorized
>>> and 802.1X authentication is no longer required.
>>
>> It had been a while since I had looked at our offload code
>> (basically since the initial implementation for the nl80211 work) so
>> I was unsure why this would be needed.
>>
>> So initially we added a PORT_AUTHORIZED *attribute* in the nl80211
>> api and later on the PORT_AUTHORIZED *event* was introduced and
>> 4-way hs offload support in wpa_supplicant is ignoring the
>> *attribute* and only handling the *event*. I think this information
>> is important enough to add to this commit message with a reference
>> to commit 503c1fb98ba3 ("cfg80211/nl80211: add a port authorized
>> event") which "broke" the functionality in brcmfmac.
> 
> Thanks a lot for the feedback.
> After looking further, it is observed that the connection state will be
> set to WPA_COMPLETED in wpa_supplicant after it sets PMK to the driver.
> So no need to have this change. Let's drop it form the series.

In my opinion wpa_supplicant does set WPA_COMPLETED too early. If we 
were to use eapol-over-nl80211 and set the netdev carrier when the 
connection is authorized it would be kinda ok and we would not need the 
event. Added Jouni to chime in on this.

Regards,
Arend
diff mbox series

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 35301237d435..ad0d775a1244 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5266,10 +5266,13 @@  static bool brcmf_is_linkup(struct brcmf_cfg80211_vif *vif,
 	u32 event = e->event_code;
 	u32 status = e->status;
 
-	if (vif->profile.use_fwsup == BRCMF_PROFILE_FWSUP_PSK &&
-	    event == BRCMF_E_PSK_SUP &&
-	    status == BRCMF_E_STATUS_FWSUP_COMPLETED)
+	if (event == BRCMF_E_PSK_SUP &&
+	    status == BRCMF_E_STATUS_FWSUP_COMPLETED) {
 		set_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state);
+		if (vif->profile.use_fwsup == BRCMF_PROFILE_FWSUP_1X)
+			return true;
+	}
+
 	if (event == BRCMF_E_SET_SSID && status == BRCMF_E_STATUS_SUCCESS) {
 		brcmf_dbg(CONN, "Processing set ssid\n");
 		memcpy(vif->profile.bssid, e->addr, ETH_ALEN);
@@ -5280,11 +5283,10 @@  static bool brcmf_is_linkup(struct brcmf_cfg80211_vif *vif,
 	}
 
 	if (test_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state) &&
-	    test_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &vif->sme_state)) {
-		clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state);
-		clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &vif->sme_state);
+	    test_and_clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS,
+			       &vif->sme_state))
 		return true;
-	}
+
 	return false;
 }
 
@@ -5501,6 +5503,13 @@  brcmf_bss_connect_done(struct brcmf_cfg80211_info *cfg,
 		brcmf_dbg(CONN, "Report connect result - connection %s\n",
 			  completed ? "succeeded" : "failed");
 	}
+
+	if (test_and_clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS,
+			       &ifp->vif->sme_state) &&
+	    profile->use_fwsup == BRCMF_PROFILE_FWSUP_1X) {
+		cfg80211_port_authorized(ndev, profile->bssid, GFP_KERNEL);
+		brcmf_dbg(CONN, "Report port authorized\n");
+	}
 	brcmf_dbg(TRACE, "Exit\n");
 	return 0;
 }