[PATCH] Register EAPOL frame listeners earlier

[PATCH] Register EAPOL frame listeners earlier

[jeremy.whiting]

From: Ed Smith <ed.smith@collabora.com>

If we register the main EAPOL frame listener as late as the associate
event, it may not observe ptk_1_of_4. This defeats handling for early
messages in eapol_rx_packet, which only sees messages once it has been
registered.

If we move registration to the authenticate event, then the EAPOL
frame listeners should observe all messages, without any possible
races. Note that the messages are not actually processed until
eapol_start() is called, and we haven't moved that call site. All
that's changing here is how early EAPOL messages can be observed.
---
 src/netdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index 09fac959..fc84c398 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -2982,8 +2982,13 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
 						NULL, netdev->user_data);
 
 		/* We have sent another CMD_AUTHENTICATE / CMD_ASSOCIATE */
-		if (ret == 0 || ret == -EAGAIN)
+		if (ret == 0 || ret == -EAGAIN) {
+			if (!netdev->sm) {
+				netdev->sm = eapol_sm_new(netdev->handshake);
+				eapol_register(netdev->sm);
+			}
 			return;
+		}
 
 		retry = kernel_will_retry_auth(status_code,
 				L_CPU_TO_LE16(auth->algorithm),
@@ -3099,9 +3104,6 @@ static void netdev_associate_event(struct l_genl_msg *msg,
 			netdev->ap = NULL;
 		}
 
-		netdev->sm = eapol_sm_new(netdev->handshake);
-		eapol_register(netdev->sm);
-
 		/* Just in case this was a retry */
 		netdev->ignore_connect_event = false;
 
-- 
2.44.0

Re: [PATCH] Register EAPOL frame listeners earlier

[James Prestwood]

Hi Jeremy,

On 3/26/24 4:11 PM, jeremy.whiting@collabora.com wrote:
> From: Ed Smith <ed.smith@collabora.com>
>
> If we register the main EAPOL frame listener as late as the associate
> event, it may not observe ptk_1_of_4. This defeats handling for early
> messages in eapol_rx_packet, which only sees messages once it has been
> registered.
>
> If we move registration to the authenticate event, then the EAPOL
> frame listeners should observe all messages, without any possible
> races. Note that the messages are not actually processed until
> eapol_start() is called, and we haven't moved that call site. All
> that's changing here is how early EAPOL messages can be observed.
> ---
>   src/netdev.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/src/netdev.c b/src/netdev.c
> index 09fac959..fc84c398 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -2982,8 +2982,13 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
>   						NULL, netdev->user_data);
>   
>   		/* We have sent another CMD_AUTHENTICATE / CMD_ASSOCIATE */
> -		if (ret == 0 || ret == -EAGAIN)
> +		if (ret == 0 || ret == -EAGAIN) {
> +			if (!netdev->sm) {
> +				netdev->sm = eapol_sm_new(netdev->handshake);
> +				eapol_register(netdev->sm);
> +			}
>   			return;
> +		}
>   
>   		retry = kernel_will_retry_auth(status_code,
>   				L_CPU_TO_LE16(auth->algorithm),
> @@ -3099,9 +3104,6 @@ static void netdev_associate_event(struct l_genl_msg *msg,
>   			netdev->ap = NULL;
>   		}
>   
> -		netdev->sm = eapol_sm_new(netdev->handshake);
> -		eapol_register(netdev->sm);
> -
>   		/* Just in case this was a retry */
>   		netdev->ignore_connect_event = false;
>   

I need to have the CI email the original sender so you get notified, but 
as Denis mentioned this still breaks FT because IWD does not use the 
conventional CMD_AUTHENTICATE and instead sends an auth frame using 
CMD_FRAME. This means that there is no authenticate event at all when FT 
is involved. Because of this, no SM gets created (with this patch) which 
breaks rekeys.

After FT authentication netdev_ft_reassociate() is called which issues 
the CMD_CONNECT call (this triggers association). So I think you would 
need to both create the SM similar to where you did here, as well as 
after it gets destroyed in here:

https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/src/netdev.c#n4279

For reference, this is the CI output with this patch:

**Autotest Runner**
Test ID: testrunner
Desc: Runs IWD's autotest framework
Duration: 1868.49 seconds
**Result: FAIL**

Output:
```
testFT-8021x-roam,testFT-FILS,testNetconfigRoam,testPSK-roam,testSAE-roam

Thanks,

James

Re: [PATCH] Register EAPOL frame listeners earlier

[Jeremy Whiting]

On Wednesday, March 27, 2024 05:51 AM MDT, James Prestwood <prestwoj@gmail.com> wrote:

> Hi Jeremy,
> 
> On 3/26/24 4:11 PM, jeremy.whiting@collabora.com wrote:
> > From: Ed Smith <ed.smith@collabora.com>
> >
> > If we register the main EAPOL frame listener as late as the associate
> > event, it may not observe ptk_1_of_4. This defeats handling for early
> > messages in eapol_rx_packet, which only sees messages once it has been
> > registered.
> >
> > If we move registration to the authenticate event, then the EAPOL
> > frame listeners should observe all messages, without any possible
> > races. Note that the messages are not actually processed until
> > eapol_start() is called, and we haven't moved that call site. All
> > that's changing here is how early EAPOL messages can be observed.
> > ---
> >   src/netdev.c | 10 ++++++----
> >   1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/netdev.c b/src/netdev.c
> > index 09fac959..fc84c398 100644
> > --- a/src/netdev.c
> > +++ b/src/netdev.c
> > @@ -2982,8 +2982,13 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
> >   						NULL, netdev->user_data);
> >   
> >   		/* We have sent another CMD_AUTHENTICATE / CMD_ASSOCIATE */
> > -		if (ret == 0 || ret == -EAGAIN)
> > +		if (ret == 0 || ret == -EAGAIN) {
> > +			if (!netdev->sm) {
> > +				netdev->sm = eapol_sm_new(netdev->handshake);
> > +				eapol_register(netdev->sm);
> > +			}
> >   			return;
> > +		}
> >   
> >   		retry = kernel_will_retry_auth(status_code,
> >   				L_CPU_TO_LE16(auth->algorithm),
> > @@ -3099,9 +3104,6 @@ static void netdev_associate_event(struct l_genl_msg *msg,
> >   			netdev->ap = NULL;
> >   		}
> >   
> > -		netdev->sm = eapol_sm_new(netdev->handshake);
> > -		eapol_register(netdev->sm);
> > -
> >   		/* Just in case this was a retry */
> >   		netdev->ignore_connect_event = false;
> >   
> 
> I need to have the CI email the original sender so you get notified, but 
> as Denis mentioned this still breaks FT because IWD does not use the 
> conventional CMD_AUTHENTICATE and instead sends an auth frame using 
> CMD_FRAME. This means that there is no authenticate event at all when FT 
> is involved. Because of this, no SM gets created (with this patch) which 
> breaks rekeys.
> 
> After FT authentication netdev_ft_reassociate() is called which issues 
> the CMD_CONNECT call (this triggers association). So I think you would 
> need to both create the SM similar to where you did here, as well as 
> after it gets destroyed in here:

I think I see. I've got a couple of questions though.

1. How if possible can I run the CI tests locally?
2. Should I instead just change the eapol_register that we removed from _associate_event to keep doing the _register if netdev->sm is null?
i.e. Change the patch from removing the 2 lines to wrapping them in
if (!netdev->sm) {
}

At any rate I just sent a new patch with the suggested addition of registering after freeing in netdev_ft_reassociate. I can test that this afternoon, or try the other approach as well.

thanks,
Jeremy

> 
> https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/src/netdev.c#n4279
> 
> For reference, this is the CI output with this patch:
> 
> **Autotest Runner**
> Test ID: testrunner
> Desc: Runs IWD's autotest framework
> Duration: 1868.49 seconds
> **Result: FAIL**
> 
> Output:
> ```
> testFT-8021x-roam,testFT-FILS,testNetconfigRoam,testPSK-roam,testSAE-roam
> 
> Thanks,
> 
> James
>

Re: [PATCH] Register EAPOL frame listeners earlier

[James Prestwood]

Hi Jeremy,

On 3/27/24 11:53 AM, Jeremy Whiting wrote:
> On Wednesday, March 27, 2024 05:51 AM MDT, James Prestwood <prestwoj@gmail.com> wrote:
>
>> Hi Jeremy,
>>
>> On 3/26/24 4:11 PM, jeremy.whiting@collabora.com wrote:
>>> From: Ed Smith <ed.smith@collabora.com>
>>>
>>> If we register the main EAPOL frame listener as late as the associate
>>> event, it may not observe ptk_1_of_4. This defeats handling for early
>>> messages in eapol_rx_packet, which only sees messages once it has been
>>> registered.
>>>
>>> If we move registration to the authenticate event, then the EAPOL
>>> frame listeners should observe all messages, without any possible
>>> races. Note that the messages are not actually processed until
>>> eapol_start() is called, and we haven't moved that call site. All
>>> that's changing here is how early EAPOL messages can be observed.
>>> ---
>>>    src/netdev.c | 10 ++++++----
>>>    1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/netdev.c b/src/netdev.c
>>> index 09fac959..fc84c398 100644
>>> --- a/src/netdev.c
>>> +++ b/src/netdev.c
>>> @@ -2982,8 +2982,13 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
>>>    						NULL, netdev->user_data);
>>>    
>>>    		/* We have sent another CMD_AUTHENTICATE / CMD_ASSOCIATE */
>>> -		if (ret == 0 || ret == -EAGAIN)
>>> +		if (ret == 0 || ret == -EAGAIN) {
>>> +			if (!netdev->sm) {
>>> +				netdev->sm = eapol_sm_new(netdev->handshake);
>>> +				eapol_register(netdev->sm);
>>> +			}
>>>    			return;
>>> +		}
>>>    
>>>    		retry = kernel_will_retry_auth(status_code,
>>>    				L_CPU_TO_LE16(auth->algorithm),
>>> @@ -3099,9 +3104,6 @@ static void netdev_associate_event(struct l_genl_msg *msg,
>>>    			netdev->ap = NULL;
>>>    		}
>>>    
>>> -		netdev->sm = eapol_sm_new(netdev->handshake);
>>> -		eapol_register(netdev->sm);
>>> -
>>>    		/* Just in case this was a retry */
>>>    		netdev->ignore_connect_event = false;
>>>    
>> I need to have the CI email the original sender so you get notified, but
>> as Denis mentioned this still breaks FT because IWD does not use the
>> conventional CMD_AUTHENTICATE and instead sends an auth frame using
>> CMD_FRAME. This means that there is no authenticate event at all when FT
>> is involved. Because of this, no SM gets created (with this patch) which
>> breaks rekeys.
>>
>> After FT authentication netdev_ft_reassociate() is called which issues
>> the CMD_CONNECT call (this triggers association). So I think you would
>> need to both create the SM similar to where you did here, as well as
>> after it gets destroyed in here:
> I think I see. I've got a couple of questions though.
>
> 1. How if possible can I run the CI tests locally?

I'll be the first to admit, the documentation isn't great. But:

https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/doc/test-runner.txt

> 2. Should I instead just change the eapol_register that we removed from _associate_event to keep doing the _register if netdev->sm is null?
> i.e. Change the patch from removing the 2 lines to wrapping them in
> if (!netdev->sm) {
> }

That also could work. I would just comment it, since its really just to 
serve FT. Explicitly creating it in netdev_ft_reassociate may be more 
clear. Maybe Denis has a preference.

>
> At any rate I just sent a new patch with the suggested addition of registering after freeing in netdev_ft_reassociate. I can test that this afternoon, or try the other approach as well.
>
> thanks,
> Jeremy
>
>> https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/src/netdev.c#n4279
>>
>> For reference, this is the CI output with this patch:
>>
>> **Autotest Runner**
>> Test ID: testrunner
>> Desc: Runs IWD's autotest framework
>> Duration: 1868.49 seconds
>> **Result: FAIL**
>>
>> Output:
>> ```
>> testFT-8021x-roam,testFT-FILS,testNetconfigRoam,testPSK-roam,testSAE-roam
>>
>> Thanks,
>>
>> James
>>

[PATCH] Register EAPOL frame listeners earlier

[jeremy.whiting]

From: Ed Smith <ed.smith@collabora.com>

If we register the main EAPOL frame listener as late as the associate
event, it may not observe ptk_1_of_4. This defeats handling for early
messages in eapol_rx_packet, which only sees messages once it has been
registered.

If we move registration to the authenticate event, then the EAPOL
frame listeners should observe all messages, without any possible
races. Note that the messages are not actually processed until
eapol_start() is called, and we haven't moved that call site. All
that's changing here is how early EAPOL messages can be observed.
---
 src/netdev.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index 09fac959..886a85f5 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -2896,6 +2896,14 @@ static bool kernel_will_retry_auth(uint16_t status_code,
 	return false;
 }
 
+static void netdev_ensure_registered(struct netdev *netdev)
+{
+	if (!netdev->sm) {
+		netdev->sm = eapol_sm_new(netdev->handshake);
+		eapol_register(netdev->sm);
+	}
+}
+
 static void netdev_authenticate_event(struct l_genl_msg *msg,
 							struct netdev *netdev)
 {
@@ -2982,8 +2990,10 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
 						NULL, netdev->user_data);
 
 		/* We have sent another CMD_AUTHENTICATE / CMD_ASSOCIATE */
-		if (ret == 0 || ret == -EAGAIN)
+		if (ret == 0 || ret == -EAGAIN) {
+			netdev_ensure_registered(netdev);
 			return;
+		}
 
 		retry = kernel_will_retry_auth(status_code,
 				L_CPU_TO_LE16(auth->algorithm),
@@ -3099,9 +3109,6 @@ static void netdev_associate_event(struct l_genl_msg *msg,
 			netdev->ap = NULL;
 		}
 
-		netdev->sm = eapol_sm_new(netdev->handshake);
-		eapol_register(netdev->sm);
-
 		/* Just in case this was a retry */
 		netdev->ignore_connect_event = false;
 
@@ -4279,6 +4286,8 @@ int netdev_ft_reassociate(struct netdev *netdev,
 	if (netdev->sm) {
 		eapol_sm_free(netdev->sm);
 		netdev->sm = NULL;
+
+		netdev_ensure_registered(netdev);
 	}
 
 	msg = netdev_build_cmd_associate_common(netdev);
-- 
2.44.0

Re: [PATCH] Register EAPOL frame listeners earlier

[Denis Kenzior]

Hi Jeremy/Ed,

On 3/27/24 13:49, jeremy.whiting@collabora.com wrote:
> From: Ed Smith <ed.smith@collabora.com>
> 
> If we register the main EAPOL frame listener as late as the associate
> event, it may not observe ptk_1_of_4. This defeats handling for early
> messages in eapol_rx_packet, which only sees messages once it has been
> registered.
> 
> If we move registration to the authenticate event, then the EAPOL
> frame listeners should observe all messages, without any possible
> races. Note that the messages are not actually processed until
> eapol_start() is called, and we haven't moved that call site. All
> that's changing here is how early EAPOL messages can be observed.
> ---
>   src/netdev.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
> 

Can we please use patch versioning?  I can't keep track which version of this 
patch I'm looking at now.

> diff --git a/src/netdev.c b/src/netdev.c
> index 09fac959..886a85f5 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -2896,6 +2896,14 @@ static bool kernel_will_retry_auth(uint16_t status_code,
>   	return false;
>   }
>   
> +static void netdev_ensure_registered(struct netdev *netdev)

The naming could use some work.  What is being registered?

> +{
> +	if (!netdev->sm) {

I would do:

if (L_WARN_ON(netdev->sm))
	return;

....

> +		netdev->sm = eapol_sm_new(netdev->handshake);
> +		eapol_register(netdev->sm);
> +	}
> +}
> +
>   static void netdev_authenticate_event(struct l_genl_msg *msg,
>   							struct netdev *netdev)
>   {
> @@ -2982,8 +2990,10 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
>   						NULL, netdev->user_data);
>   
>   		/* We have sent another CMD_AUTHENTICATE / CMD_ASSOCIATE */
> -		if (ret == 0 || ret == -EAGAIN)
> +		if (ret == 0 || ret == -EAGAIN) {
> +			netdev_ensure_registered(netdev);

Nit: The whole point of EAGAIN is that we're not proceeding to the next stage. 
See src/auth-proto.h.  You should only be trying to create eapol_sm when we do.

>   			return;
> +		}
>   
>   		retry = kernel_will_retry_auth(status_code,
>   				L_CPU_TO_LE16(auth->algorithm),

Regards,
-Denis

[PATCH] Register EAPOL frame listeners earlier

[jeremy.whiting]

From: Ed Smith <ed.smith@collabora.com>

If we register the main EAPOL frame listener as late as the associate
event, it may not observe ptk_1_of_4. This defeats handling for early
messages in eapol_rx_packet, which only sees messages once it has been
registered.

If we move registration to the authenticate event, then the EAPOL
frame listeners should observe all messages, without any possible
races. Note that the messages are not actually processed until
eapol_start() is called, and we haven't moved that call site. All
that's changing here is how early EAPOL messages can be observed.
---
 src/netdev.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index 09fac959..d6dc7004 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -3011,6 +3011,11 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
 		}
 	}
 
+	if (!netdev->sm) {
+		netdev->sm = eapol_sm_new(netdev->handshake);
+		eapol_register(netdev->sm);
+	}
+
 auth_error:
 	netdev_connect_failed(netdev, NETDEV_RESULT_AUTHENTICATION_FAILED,
 				status_code);
@@ -3099,9 +3104,6 @@ static void netdev_associate_event(struct l_genl_msg *msg,
 			netdev->ap = NULL;
 		}
 
-		netdev->sm = eapol_sm_new(netdev->handshake);
-		eapol_register(netdev->sm);
-
 		/* Just in case this was a retry */
 		netdev->ignore_connect_event = false;
 
-- 
2.44.0

Re: [PATCH] Register EAPOL frame listeners earlier

[Jeremy Whiting]

Hello all,


Hold on this change. I just tested on the device itself and found with 
this change I'm unable to connect to either my usual wifi setup (home 
isp router) or the ios hotspot. Will discuss tomorrow with Ed but I 
think moving it after all the possible auth failures is making it never 
happen at all in some cases.


BR,

Jeremy

On 3/25/24 17:41, jeremy.whiting@collabora.com wrote:
> From: Ed Smith <ed.smith@collabora.com>
>
> If we register the main EAPOL frame listener as late as the associate
> event, it may not observe ptk_1_of_4. This defeats handling for early
> messages in eapol_rx_packet, which only sees messages once it has been
> registered.
>
> If we move registration to the authenticate event, then the EAPOL
> frame listeners should observe all messages, without any possible
> races. Note that the messages are not actually processed until
> eapol_start() is called, and we haven't moved that call site. All
> that's changing here is how early EAPOL messages can be observed.
> ---
>   src/netdev.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/netdev.c b/src/netdev.c
> index 09fac959..d6dc7004 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -3011,6 +3011,11 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
>   		}
>   	}
>   
> +	if (!netdev->sm) {
> +		netdev->sm = eapol_sm_new(netdev->handshake);
> +		eapol_register(netdev->sm);
> +	}
> +
>   auth_error:
>   	netdev_connect_failed(netdev, NETDEV_RESULT_AUTHENTICATION_FAILED,
>   				status_code);
> @@ -3099,9 +3104,6 @@ static void netdev_associate_event(struct l_genl_msg *msg,
>   			netdev->ap = NULL;
>   		}
>   
> -		netdev->sm = eapol_sm_new(netdev->handshake);
> -		eapol_register(netdev->sm);
> -
>   		/* Just in case this was a retry */
>   		netdev->ignore_connect_event = false;
>   

Re: [PATCH] Register EAPOL frame listeners earlier

[Denis Kenzior]

Hi,

On 3/25/24 18:41, jeremy.whiting@collabora.com wrote:
> From: Ed Smith <ed.smith@collabora.com>
> 
> If we register the main EAPOL frame listener as late as the associate
> event, it may not observe ptk_1_of_4. This defeats handling for early
> messages in eapol_rx_packet, which only sees messages once it has been
> registered.
> 

Out of curiosity, is this driver using CONTROL_PORT_OVER_NL80211 ?

You should really talk to the vendor about getting the firmware fixed.  Why are 
EAPoL packets making it out before the Association event?

> If we move registration to the authenticate event, then the EAPOL
> frame listeners should observe all messages, without any possible
> races. Note that the messages are not actually processed until
> eapol_start() is called, and we haven't moved that call site. All
> that's changing here is how early EAPOL messages can be observed.
> ---
>   src/netdev.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/netdev.c b/src/netdev.c
> index 09fac959..d6dc7004 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -3011,6 +3011,11 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
>   		}
>   	}
>   
> +	if (!netdev->sm) {
> +		netdev->sm = eapol_sm_new(netdev->handshake);
> +		eapol_register(netdev->sm);
> +	}
> +

You can't really do this here.  In the case of SAE (wpa3-personal), a successful 
transition from authenticate -> associate returns early:

                 /* We have sent another CMD_AUTHENTICATE / CMD_ASSOCIATE */
                 if (ret == 0 || ret == -EAGAIN)
                         return;

It is probably a good idea to only create the eapol_sm state machine once the 
transition to Associate has happened (in other words, ret == 0).  However...

>   auth_error:
>   	netdev_connect_failed(netdev, NETDEV_RESULT_AUTHENTICATION_FAILED,
>   				status_code);
> @@ -3099,9 +3104,6 @@ static void netdev_associate_event(struct l_genl_msg *msg,
>   			netdev->ap = NULL;
>   		}
>   
> -		netdev->sm = eapol_sm_new(netdev->handshake);
> -		eapol_register(netdev->sm);
> -

netdev_authenticate_event doesn't cover FT cases, so most likely this simple 
removal here will result in FT being broken, or rather re-keys after FT.  Thus 
special care must be taken for FT.

>   		/* Just in case this was a retry */
>   		netdev->ignore_connect_event = false;
>   

Your original patch was took care of both cases, albeit in a brute force way.

Regards,
-Denis