| Message ID | 20230419203033.26365-1-prestwoj@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ft: fix double free when disconnecting mid-FT | expand |
Hi James, On 4/19/23 15:30, James Prestwood wrote: > If IWD gets a disconnect during FT the roaming state will be > cleared, as well as any ft_info's during ft_clear_authentications. > This includes canceling the offchannel operation which also > destroys any pending ft_info's if !info->parsed. This causes a > double free afterwards. Knowning this, check the status of the > ft_info and only free if it has been parsed. > > Log and crash backtrace below: > > iwd[488]: src/station.c:station_try_next_transition() 5, target aa:46:8d:37:7c:87 > iwd[488]: src/wiphy.c:wiphy_radio_work_insert() Inserting work item 16668 > iwd[488]: src/wiphy.c:wiphy_radio_work_insert() Inserting work item 16669 > iwd[488]: src/wiphy.c:wiphy_radio_work_done() Work item 16667 done > iwd[488]: src/wiphy.c:wiphy_radio_work_next() Starting work item 16668 > iwd[488]: src/netdev.c:netdev_mlme_notify() MLME notification Remain on Channel(55) > iwd[488]: src/netdev.c:netdev_mlme_notify() MLME notification Del Station(20) > iwd[488]: src/netdev.c:netdev_link_notify() event 16 on ifindex 5 > iwd[488]: src/netdev.c:netdev_mlme_notify() MLME notification Deauthenticate(39) > iwd[488]: src/netdev.c:netdev_deauthenticate_event() > iwd[488]: src/netdev.c:netdev_mlme_notify() MLME notification Disconnect(48) > iwd[488]: src/netdev.c:netdev_disconnect_event() > iwd[488]: Received Deauthentication event, reason: 6, from_ap: true > iwd[488]: src/station.c:station_disconnect_event() 5 > iwd[488]: src/station.c:station_disassociated() 5 > iwd[488]: src/station.c:station_reset_connection_state() 5 > iwd[488]: src/station.c:station_roam_state_clear() 5 > iwd[488]: double free or corruption (fasttop) > > 5 0x0000555b3dbf44a4 in ft_info_destroy () > 6 0x0000555b3dbf45b3 in remove_ifindex () > 7 0x0000555b3dc4653c in l_queue_foreach_remove () > 8 0x0000555b3dbd0dd1 in station_reset_connection_state () > 9 0x0000555b3dbd37e5 in station_disassociated () > 10 0x0000555b3dbc8bb8 in netdev_mlme_notify () > 11 0x0000555b3dc4e80b in received_data () > 12 0x0000555b3dc4b430 in io_callback () > 13 0x0000555b3dc4a5ed in l_main_iterate () > 14 0x0000555b3dc4a6bc in l_main_run () > 15 0x0000555b3dc4a8e0 in l_main_run_with_signal () > 16 0x0000555b3dbbe888 in main () > --- > src/ft.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > v2: > * This was missing the check for the offchannel_id as well. If there is > not a current offchannel operation the info needs to be freed as > well (not only when info->parsed == true). > > diff --git a/src/ft.c b/src/ft.c > index 8ae683a7..e7794bf5 100644 > --- a/src/ft.c > +++ b/src/ft.c > @@ -1175,6 +1175,7 @@ static bool remove_ifindex(void *data, void *user_data) > { > struct ft_info *info = data; > uint32_t ifindex = L_PTR_TO_UINT(user_data); > + bool need_free = info->parsed || !info->offchannel_id; If offchannel_id is true, then info->parsed is false, no? > > if (info->ifindex != ifindex) > return false; > @@ -1183,7 +1184,15 @@ static bool remove_ifindex(void *data, void *user_data) > offchannel_cancel(netdev_get_wdev_id(netdev_find(ifindex)), > info->offchannel_id); > > - ft_info_destroy(info); > + /* > + * If we hadn't yet got an authenticate response or there was something > + * wrong with parsing (i.e. !info->parsed) the offchannel destroy > + * callback will end up destroying the info. Otherwise we need to > + * destroy it here. > + */ > + if (need_free) > + ft_info_destroy(info); > + So I would write this as: if (info->offchannel_id) offchannel_cancel else ft_info_destroy(...); However, I'm pretty sure this patch is not fixing anything but just trading one double-free for another. offchannel_cancel results in ft_authenticate_destroy being called, which in turn results in l_queue_remove(). remove_ifindex() is called from inside l_queue_foreach_remove(), and you can't call l_queue_remove() on the same queue inside the foreach callback. Regards, -Denis
diff --git a/src/ft.c b/src/ft.c index 8ae683a7..e7794bf5 100644 --- a/src/ft.c +++ b/src/ft.c @@ -1175,6 +1175,7 @@ static bool remove_ifindex(void *data, void *user_data) { struct ft_info *info = data; uint32_t ifindex = L_PTR_TO_UINT(user_data); + bool need_free = info->parsed || !info->offchannel_id; if (info->ifindex != ifindex) return false; @@ -1183,7 +1184,15 @@ static bool remove_ifindex(void *data, void *user_data) offchannel_cancel(netdev_get_wdev_id(netdev_find(ifindex)), info->offchannel_id); - ft_info_destroy(info); + /* + * If we hadn't yet got an authenticate response or there was something + * wrong with parsing (i.e. !info->parsed) the offchannel destroy + * callback will end up destroying the info. Otherwise we need to + * destroy it here. + */ + if (need_free) + ft_info_destroy(info); + return true; }