| Message ID | 20240327184927.677804-1-jeremy.whiting@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Register EAPOL frame listeners earlier | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-alpine-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-gitlint | success | GitLint |
| prestwoj/iwd-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-incremental_build | success | Incremental build not run PASS |
| prestwoj/iwd-alpine-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-alpine-ci-incremental_build | success | Incremental build not run PASS |
| prestwoj/iwd-alpine-ci-build | success | Build - Configure |
| prestwoj/iwd-ci-build | success | Build - Configure |
| prestwoj/iwd-alpine-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-alpine-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-clang | success | clang PASS |
| prestwoj/iwd-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-ci-testrunner | success | test-runner PASS |
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
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);
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(-)