[PATCH] cfg80211: fix memory leak/corruption of bss_list

[PATCH] cfg80211: fix memory leak/corruption of bss_list

[Eliad Peller]

cfg80211_dev_free() calls cfg80211_put_bss() directly on all
the remaining bss entries, skipping the proper bss entry
cleanup that usually made by __cfg80211_unlink_bss(), and
leaving the bss_list and the rb_tree with dangling pointers.

Fix it by calling cfg80211_unlink_bss() instead.

Cc: stable@vger.kernel.org
Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 net/wireless/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 4e86a86..232c385 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -729,7 +729,7 @@ void cfg80211_dev_free(struct cfg80211_registered_device *rdev)
 	mutex_destroy(&rdev->devlist_mtx);
 	mutex_destroy(&rdev->sched_scan_mtx);
 	list_for_each_entry_safe(scan, tmp, &rdev->bss_list, list)
-		cfg80211_put_bss(&scan->pub);
+		cfg80211_unlink_bss(&rdev->wiphy, &scan->pub);
 	kfree(rdev);
 }
 
-- 
1.7.6.401.g6a319

Re: [PATCH] cfg80211: fix memory leak/corruption of bss_list

[Ben Greear]

On 05/17/2012 09:06 AM, Eliad Peller wrote:
> cfg80211_dev_free() calls cfg80211_put_bss() directly on all
> the remaining bss entries, skipping the proper bss entry
> cleanup that usually made by __cfg80211_unlink_bss(), and
> leaving the bss_list and the rb_tree with dangling pointers.
>
> Fix it by calling cfg80211_unlink_bss() instead.

This doesn't apply clean against 3.3..though not too hard
to fix it up by hand.

Do you know how far back this does need to be applied (3.0, for instance)?

Thanks,
Ben

>
> Cc: stable@vger.kernel.org
> Signed-off-by: Eliad Peller<eliad@wizery.com>
> ---
>   net/wireless/core.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/wireless/core.c b/net/wireless/core.c
> index 4e86a86..232c385 100644
> --- a/net/wireless/core.c
> +++ b/net/wireless/core.c
> @@ -729,7 +729,7 @@ void cfg80211_dev_free(struct cfg80211_registered_device *rdev)
>   	mutex_destroy(&rdev->devlist_mtx);
>   	mutex_destroy(&rdev->sched_scan_mtx);
>   	list_for_each_entry_safe(scan, tmp,&rdev->bss_list, list)
> -		cfg80211_put_bss(&scan->pub);
> +		cfg80211_unlink_bss(&rdev->wiphy,&scan->pub);
>   	kfree(rdev);
>   }
>


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

Re: [PATCH] cfg80211: fix memory leak/corruption of bss_list

[Eliad Peller]

hi Ben,

On Thu, May 17, 2012 at 10:29 PM, Ben Greear <greearb@candelatech.com> wrote:
> On 05/17/2012 09:06 AM, Eliad Peller wrote:
>>
>> cfg80211_dev_free() calls cfg80211_put_bss() directly on all
>> the remaining bss entries, skipping the proper bss entry
>> cleanup that usually made by __cfg80211_unlink_bss(), and
>> leaving the bss_list and the rb_tree with dangling pointers.
>>
>> Fix it by calling cfg80211_unlink_bss() instead.
>
>
> This doesn't apply clean against 3.3..though not too hard
> to fix it up by hand.
>
> Do you know how far back this does need to be applied (3.0, for instance)?
>
as Johannes noted, this patch doesn't seem to solve an actual bug, so
it's not needed.

Eliad.

Re: [PATCH] cfg80211: fix memory leak/corruption of bss_list

[Johannes Berg]

On Thu, 2012-05-17 at 19:06 +0300, Eliad Peller wrote:
> cfg80211_dev_free() calls cfg80211_put_bss() directly on all
> the remaining bss entries, skipping the proper bss entry
> cleanup that usually made by __cfg80211_unlink_bss(), and
> leaving the bss_list and the rb_tree with dangling pointers.

>  	list_for_each_entry_safe(scan, tmp, &rdev->bss_list, list)
> -		cfg80211_put_bss(&scan->pub);
> +		cfg80211_unlink_bss(&rdev->wiphy, &scan->pub);
>  	kfree(rdev);

I don't see why we care, we free rdev anyway.

johannes

Re: [PATCH] cfg80211: fix memory leak/corruption of bss_list

[Eliad Peller]

On Thu, May 17, 2012 at 10:47 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2012-05-17 at 19:06 +0300, Eliad Peller wrote:
>> cfg80211_dev_free() calls cfg80211_put_bss() directly on all
>> the remaining bss entries, skipping the proper bss entry
>> cleanup that usually made by __cfg80211_unlink_bss(), and
>> leaving the bss_list and the rb_tree with dangling pointers.
>
>>       list_for_each_entry_safe(scan, tmp, &rdev->bss_list, list)
>> -             cfg80211_put_bss(&scan->pub);
>> +             cfg80211_unlink_bss(&rdev->wiphy, &scan->pub);
>>       kfree(rdev);
>
> I don't see why we care, we free rdev anyway.
>
yeah, you have a point here...

we got a crash report for an older kernel with some custom patches,
that indicated a possible write-after-free on the bss release. i was
probably too rushed to blame this code.

thanks,
Eliad.

Re: [PATCH] cfg80211: fix memory leak/corruption of bss_list

[Ben Greear]

On 05/17/2012 02:34 PM, Eliad Peller wrote:
> On Thu, May 17, 2012 at 10:47 PM, Johannes Berg
> <johannes@sipsolutions.net>  wrote:
>> On Thu, 2012-05-17 at 19:06 +0300, Eliad Peller wrote:
>>> cfg80211_dev_free() calls cfg80211_put_bss() directly on all
>>> the remaining bss entries, skipping the proper bss entry
>>> cleanup that usually made by __cfg80211_unlink_bss(), and
>>> leaving the bss_list and the rb_tree with dangling pointers.
>>
>>>        list_for_each_entry_safe(scan, tmp,&rdev->bss_list, list)
>>> -             cfg80211_put_bss(&scan->pub);
>>> +             cfg80211_unlink_bss(&rdev->wiphy,&scan->pub);
>>>        kfree(rdev);
>>
>> I don't see why we care, we free rdev anyway.
>>
> yeah, you have a point here...
>
> we got a crash report for an older kernel with some custom patches,
> that indicated a possible write-after-free on the bss release. i was
> probably too rushed to blame this code.

Ahhh, as luck would have it..we saw a crash today (in a hacked 3.3.4) that could
be explained by a stale bss reference, so I was all excited by your patch,
thinking I didn't have to go looking for the bug :)

My crash was in an older version of the ethtool-stats logic,
so it could just be my bug, as well....

Thanks,
Ben


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