| Message ID | 1336590563-8513-1-git-send-email-greearb@candelatech.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, May 09, 2012 at 12:09:23PM -0700, greearb@candelatech.com wrote: > This is a hack to fix use-after-free when the > first-enabled station is deleted. Does the "station" in that refer to the wpa_supplicant interface? > diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c > +#include "p2p/p2p_i.h" This won't fly (internal header file cannot be included here). > + /* P2P logic is bad: It saves a pointer to wpa_s in global->p2p > + * in the wpas_p2p_init logic. And, only does this for the first > + * station. When a station goes away, the p2p logic is not fixed > + * up properly, leading to use-after-free issues when global->p2p->msg_ctx > + * or cb_ctx is used (at least). Well, whether it is bad or not, there is an assumption on the first wpa_supplicant interface being used for P2P management. > + * I think p2p logic is going to need a serious re-write to get rid of > + * that global pointer or at least manage it better. In the meantime, > + * this code attempts to at least stop crashes due to stale memory access. I'm not sure I want to go as far as re-writing this part any time soon.. > + if (global->p2p && global->p2p->cfg && > + (global->p2p->cfg->msg_ctx == wpa_s || > + global->p2p->cfg->cb_ctx == wpa_s)) { > + wpa_dbg(wpa_s, MSG_ERROR, "ERROR: global->p2p still has reference in deinit_iface()"); > + wpas_p2p_deinit(wpa_s); > + /* If that didn't do the trick, I think we just have to > + * de-init everything. > + */ > + if (global->p2p && global->p2p->cfg && > + (global->p2p->cfg->msg_ctx == wpa_s || > + global->p2p->cfg->cb_ctx == wpa_s)) { > + wpa_dbg(wpa_s, MSG_ERROR, "ERROR: global->p2p still has reference after p2p_deinit(), disabling p2p globally."); > + wpas_p2p_deinit_global(global); > + } > + } It's probable fine to disable P2P in case the first wpa_s instance goes away (i.e., the interface that called wpas_p2p_init()).. Though, this needs to be done bit more cleanly so that p2p_i.h does not need to get included here (may need to maintain a pointer to the initializing wpa_s within struct wpa_global)..
On 05/09/2012 12:40 PM, Jouni Malinen wrote: > On Wed, May 09, 2012 at 12:09:23PM -0700, greearb@candelatech.com wrote: >> This is a hack to fix use-after-free when the >> first-enabled station is deleted. > > Does the "station" in that refer to the wpa_supplicant interface? Like when I delete wlan0 (via wpa_cli commands). >> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c >> +#include "p2p/p2p_i.h" > > This won't fly (internal header file cannot be included here). I know it's not the proper way..but it does compile and work. The whole patch is a hack. I don't expect it to go upstream as it is...hoping someone who understands how this is supposed to work can do it right. >> + /* P2P logic is bad: It saves a pointer to wpa_s in global->p2p >> + * in the wpas_p2p_init logic. And, only does this for the first >> + * station. When a station goes away, the p2p logic is not fixed >> + * up properly, leading to use-after-free issues when global->p2p->msg_ctx >> + * or cb_ctx is used (at least). > > Well, whether it is bad or not, there is an assumption on the first > wpa_supplicant interface being used for P2P management. > >> + * I think p2p logic is going to need a serious re-write to get rid of >> + * that global pointer or at least manage it better. In the meantime, >> + * this code attempts to at least stop crashes due to stale memory access. > > I'm not sure I want to go as far as re-writing this part any time soon.. If I ever actually need P2P to work, I might make an attempt. In the meantime, as long as it doesn't crash stuff I'm happy. Maybe just re-build the global->p2p data after its station is deleted using any remaining stations? >> + if (global->p2p&& global->p2p->cfg&& >> + (global->p2p->cfg->msg_ctx == wpa_s || >> + global->p2p->cfg->cb_ctx == wpa_s)) { >> + wpa_dbg(wpa_s, MSG_ERROR, "ERROR: global->p2p still has reference in deinit_iface()"); >> + wpas_p2p_deinit(wpa_s); >> + /* If that didn't do the trick, I think we just have to >> + * de-init everything. >> + */ >> + if (global->p2p&& global->p2p->cfg&& >> + (global->p2p->cfg->msg_ctx == wpa_s || >> + global->p2p->cfg->cb_ctx == wpa_s)) { >> + wpa_dbg(wpa_s, MSG_ERROR, "ERROR: global->p2p still has reference after p2p_deinit(), disabling p2p globally."); >> + wpas_p2p_deinit_global(global); >> + } >> + } > > It's probable fine to disable P2P in case the first wpa_s instance goes > away (i.e., the interface that called wpas_p2p_init()).. Though, this > needs to be done bit more cleanly so that p2p_i.h does not need to get > included here (may need to maintain a pointer to the initializing wpa_s > within struct wpa_global).. Thanks, Ben
On Wed, May 09, 2012 at 01:06:28PM -0700, Ben Greear wrote: > If I ever actually need P2P to work, I might make an attempt. > In the meantime, as long as it doesn't crash stuff I'm happy. > > Maybe just re-build the global->p2p data after its station is deleted > using any remaining stations? I committed the simple fix for now, i.e., disable P2P on removal of the specific interface. P2P will be re-enabled on addition of the next interface. It would be possible to re-initialize P2P based on other interfaces (or just update the p2p->cfg->msg_ctx without p2p_init/p2p_deinit). However, there are assumptions in how upper layer programs manage P2P and that would get pretty messy if the specific management interface would change dynamically. As such, I think it is fine to leave this at the simplest level of just making sure non-P2P operations do not get affected and the process does not crash on use of freed memory.
diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c index 0996c38..dbf06b4 100644 --- a/wpa_supplicant/wpa_supplicant.c +++ b/wpa_supplicant/wpa_supplicant.c @@ -32,6 +32,7 @@ #include "common/wpa_ctrl.h" #include "common/ieee802_11_defs.h" #include "p2p/p2p.h" +#include "p2p/p2p_i.h" #include "blacklist.h" #include "wpas_glue.h" #include "wps_supplicant.h" @@ -2692,6 +2693,8 @@ next_driver: static void wpa_supplicant_deinit_iface(struct wpa_supplicant *wpa_s, int notify, int terminate) { + struct wpa_global *global = wpa_s->global; + if (wpa_s->drv_priv) { wpa_supplicant_deauthenticate(wpa_s, WLAN_REASON_DEAUTH_LEAVING); @@ -2702,6 +2705,31 @@ static void wpa_supplicant_deinit_iface(struct wpa_supplicant *wpa_s, wpa_supplicant_cleanup(wpa_s); + /* P2P logic is bad: It saves a pointer to wpa_s in global->p2p + * in the wpas_p2p_init logic. And, only does this for the first + * station. When a station goes away, the p2p logic is not fixed + * up properly, leading to use-after-free issues when global->p2p->msg_ctx + * or cb_ctx is used (at least). + * I think p2p logic is going to need a serious re-write to get rid of + * that global pointer or at least manage it better. In the meantime, + * this code attempts to at least stop crashes due to stale memory access. + */ + if (global->p2p && global->p2p->cfg && + (global->p2p->cfg->msg_ctx == wpa_s || + global->p2p->cfg->cb_ctx == wpa_s)) { + wpa_dbg(wpa_s, MSG_ERROR, "ERROR: global->p2p still has reference in deinit_iface()"); + wpas_p2p_deinit(wpa_s); + /* If that didn't do the trick, I think we just have to + * de-init everything. + */ + if (global->p2p && global->p2p->cfg && + (global->p2p->cfg->msg_ctx == wpa_s || + global->p2p->cfg->cb_ctx == wpa_s)) { + wpa_dbg(wpa_s, MSG_ERROR, "ERROR: global->p2p still has reference after p2p_deinit(), disabling p2p globally."); + wpas_p2p_deinit_global(global); + } + } + if (wpa_s->drv_priv) wpa_drv_deinit(wpa_s);
From: Ben Greear <greearb@candelatech.com> This is a hack to fix use-after-free when the first-enabled station is deleted. This needs to be fixed properly, but at least now we do not crash. Valgrind report from one such crash: For counts of detected and suppressed errors, rerun with: -v ==31996== ERROR SUMMARY: 59 errors from 59 contexts (suppressed: 6 from 6) ==31996== Invalid read of size 1 ==31996== at 0x3108E46781: vfprintf (in /lib64/libc-2.13.so) ==31996== by 0x3108E6EF31: vsnprintf (in /lib64/libc-2.13.so) ==31996== by 0x3108E4FBF2: snprintf (in /lib64/libc-2.13.so) ==31996== by 0x40F995: wpa_msg (wpa_debug.c:613) ==31996== by 0x433D37: p2p_stop_find_for_freq (p2p.c:1027) ==31996== by 0x433EFD: p2p_stop_find (p2p.c:1069) ==31996== by 0x437924: p2p_flush (p2p.c:2305) ==31996== by 0x437829: p2p_deinit (p2p.c:2286) ==31996== by 0x42B38C: wpas_p2p_deinit_global (p2p_supplicant.c:2571) ==31996== by 0x4CA169: wpa_supplicant_deinit (wpa_supplicant.c:3049) ==31996== by 0x4D5E08: main (main.c:288) ==31996== Address 0x504661e is 46 bytes inside a block of size 1,600 free'd ==31996== at 0x4A05187: free (vg_replace_malloc.c:325) ==31996== by 0x4C9C2E: wpa_supplicant_remove_iface (wpa_supplicant.c:2830) ==31996== by 0x4BA40E: wpa_supplicant_global_iface_remove (ctrl_iface.c:4441) ==31996== by 0x4BA859: wpa_supplicant_global_ctrl_iface_process (ctrl_iface.c:4555) ==31996== by 0x4BBE4D: wpa_supplicant_global_ctrl_iface_receive (ctrl_iface_unix.c:631) ==31996== by 0x4115B0: eloop_sock_table_dispatch_table (eloop.c:335) ==31996== by 0x41161D: eloop_sock_table_dispatch (eloop.c:352) ==31996== by 0x4120EC: eloop_run (eloop.c:766) ==31996== by 0x4CA13F: wpa_supplicant_run (wpa_supplicant.c:3028) ==31996== by 0x4D5DF9: main (main.c:286) ==31996== Signed-hostap: Ben Greear <greearb@candelatech.com> --- wpa_supplicant/wpa_supplicant.c | 28 ++++++++++++++++++++++++++++ 1 files changed, 28 insertions(+), 0 deletions(-)