diff mbox

BSS: Fix use-after-realloc

Message ID 1330943415-6823-1-git-send-email-eliad@wizery.com
State Accepted
Commit eb37e085a4c17a7ebdf258d480c5f2c8a2ac7f08
Headers show

Commit Message

Eliad Peller March 5, 2012, 10:30 a.m. UTC 
After reallocation of the bss struct, current_bss
wasn't updated and could hold an invalid pointer
(which might get dereferenced later).

Update current_bss if the pointer was changed.

Signed-hostap: Eliad Peller <eliad@wizery.com>
intended-for: hostap-1
---
realloc is pretty dangerous if a reference could be
saved somewhere. i suspect there might be similar issues
in the codebase, but i haven't looked at it throughtly.

 wpa_supplicant/bss.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Comments

Jouni Malinen March 5, 2012, 2:22 p.m. UTC | #1 
On Mon, Mar 05, 2012 at 12:30:15PM +0200, Eliad Peller wrote:
> After reallocation of the bss struct, current_bss
> wasn't updated and could hold an invalid pointer
> (which might get dereferenced later).
> 
> Update current_bss if the pointer was changed.

Thanks for catching this!

> realloc is pretty dangerous if a reference could be
> saved somewhere. i suspect there might be similar issues
> in the codebase, but i haven't looked at it throughtly.

realloc is not the only reason for that.. Similar cases apply for
configuration re-read, but I would hope that those are all covered with
clearing current_ssid.

> diff --git a/wpa_supplicant/bss.c b/wpa_supplicant/bss.c
> @@ -333,6 +333,8 @@ static void wpa_bss_update(struct wpa_supplicant *wpa_s, struct wpa_bss *bss,
>  				  res->ie_len + res->beacon_ie_len);
>  			bss->ie_len = res->ie_len;
>  			bss->beacon_ie_len = res->beacon_ie_len;
> +			if (wpa_s->current_bss == bss)
> +				wpa_s->current_bss = nbss;

This is broken.. bss == nbss here. I would assume you wanted to do that
just before the "bss = nbss;" line.
Eliad Peller March 5, 2012, 2:34 p.m. UTC | #2 
On Mon, Mar 5, 2012 at 4:22 PM, Jouni Malinen <j@w1.fi> wrote:
> On Mon, Mar 05, 2012 at 12:30:15PM +0200, Eliad Peller wrote:
>> After reallocation of the bss struct, current_bss
>> wasn't updated and could hold an invalid pointer
>> (which might get dereferenced later).
>>
>> Update current_bss if the pointer was changed.
>
> Thanks for catching this!
>

>> diff --git a/wpa_supplicant/bss.c b/wpa_supplicant/bss.c
>> @@ -333,6 +333,8 @@ static void wpa_bss_update(struct wpa_supplicant *wpa_s, struct wpa_bss *bss,
>>                                 res->ie_len + res->beacon_ie_len);
>>                       bss->ie_len = res->ie_len;
>>                       bss->beacon_ie_len = res->beacon_ie_len;
>> +                     if (wpa_s->current_bss == bss)
>> +                             wpa_s->current_bss = nbss;
>
> This is broken.. bss == nbss here. I would assume you wanted to do that
> just before the "bss = nbss;" line.
>
err...
you are right of course.
thanks for catching this! :)

do you want me to resend or can you just fix and apply it?

Eliad.
Jouni Malinen March 5, 2012, 3:12 p.m. UTC | #3 
On Mon, Mar 05, 2012 at 04:34:44PM +0200, Eliad Peller wrote:
> On Mon, Mar 5, 2012 at 4:22 PM, Jouni Malinen <j@w1.fi> wrote:
> > This is broken.. bss == nbss here. I would assume you wanted to do that
> > just before the "bss = nbss;" line.
> >
> err...
> you are right of course.
> thanks for catching this! :)
> 
> do you want me to resend or can you just fix and apply it?

No need to resend - I applied a fixed version of the patch. Thanks!
diff mbox

Patch

diff --git a/wpa_supplicant/bss.c b/wpa_supplicant/bss.c
index 2a5bb85..c0b4331 100644
--- a/wpa_supplicant/bss.c
+++ b/wpa_supplicant/bss.c
@@ -333,6 +333,8 @@  static void wpa_bss_update(struct wpa_supplicant *wpa_s, struct wpa_bss *bss,
 				  res->ie_len + res->beacon_ie_len);
 			bss->ie_len = res->ie_len;
 			bss->beacon_ie_len = res->beacon_ie_len;
+			if (wpa_s->current_bss == bss)
+				wpa_s->current_bss = nbss;
 		}
 		dl_list_add(prev, &bss->list_id);
 	}