[PATCH] supplicant/p2p: Fix use-after free crash.

[Ben Greear]

On 05/09/2012 12:40 PM, Jouni Malinen wrote:
> On Wed, May 09, 2012 at 12:09:23PM -0700, greearb at 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

-- 
Ben Greear <greearb at candelatech.com>
Candela Technologies Inc  http://www.candelatech.com