Patchwork supplicant/p2p: Fix use-after free crash.

login
register
mail settings
Submitter Ben Greear
Date May 9, 2012, 7:09 p.m.
Message ID <1336590563-8513-1-git-send-email-greearb@candelatech.com>
Download mbox | patch
Permalink /patch/158037/
State Superseded
Headers show

Comments

Ben Greear - May 9, 2012, 7:09 p.m.
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(-)
Jouni Malinen - May 9, 2012, 7:40 p.m.
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)..
Ben Greear - May 9, 2012, 8:06 p.m.
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
Jouni Malinen - May 10, 2012, 8:02 a.m.
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.

Patch

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);