[v3] ft: fix double free when disconnecting mid-FT

Commit Message

James Prestwood April 24, 2023, 1:40 p.m. UTC

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. In addition the l_queue_remove inside the
foreach callback is not a safe operation either.

To fix this don't remove the ft_info inside the offchannel
destroy callback. The info will get freed by ft_associate regardless
of the outcome (parsed or !parsed). This is also consistent with
how the onchannel logic works.

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 | 5 -----
 1 file changed, 5 deletions(-)

v3:
 * Addressed Denis' comments about removing a list item during a foreach
   callback. This left a simpler option to not remove the item at all
   during the offchannel callback and left ft_associate() handle it.

Comments

Denis Kenzior April 30, 2023, 6 p.m. UTC | #1

Hi James,

On 4/24/23 08:40, 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. In addition the l_queue_remove inside the
> foreach callback is not a safe operation either.
> 
> To fix this don't remove the ft_info inside the offchannel
> destroy callback. The info will get freed by ft_associate regardless
> of the outcome (parsed or !parsed). This is also consistent with
> how the onchannel logic works.
> 
> 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 | 5 -----
>   1 file changed, 5 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

Patch

diff --git a/src/ft.c b/src/ft.c
index 8ae683a7..62fb0ae5 100644
--- a/src/ft.c
+++ b/src/ft.c
@@ -1060,11 +1060,6 @@  static void ft_authenticate_destroy(int error, void *user_data)
 	struct ft_info *info = user_data;
 
 	info->offchannel_id = 0;
-
-	if (!info->parsed) {
-		l_queue_remove(info_list, info);
-		ft_info_destroy(info);
-	}
 }
 
 /*