| Message ID | 20240326231151.607163-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-ci-build | success | Build - Configure |
| prestwoj/iwd-alpine-ci-build | success | Build - Configure |
| prestwoj/iwd-ci-clang | success | clang PASS |
| prestwoj/iwd-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-alpine-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-ci-makecheck | success | Make Check |
| prestwoj/iwd-alpine-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-ci-testrunner | fail | test-runner - FAIL: testFT-8021x-roam,testFT-FILS,testNetconfigRoam,testPSK-roam,testSAE-roam |
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
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 >
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 >>
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;
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(-)