SIGSEGV in Supplicant

[Peer, Ilan]

> -----Original Message-----
> From: Jouni Malinen [mailto:j at w1.fi]
> Sent: Thursday, June 11, 2015 03:14
> To: Peer, Ilan
> Cc: abdoulaye berthe; hostap at lists.shmoo.com;
> mikael.kanstrup at sonymobile.com; Abdoulaye Berthe
> Subject: Re: SIGSEGV in Supplicant
> 
> On Mon, Jun 01, 2015 at 10:44:38AM +0000, Peer, Ilan wrote:
> > It is possible that in this flow, there was a pending query that was not started
> yet, so when gas_query_deinit() flow was executed and gas_query_free() was
> called, the 'query' object was freed, but without having the radio work
> removed. Thus, later, when all the radio works of the interface were
> removed, gas_query_start_cb() is called, which in turn called
> gas_query_free() which tried to access query->list which resulted with a
> segfault.
> >
> > Can you please check if the attached patch fixes things? (I did not
> > test it ...)
> 
> This is not really the cleanest way of handling this, but I could not find any
> better way to do it without significant extra complexity which is not really
> justifiable on the interface cleanup path.. The issue here is in
> gas_query_free() not being able to remove the pending radio work that has
> not yet been started since it does not have a pointer to that radio work.
> Clearing all gas-query radio works just before calling
> gas_query_deinit() is a simple way of avoiding this and that's fine for
> wpa_supplicant_cleanup() to do, so I applied this. However, this needed a
> small change to avoid a NULL pointer dereference if the interface in question
> had not completed initialization (wpa_s->radio could be NULL here). I added
> that and a comment explaining why this is here.
> 
> The fix is here:
> http://w1.fi/cgit/hostap/commit/?id=57e832de37ea0a82e650d8230457e08
> 68a01b72e
> 
> And these hwsim test cases reproduce the issue:
> http://w1.fi/cgit/hostap/commit/?id=c518fecc826a5c7101ed6c7708eb618cf
> 2ffbfe2
> 
> ap_hs20_connect_deinit should be practically identical to the problem
> reported in this thread. gas_query_deinit is a bit simpler test case which is
> also hitting the same issue, but through a bit different code path.
> 

Looks good :)

Thanks,

Ilan.