[PATCH] netdev: fix asymmetry in handshake events

[PATCH] netdev: fix asymmetry in handshake events

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2260 bytes --]

A failed/timed out handshake can sometimes result in no
HANDSHAKE_EVENT_FAILED event. This is because if IWD receives
a deauth before the eapol timeout expires the handshake is
cleaned up without any error being signaled. This ends up being
ok because netdev has its own handshake failed event associated
with the connect attempt which station/others handle
appropriately.

The issue lies more with what a handshake event consumer would
expect. Not receiving a failed event in the case of a failure
is unexpected. Future changes will rely on symmetry of these
events so it is being fixed now.

This case can be detected inside the netdev death event handler
by checking if last_code was ever set. If last_code is zero,
and netdev is not operational we have hit this situation and
netdev needs to manually trigger HANDSHAKE_EVENT_FAILED.
---
 src/netdev.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/netdev.c b/src/netdev.c
index bf35b1ee..ebbcbbbc 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -869,6 +869,24 @@ static void netdev_disconnect_event(struct l_genl_msg *msg,
 	l_info("Received Deauthentication event, reason: %hu, from_ap: %s",
 			reason_code, disconnect_by_ap ? "true" : "false");
 
+	/*
+	 * In some cases of a failed handshake the AP will send a deauth before
+	 * IWD's eapol timer expires. This causes asymmetry in the handshake
+	 * started/failed events because the eapol timer would get cleaned up by
+	 * netdev_connect_free below and no failed event would be sent. We can
+	 * detect this because last_code is only set via netdev_handshake_failed
+	 * which indicates the handshake failed event DID fire. If no failed
+	 * event fired, and netdev is not operational, something went wrong in
+	 * the handshake and this deauth event is the AP rejecting it. In this
+	 * case signal the failed event which will ultimately clean up the
+	 * connection via netdev_handshake_failed.
+	 */
+	if (!netdev->operational && !netdev->last_code) {
+		handshake_event(netdev->handshake, HANDSHAKE_EVENT_FAILED,
+				reason_code);
+		return;
+	}
+
 	event_filter = netdev->event_filter;
 	event_data = netdev->user_data;
 	netdev_connect_free(netdev);
-- 
2.26.2

Re: [PATCH] netdev: fix asymmetry in handshake events

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3042 bytes --]

Hi James,

On 1/4/21 5:32 PM, James Prestwood wrote:
> A failed/timed out handshake can sometimes result in no
> HANDSHAKE_EVENT_FAILED event. This is because if IWD receives
> a deauth before the eapol timeout expires the handshake is
> cleaned up without any error being signaled. This ends up being
> ok because netdev has its own handshake failed event associated
> with the connect attempt which station/others handle
> appropriately.
> 
> The issue lies more with what a handshake event consumer would
> expect. Not receiving a failed event in the case of a failure
> is unexpected. Future changes will rely on symmetry of these
> events so it is being fixed now.
> 
> This case can be detected inside the netdev death event handler
> by checking if last_code was ever set. If last_code is zero,
> and netdev is not operational we have hit this situation and
> netdev needs to manually trigger HANDSHAKE_EVENT_FAILED.
> ---
>   src/netdev.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/src/netdev.c b/src/netdev.c
> index bf35b1ee..ebbcbbbc 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -869,6 +869,24 @@ static void netdev_disconnect_event(struct l_genl_msg *msg,
>   	l_info("Received Deauthentication event, reason: %hu, from_ap: %s",
>   			reason_code, disconnect_by_ap ? "true" : "false");
>   
> +	/*
> +	 * In some cases of a failed handshake the AP will send a deauth before
> +	 * IWD's eapol timer expires. This causes asymmetry in the handshake
> +	 * started/failed events because the eapol timer would get cleaned up by
> +	 * netdev_connect_free below and no failed event would be sent. We can

Yes, this can happen.  As you point out, we rely on station.c detecting this 
case and resolving it the same way as a failed handshake result.

> +	 * detect this because last_code is only set via netdev_handshake_failed
> +	 * which indicates the handshake failed event DID fire. If no failed
> +	 * event fired, and netdev is not operational, something went wrong in
> +	 * the handshake and this deauth event is the AP rejecting it. In this
> +	 * case signal the failed event which will ultimately clean up the
> +	 * connection via netdev_handshake_failed.

Well sort of.  We do not really know why the AP disconnected us (unless we look 
at the reason code, and that is never reliable).  So, in some respect, this 
isn't really a handshake failure.  Abort maybe?

> +	 */
> +	if (!netdev->operational && !netdev->last_code) {
> +		handshake_event(netdev->handshake, HANDSHAKE_EVENT_FAILED,
> +				reason_code);

Wouldn't this cause us to issue another Disconnect?  Sort of useless to do that 
if we know we're disconnected already.  The kernel just told us.

Perhaps this should be treated as another handshake event.  Like 
HANDSHAKE_EVENT_ABORTED.

> +		return;
> +	}
> +
>   	event_filter = netdev->event_filter;
>   	event_data = netdev->user_data;
>   	netdev_connect_free(netdev);
> 

Regards,
-Denis

Re: [PATCH] netdev: fix asymmetry in handshake events

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

Hi,

> Wouldn't this cause us to issue another Disconnect?  Sort of useless
> to do that 
> if we know we're disconnected already.  The kernel just told us.
> 
Yes, your right.

> Perhaps this should be treated as another handshake event.  Like 
> HANDSHAKE_EVENT_ABORTED.

Well basically I wanted this symmetry for the diagnostic interface, for
purposes of timing auth/assoc/handshake etc. as well as obtaining the
status/reason code. This was a quick hack but there were unforseen
consequences.

I was planning on this eventually but instead maybe I could just make
the netdev result events subscribeable with a watchlist. This would get
me the auth/assoc timings, status/reason code, as well as handle this
case as station already does without the need for a special purpose
event.

Thanks,
James