| Message ID | 20201215093455.51933-2-kazikcz@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] hostapd: fix dpp_listen in dpp responder scenario | expand |
On Tue, Dec 15, 2020 at 09:34:55AM +0000, Michal Kazior wrote: > Some time ago it was found some drivers are > setting their hw/ucode rx filters restrictively > enough to prevent DPP multicast frames from being > delivered. > > A set of patches was introduced to the kernel and > ath9k driver as well as hostapd, eg. > > a39e9af90 ("nl80211: DPP listen mode callback") > 4d2ec436e ("DPP: Add driver operation for enabling/disabling listen mode") So all this applies to patch 1/2 where hostapd side was not previously covered. However, I'm not sure I understand why that description is here in patch 2/2 as well.. > Turns out chirping was omitted. As such chirping > wasn't working on drivers with restrictive rx > filters. This sounds strange.. The response to chirping would be a unicast frame, so that hardware/firmware RX filter change should not have any impact here. > diff --git a/wpa_supplicant/dpp_supplicant.c b/wpa_supplicant/dpp_supplicant.c > @@ -3420,6 +3420,7 @@ static void wpas_dpp_chirp_tx_status(struct wpa_supplicant *wpa_s, > wpa_printf(MSG_DEBUG, "DPP: Chirp send completed - wait for response"); > + wpas_dpp_listen_start(wpa_s, freq); This looks quite incorrect thing to do.. This function is called when the TX operation with 2000 ms wait for response gets TX status. It would not be appropriate to start DPP listen while that 2000 ms wait for the response is ongoing. Could you please provide more details on what exactly this is trying to do and what kind of a frame is not received without this addition?
On Sat, Feb 6, 2021 at 3:14 PM Jouni Malinen <j@w1.fi> wrote: > > On Tue, Dec 15, 2020 at 09:34:55AM +0000, Michal Kazior wrote: > > Some time ago it was found some drivers are > > setting their hw/ucode rx filters restrictively > > enough to prevent DPP multicast frames from being > > delivered. > > > > A set of patches was introduced to the kernel and > > ath9k driver as well as hostapd, eg. > > > > a39e9af90 ("nl80211: DPP listen mode callback") > > 4d2ec436e ("DPP: Add driver operation for enabling/disabling listen mode") > > So all this applies to patch 1/2 where hostapd side was not previously > covered. However, I'm not sure I understand why that description is here > in patch 2/2 as well.. I felt it's similar enough it warrants the same context to be provided. > > Turns out chirping was omitted. As such chirping > > wasn't working on drivers with restrictive rx > > filters. > > This sounds strange.. The response to chirping would be a unicast frame, > so that hardware/firmware RX filter change should not have any impact > here. Now that you say it, it would make sense to expect a unicast frame to be sent back to the chirping station. But that's not what is happening. This is output from a script I used to test the problem. It prefixes prints from wpa_s and hostapd, and you can see hostapd is responding with a type=0 bcast. > wpas wlp11s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2437 type=13 > wpas wlp11s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=5745 type=13 > wpas wlp11s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2412 type=13 > hapd wlp2s0: DPP-RX src=2c:d0:5a:66:88:74 freq=2412 type=13 > hapd wlp2s0: DPP-CHIRP-RX id=1 src=2c:d0:5a:66:88:74 freq=2412 hash=c4919f3a9600ad74f07614e2efcb63879bc6dbebf57a55a31dfcea9514ecc9d5 > hapd wlp2s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2412 type=0 > wpas wlp11s0: DPP-RX src=9c:b7:0d:43:77:48 freq=2412 type=0 > wpas wlp11s0: DPP-CHIRP-STOPPED > wpas wlp11s0: DPP-TX dst=9c:b7:0d:43:77:48 freq=2412 type=1 > hapd wlp2s0: DPP-RX src=2c:d0:5a:66:88:74 freq=2412 type=1 > hapd wlp2s0: DPP-AUTH-DIRECTION mutual=0 > hapd wlp2s0: DPP-TX dst=2c:d0:5a:66:88:74 freq=2412 type=2 > hapd wlp2s0: DPP-TX-STATUS dst=2c:d0:5a:66:88:74 result=SUCCESS > hapd wlp2s0: DPP-AUTH-SUCCESS init=1 > hapd wlp2s0: DPP-CONF-REQ-RX src=2c:d0:5a:66:88:74 > hapd wlp2s0: DPP-BAND-SUPPORT 81,83,84,115,116,117,118,119,120,121,122,123,124,125,126,127 > hapd wlp2s0: DPP-RX src=2c:d0:5a:66:88:74 freq=2412 type=11 > hapd wlp2s0: DPP-CONF-SENT (snip) This is buffered a bit but it is roughly chronological. Without this patch what I see is this: > wpas wlp11s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2437 type=13 > hapd wlp2s0: DPP-RX src=2c:d0:5a:66:88:74 freq=2412 type=13 > hapd wlp2s0: DPP-CHIRP-RX id=1 src=2c:d0:5a:66:88:74 freq=2412 hash=5da8e3eaea337eca38b78a12787e83382b99628a16f675003757ed73720f1318 > hapd wlp2s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2412 type=0 > wpas wlp11s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=5745 type=13 > wpas wlp11s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2412 type=13 > hapd wlp2s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2412 type=0 > hapd nl80211: kernel reports: Match already configured > hapd wlp2s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2412 type=0 > hapd nl80211: kernel reports: Match already configured > hapd wlp2s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2412 type=0 > hapd nl80211: kernel reports: Match already configured > wpas wlp11s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2437 type=13 > wpas wlp11s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=5745 type=13 > wpas wlp11s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2412 type=13 > hapd wlp2s0: DPP-TX dst=ff:ff:ff:ff:ff:ff freq=2412 type=0 > hapd nl80211: kernel reports: Match already configured > hapd DPP: No response received from responder - stopping initiation attempt > hapd wlp2s0: DPP-AUTH-INIT-FAILED > > diff --git a/wpa_supplicant/dpp_supplicant.c b/wpa_supplicant/dpp_supplicant.c > > @@ -3420,6 +3420,7 @@ static void wpas_dpp_chirp_tx_status(struct wpa_supplicant *wpa_s, > > wpa_printf(MSG_DEBUG, "DPP: Chirp send completed - wait for response"); > > + wpas_dpp_listen_start(wpa_s, freq); > > This looks quite incorrect thing to do.. This function is called when > the TX operation with 2000 ms wait for response gets TX status. It would > not be appropriate to start DPP listen while that 2000 ms wait for the > response is ongoing. Do you mean that it's racy? I guess it is... The response may already be lost when tx-status for the chirp is processed. Or do you mean something else here? Michał
On Mon, Feb 08, 2021 at 03:20:37PM +0100, Michal Kazior wrote: > On Sat, Feb 6, 2021 at 3:14 PM Jouni Malinen <j@w1.fi> wrote: > > This sounds strange.. The response to chirping would be a unicast frame, > > so that hardware/firmware RX filter change should not have any impact > > here. > > Now that you say it, it would make sense to expect a unicast frame to > be sent back to the chirping station. But that's not what is > happening. > > This is output from a script I used to test the problem. It prefixes > prints from wpa_s and hostapd, and you can see hostapd is responding > with a type=0 bcast. That's a bug.. I'll fix it so that the Authentication Request frame goes out as a unicast frame to the source address of the Presence Announcement which is what the tech spec defines for this sequence. > > > diff --git a/wpa_supplicant/dpp_supplicant.c b/wpa_supplicant/dpp_supplicant.c > > > @@ -3420,6 +3420,7 @@ static void wpas_dpp_chirp_tx_status(struct wpa_supplicant *wpa_s, > > > wpa_printf(MSG_DEBUG, "DPP: Chirp send completed - wait for response"); > > > + wpas_dpp_listen_start(wpa_s, freq); > > > > This looks quite incorrect thing to do.. This function is called when > > the TX operation with 2000 ms wait for response gets TX status. It would > > not be appropriate to start DPP listen while that 2000 ms wait for the > > response is ongoing. > > Do you mean that it's racy? I guess it is... The response may already > be lost when tx-status for the chirp is processed. Or do you mean > something else here? Starting listen mode (ROC in mac80211) while already in offchannel-TX-with-wait-for-response (also ROC) does not sound good since there are constraints on how mac80211 allows ROCs to be combined or extended. This would also be a race condition since this call happens from TX status handler and the Authentication Request frame may have already been transmitted, but this was not my main concern (but it sounds like a valid concern). Since there are cases where the Authentication Request frame might still be sent to the broadcast address while the Enrollee is in the chirping loop, it would make sense to update the hardware RX filters. However, this should be done without introducing a separate listen operation. Wouldn't a call to wpa_drv_dpp_listen(wpa_s, true) when starting chirping be all that's needed here? That would then need to be stopped when chirping state terminates.
On Tue, Feb 9, 2021 at 7:40 PM Jouni Malinen <j@w1.fi> wrote: > On Mon, Feb 08, 2021 at 03:20:37PM +0100, Michal Kazior wrote: > > On Sat, Feb 6, 2021 at 3:14 PM Jouni Malinen <j@w1.fi> wrote: > > > This sounds strange.. The response to chirping would be a unicast frame, > > > so that hardware/firmware RX filter change should not have any impact > > > here. > > > > Now that you say it, it would make sense to expect a unicast frame to > > be sent back to the chirping station. But that's not what is > > happening. > > > > This is output from a script I used to test the problem. It prefixes > > prints from wpa_s and hostapd, and you can see hostapd is responding > > with a type=0 bcast. > > That's a bug.. I'll fix it so that the Authentication Request frame goes > out as a unicast frame to the source address of the Presence > Announcement which is what the tech spec defines for this sequence. Thanks! > > > > diff --git a/wpa_supplicant/dpp_supplicant.c b/wpa_supplicant/dpp_supplicant.c > > > > @@ -3420,6 +3420,7 @@ static void wpas_dpp_chirp_tx_status(struct wpa_supplicant *wpa_s, > > > > wpa_printf(MSG_DEBUG, "DPP: Chirp send completed - wait for response"); > > > > + wpas_dpp_listen_start(wpa_s, freq); > > > > > > This looks quite incorrect thing to do.. This function is called when > > > the TX operation with 2000 ms wait for response gets TX status. It would > > > not be appropriate to start DPP listen while that 2000 ms wait for the > > > response is ongoing. > > > > Do you mean that it's racy? I guess it is... The response may already > > be lost when tx-status for the chirp is processed. Or do you mean > > something else here? > > Starting listen mode (ROC in mac80211) while already in > offchannel-TX-with-wait-for-response (also ROC) does not sound good > since there are constraints on how mac80211 allows ROCs to be combined > or extended. Oh, you're right. > This would also be a race condition since this call happens from TX > status handler and the Authentication Request frame may have already > been transmitted, but this was not my main concern (but it sounds like a > valid concern). > > Since there are cases where the Authentication Request frame might still > be sent to the broadcast address while the Enrollee is in the chirping > loop, it would make sense to update the hardware RX filters. However, > this should be done without introducing a separate listen operation. > Wouldn't a call to wpa_drv_dpp_listen(wpa_s, true) when starting > chirping be all that's needed here? That would then need to be stopped > when chirping state terminates. I think this is what I tried originally, but apparently didn't go that route for some reason that I can't remember anymore. I'll see what I can do. Michał
diff --git a/wpa_supplicant/dpp_supplicant.c b/wpa_supplicant/dpp_supplicant.c index 09355cf7f..0eef64ca3 100644 --- a/wpa_supplicant/dpp_supplicant.c +++ b/wpa_supplicant/dpp_supplicant.c @@ -3420,6 +3420,7 @@ static void wpas_dpp_chirp_tx_status(struct wpa_supplicant *wpa_s, } wpa_printf(MSG_DEBUG, "DPP: Chirp send completed - wait for response"); + wpas_dpp_listen_start(wpa_s, freq); if (eloop_register_timeout(2, 0, wpas_dpp_chirp_timeout, wpa_s, NULL) < 0) wpas_dpp_chirp_stop(wpa_s);