9:24 p.m.
Previously we added logic to defer doing anything in ap_free() to after
the AP event handler has returned so that ap_event() has a chance to
inform whoever called it that the ap_state has been freed. But there's
also a chance that the event handler is destroying both the AP and the
netdev it runs on, so after the handler has returned we can't even use
netdev_get_wdev_id or netdev_get_ifindex. The easiest solution seems to
be to call ap_reset() in ap_free() even if we're within an event handler
to ensure we no longer need any external objects. Also make sure
ap_reset() can be called multiple times.
Another option would be to watch for NETDEV_WATCH_EVENT_DEL and remove
our reference to the netdev (because there's no need actually call
l_rtnl_ifaddr_delete or frame_watch_wdev_remove if the netdev was
destroyed -- frame_watch already tracks netdev removals), or to save
just the ifindex and the wdev id...
---
src/ap.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/src/ap.c b/src/ap.c
index 32e45865..6f3dfa60 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -207,16 +207,22 @@ static void ap_reset(struct ap_state *ap)
ap->authorized_macs_num = 0;
}
- if (ap->mlme_watch)
+ if (ap->mlme_watch) {
l_genl_family_unregister(ap->nl80211, ap->mlme_watch);
+ ap->mlme_watch = 0;
+ }
frame_watch_wdev_remove(netdev_get_wdev_id(netdev));
- if (ap->start_stop_cmd_id)
+ if (ap->start_stop_cmd_id) {
l_genl_family_cancel(ap->nl80211, ap->start_stop_cmd_id);
+ ap->start_stop_cmd_id = 0;
+ }
- if (ap->rtnl_add_cmd)
+ if (ap->rtnl_add_cmd) {
l_netlink_cancel(rtnl, ap->rtnl_add_cmd);
+ ap->rtnl_add_cmd = 0;
+ }
if (ap->rtnl_get_gateway4_mac_cmd) {
l_netlink_cancel(rtnl, ap->rtnl_get_gateway4_mac_cmd);
@@ -228,12 +234,12 @@ static void ap_reset(struct ap_state *ap)
ap->rtnl_get_dns4_mac_cmd = 0;
}
- l_queue_destroy(ap->sta_states, ap_sta_free);
+ l_queue_destroy(l_steal_ptr(ap->sta_states), ap_sta_free);
if (ap->rates)
- l_uintset_free(ap->rates);
+ l_uintset_free(l_steal_ptr(ap->rates));
- l_queue_destroy(ap->wsc_pbc_probes, l_free);
+ l_queue_destroy(l_steal_ptr(ap->wsc_pbc_probes), l_free);
l_timeout_remove(ap->wsc_pbc_timeout);
ap->started = false;
@@ -258,7 +264,8 @@ static bool ap_event_done(struct ap_state *ap, bool prev_in_event)
ap->in_event = prev_in_event;
if (!prev_in_event && ap->free_pending) {
- ap_free(ap);
+ l_genl_family_free(ap->nl80211);
+ l_free(ap);
return true;
}
@@ -3386,12 +3393,13 @@ free_ap:
/* Free @ap without a graceful shutdown */
void ap_free(struct ap_state *ap)
{
+ ap_reset(ap);
+
if (ap->in_event) {
ap->free_pending = true;
return;
}
- ap_reset(ap);
l_genl_family_free(ap->nl80211);
l_free(ap);
}
--
2.32.0
9:24 p.m.
New subject: [PATCH 2/3] handshake: Allow event handler to free handshake
Like in ap.c, allow the event callback to mark the handshake state as
destroyed, without causing invalid accesses after the callback has
returned. In this case the crash was because try_handshake_complete
needed to access members of handshake_state after emitting the event,
as well as access the netdev, which also has been destroyed:
==257707== Invalid read of size 8
==257707== at 0x408C85: try_handshake_complete (netdev.c:1487)
==257707== by 0x408C85: try_handshake_complete (netdev.c:1480)
(...)
==257707== Address 0x4e187e8 is 856 bytes inside a block of size 872 free'd
==257707== at 0x484621F: free (in
/usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==257707== by 0x437887: ap_stop_handshake (ap.c:151)
==257707== by 0x439793: ap_del_station (ap.c:316)
==257707== by 0x43EA92: ap_station_disconnect (ap.c:3411)
==257707== by 0x43EA92: ap_station_disconnect (ap.c:3399)
==257707== by 0x454276: p2p_group_event (p2p.c:1006)
==257707== by 0x439147: ap_event (ap.c:281)
==257707== by 0x4393AB: ap_new_rsna (ap.c:390)
==257707== by 0x4393AB: ap_handshake_event (ap.c:1010)
==257707== by 0x408C7F: try_handshake_complete (netdev.c:1485)
==257707== by 0x408C7F: try_handshake_complete (netdev.c:1480)
(...)
---
src/handshake.c | 5 +++++
src/handshake.h | 24 ++++++++++++++++++------
src/netdev.c | 16 +++++++++++-----
3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/src/handshake.c b/src/handshake.c
index 60ec0969..734e997c 100644
--- a/src/handshake.c
+++ b/src/handshake.c
@@ -107,6 +107,11 @@ void handshake_state_free(struct handshake_state *s)
{
__typeof__(s->free) destroy = s->free;
+ if (s->in_event) {
+ s->in_event = false;
+ return;
+ }
+
l_free(s->authenticator_ie);
l_free(s->supplicant_ie);
l_free(s->authenticator_rsnxe);
diff --git a/src/handshake.h b/src/handshake.h
index 6d56dadd..34d4829d 100644
--- a/src/handshake.h
+++ b/src/handshake.h
@@ -159,17 +159,29 @@ struct handshake_state {
void *user_data;
void (*free)(struct handshake_state *s);
+ bool in_event;
handshake_event_func_t event_func;
};
-#define handshake_event(hs, event, ...) \
- do { \
- if (!(hs)->event_func) \
- break; \
+#define handshake_event(_hs, event, ...) \
+ (__extension__ ({ \
+ struct handshake_state *hs = (_hs); \
+ bool freed = false; \
\
- (hs)->event_func((hs), event, (hs)->user_data, ##__VA_ARGS__); \
- } while (0)
+ if (hs->event_func && !hs->in_event) { \
+ hs->in_event = true; \
+ hs->event_func(hs, event, hs->user_data, \
+ ##__VA_ARGS__); \
+ \
+ if (!hs->in_event) { \
+ handshake_state_free(hs); \
+ freed = true; \
+ } else \
+ hs->in_event = false; \
+ } \
+ freed; \
+ }))
void handshake_state_free(struct handshake_state *s);
diff --git a/src/netdev.c b/src/netdev.c
index b311c0ee..4dc9ce94 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -1482,7 +1482,9 @@ static void try_handshake_complete(struct netdev_handshake_state
*nhs)
if (nhs->ptk_installed && nhs->gtk_installed &&
nhs->igtk_installed &&
!nhs->complete) {
nhs->complete = true;
- handshake_event(&nhs->super, HANDSHAKE_EVENT_COMPLETE);
+
+ if (handshake_event(&nhs->super, HANDSHAKE_EVENT_COMPLETE))
+ return;
if (nhs->netdev->type == NL80211_IFTYPE_STATION ||
nhs->netdev->type == NL80211_IFTYPE_P2P_CLIENT)
@@ -2006,7 +2008,9 @@ static void netdev_group_timeout_cb(struct l_timeout *timeout, void
*user_data)
nhs->netdev->index);
nhs->complete = true;
- handshake_event(&nhs->super, HANDSHAKE_EVENT_COMPLETE);
+
+ if (handshake_event(&nhs->super, HANDSHAKE_EVENT_COMPLETE))
+ return;
netdev_connect_ok(nhs->netdev);
}
@@ -2155,7 +2159,9 @@ static void netdev_set_pmk_cb(struct l_genl_msg *msg, void
*user_data)
return;
}
- handshake_event(netdev->handshake, HANDSHAKE_EVENT_SETTING_KEYS);
+ if (handshake_event(netdev->handshake, HANDSHAKE_EVENT_SETTING_KEYS))
+ return;
+
netdev_connect_ok(netdev);
}
@@ -2906,8 +2912,8 @@ process_resp_ies:
}
/* Allow station to sync the PSK to disk */
- if (is_offload(hs))
- handshake_event(hs, HANDSHAKE_EVENT_SETTING_KEYS);
+ if (is_offload(hs) && handshake_event(hs, HANDSHAKE_EVENT_SETTING_KEYS))
+ return;
netdev_connect_ok(netdev);
return;
--
2.32.0
9:24 p.m.
New subject: [PATCH 3/3] p2p: Fix ie_tlv_extract_wfd_payload call typo
---
WFD couldn't have worked with this typo, I must have introduced it
when generating patches somehow.
src/p2p.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/p2p.c b/src/p2p.c
index a66f71e2..4c90efb6 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -938,7 +938,7 @@ static void p2p_group_event(enum ap_event_type type, const void
*event_data,
p2p_clear_association_req(&req_info);
if (dev->conn_own_wfd)
- wfd_data = ie_tlv_extract_p2p_payload(data->assoc_ies,
+ wfd_data = ie_tlv_extract_wfd_payload(data->assoc_ies,
data->assoc_ies_len,
&wfd_data_len);
--
2.32.0
1:54 a.m.
Hi Andrew,
On 1/21/22 04:24, Andrew Zaborowski wrote:
Previously we added logic to defer doing anything in ap_free() to
after
the AP event handler has returned so that ap_event() has a chance to
inform whoever called it that the ap_state has been freed. But there's
also a chance that the event handler is destroying both the AP and the
netdev it runs on, so after the handler has returned we can't even use
netdev_get_wdev_id or netdev_get_ifindex. The easiest solution seems to
be to call ap_reset() in ap_free() even if we're within an event handler
to ensure we no longer need any external objects. Also make sure
ap_reset() can be called multiple times.
Another option would be to watch for NETDEV_WATCH_EVENT_DEL and remove
our reference to the netdev (because there's no need actually call
l_rtnl_ifaddr_delete or frame_watch_wdev_remove if the netdev was
destroyed -- frame_watch already tracks netdev removals), or to save
just the ifindex and the wdev id...
---
src/ap.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
All applied, thanks.
Regards,
-Denis
240
days inactive
240
days old
3 comments
2 participants
participants (2)
-
Andrew Zaborowski -
Denis Kenzior