| Message ID | 1423230267-19973-1-git-send-email-eduardo.abinader@openbossa.org |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Feb 06, 2015 at 09:44:27AM -0400, Eduardo Abinader wrote: > This patch avoids a segmentation fault that is occurring > when a removed pending p2p interface receives a delayed > scan result and crashes when handled in > wpa_supplicant_event_scan_results. > > The scenario being used is a request to pbc join an > autonomous p2p group. Would you have a debug log showing this crash? I find it a bit difficult to see why this would happen for two reasons: 1) the wpa_s instance is freed immediately after this call (i.e., there is no way a scan results could be processed for the interface) and 2) driver_nl80211 drops the scan result indication for the interface once it has been removed. > diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c > @@ -3675,13 +3675,13 @@ static void radio_remove_interface(struct wpa_supplicant *wpa_s) > - wpa_s->radio = NULL; > if (!dl_list_empty(&radio->ifaces)) > return; /* Interfaces remain for this radio */ > > wpa_printf(MSG_DEBUG, "Remove radio %s", radio->name); > eloop_cancel_timeout(radio_start_next_work, radio, NULL); > os_free(radio); > + wpa_s->radio = NULL; This radio_remove_interface() function is called only from wpa_supplicant_deinit_iface() and this call is followed by the driver interface deinit and os_free(wpa_s) before returning to eloop. In other words, there is no way this specific wpa_s instance could be used in wpa_supplicant_event_scan_results() after this unless there are some much more severe issues of continuing to use freed memory. Allowing wpa_s->radio to continue to point to the radio in the other-interfaces-remain case looks a bit suspicious since if there were actual real uses of the wpa_s instance after this, I don't see what would prevent that other remaining wpa_s instance from getting remove and deleting the radio instance. After that, the wpa_s->radio on the not-cleared-to-NULL interface would point to freed memory. Anyway, this path should obviously not happen either due to that os_free(wpa_s) following this call..
Hi Jouni, Please see coments below. On Fri, Feb 6, 2015 at 4:22 PM, Jouni Malinen <j@w1.fi> wrote: > On Fri, Feb 06, 2015 at 09:44:27AM -0400, Eduardo Abinader wrote: >> This patch avoids a segmentation fault that is occurring >> when a removed pending p2p interface receives a delayed >> scan result and crashes when handled in >> wpa_supplicant_event_scan_results. >> >> The scenario being used is a request to pbc join an >> autonomous p2p group. > > Would you have a debug log showing this crash? I find it a bit difficult > to see why this would happen for two reasons: 1) the wpa_s instance is > freed immediately after this call (i.e., there is no way a scan results > could be processed for the interface) and 2) driver_nl80211 drops the > scan result indication for the interface once it has been removed. > Log file is here: http://filebin.ca/1ql0zqDLMH1y/wpas_crash.log.gz >> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c >> @@ -3675,13 +3675,13 @@ static void radio_remove_interface(struct wpa_supplicant *wpa_s) >> - wpa_s->radio = NULL; >> if (!dl_list_empty(&radio->ifaces)) >> return; /* Interfaces remain for this radio */ >> >> wpa_printf(MSG_DEBUG, "Remove radio %s", radio->name); >> eloop_cancel_timeout(radio_start_next_work, radio, NULL); >> os_free(radio); >> + wpa_s->radio = NULL; > > This radio_remove_interface() function is called only from > wpa_supplicant_deinit_iface() and this call is followed by the driver > interface deinit and os_free(wpa_s) before returning to eloop. In other > words, there is no way this specific wpa_s instance could be used in > wpa_supplicant_event_scan_results() after this unless there are some > much more severe issues of continuing to use freed memory. > > Allowing wpa_s->radio to continue to point to the radio in the > other-interfaces-remain case looks a bit suspicious since if there were > actual real uses of the wpa_s instance after this, I don't see what > would prevent that other remaining wpa_s instance from getting remove > and deleting the radio instance. After that, the wpa_s->radio on the > not-cleared-to-NULL interface would point to freed memory. Anyway, this > path should obviously not happen either due to that os_free(wpa_s) > following this call.. Actually wpa_s->radio is only being freed, when no other interface is using it, like wlanX. The patches only ensures radio is to null, when freed. > > -- > Jouni Malinen PGP id EFC895FA > _______________________________________________ > HostAP mailing list > HostAP@lists.shmoo.com > http://lists.shmoo.com/mailman/listinfo/hostap
On Fri, Feb 06, 2015 at 04:45:21PM -0400, Eduardo Abinader wrote: > Log file is here: > > http://filebin.ca/1ql0zqDLMH1y/wpas_crash.log.gz Thanks! I was able to reproduce this and confirm that this is indeed due to use of freed memory after the P2P group interface gets removed. > >> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c > >> @@ -3675,13 +3675,13 @@ static void radio_remove_interface(struct wpa_supplicant *wpa_s) > >> - wpa_s->radio = NULL; > >> if (!dl_list_empty(&radio->ifaces)) > >> return; /* Interfaces remain for this radio */ > >> > >> wpa_printf(MSG_DEBUG, "Remove radio %s", radio->name); > >> eloop_cancel_timeout(radio_start_next_work, radio, NULL); > >> os_free(radio); > >> + wpa_s->radio = NULL; > > > > This radio_remove_interface() function is called only from > > wpa_supplicant_deinit_iface() and this call is followed by the driver > > interface deinit and os_free(wpa_s) before returning to eloop. In other > > words, there is no way this specific wpa_s instance could be used in > > wpa_supplicant_event_scan_results() after this unless there are some > > much more severe issues of continuing to use freed memory. > > > > Allowing wpa_s->radio to continue to point to the radio in the > > other-interfaces-remain case looks a bit suspicious since if there were > > actual real uses of the wpa_s instance after this, I don't see what > > would prevent that other remaining wpa_s instance from getting remove > > and deleting the radio instance. After that, the wpa_s->radio on the > > not-cleared-to-NULL interface would point to freed memory. Anyway, this > > path should obviously not happen either due to that os_free(wpa_s) > > following this call.. > > Actually wpa_s->radio is only being freed, when no other interface is > using it, like > wlanX. The patches only ensures radio is to null, when freed. It doesn't do that. It ensures that wpa_s->radio is _not_ set to NULL when the radio instance is not freed, but it in no way ensures that wpa_s->radio gets set to NULL (for all wpa_s instances having used the radio) when the radio gets removed. That's the part that I don't think is appropriate since it could leave behind references to a freed radio on other interfaces than the one that removed the radio. Anyway, this change is not sufficient to address the issue even if it happens to hide the crash in some cases. The wpa_s instance is already freed when wpa_s->radio is used on the scan result handling part. I fixed this issue by skipping the processing steps following removal of the P2P group interface: http://w1.fi/cgit/hostap/commit/?id=b0e669beebbb0d764c354f6ef7736c58f82681ec
diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c index 6ad09a8..0897294 100644 --- a/wpa_supplicant/wpa_supplicant.c +++ b/wpa_supplicant/wpa_supplicant.c @@ -3675,13 +3675,13 @@ static void radio_remove_interface(struct wpa_supplicant *wpa_s) wpa_s->ifname, radio->name); dl_list_del(&wpa_s->radio_list); radio_remove_works(wpa_s, NULL, 0); - wpa_s->radio = NULL; if (!dl_list_empty(&radio->ifaces)) return; /* Interfaces remain for this radio */ wpa_printf(MSG_DEBUG, "Remove radio %s", radio->name); eloop_cancel_timeout(radio_start_next_work, radio, NULL); os_free(radio); + wpa_s->radio = NULL; }
This patch avoids a segmentation fault that is occurring when a removed pending p2p interface receives a delayed scan result and crashes when handled in wpa_supplicant_event_scan_results. The scenario being used is a request to pbc join an autonomous p2p group. Signed-off-by: Eduardo Abinader <eduardo.abinader@openbossa.org> --- wpa_supplicant/wpa_supplicant.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)