Re: [PATCH] Register EAPOL frame listeners earlier

[Denis Kenzior <denkenz@gmail.com>]

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