[PATCH 1/2] net: wireless, fix lock imbalance

[PATCH 1/2] net: wireless, fix lock imbalance

[Jiri Slaby]

One fail path in cfg80211_wext_siwscan omits to unlock rdev->mtx.
Fix that.

Trigerrable by "Scan for SSID" with long enough SSID (> 32).

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 net/wireless/scan.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 12dfa62..9c50c85 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -694,8 +694,10 @@ int cfg80211_wext_siwscan(struct net_device *dev,
 	/* translate "Scan for SSID" request */
 	if (wreq) {
 		if (wrqu->data.flags & IW_SCAN_THIS_ESSID) {
-			if (wreq->essid_len > IEEE80211_MAX_SSID_LEN)
-				return -EINVAL;
+			if (wreq->essid_len > IEEE80211_MAX_SSID_LEN) {
+				err = -EINVAL;
+				goto out;
+			}
 			memcpy(creq->ssids[0].ssid, wreq->essid, wreq->essid_len);
 			creq->ssids[0].ssid_len = wreq->essid_len;
 		}
-- 
1.6.5.7

[PATCH 2/2] NET: wireless, fix memory leak

[Jiri Slaby]

Stanse found a memory leak in cfg80211_wext_siwscan. creq is not
freed/assigned on all paths. Fix that.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 net/wireless/scan.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 9c50c85..3003d13 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -685,7 +685,7 @@ int cfg80211_wext_siwscan(struct net_device *dev,
 	/* No channels found? */
 	if (!i) {
 		err = -EINVAL;
-		goto out;
+		goto out_free;
 	}
 
 	/* Set real number of channels specified in creq->channels[] */
@@ -696,7 +696,7 @@ int cfg80211_wext_siwscan(struct net_device *dev,
 		if (wrqu->data.flags & IW_SCAN_THIS_ESSID) {
 			if (wreq->essid_len > IEEE80211_MAX_SSID_LEN) {
 				err = -EINVAL;
-				goto out;
+				goto out_free;
 			}
 			memcpy(creq->ssids[0].ssid, wreq->essid, wreq->essid_len);
 			creq->ssids[0].ssid_len = wreq->essid_len;
@@ -717,6 +717,9 @@ int cfg80211_wext_siwscan(struct net_device *dev,
  out:
 	cfg80211_unlock_rdev(rdev);
 	return err;
+out_free:
+	kfree(creq);
+	goto out;
 }
 EXPORT_SYMBOL_GPL(cfg80211_wext_siwscan);
 
-- 
1.6.5.7

Re: [PATCH 2/2] NET: wireless, fix memory leak

[Luis R. Rodriguez]

On Wed, Jan 6, 2010 at 8:35 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> Stanse found a memory leak in cfg80211_wext_siwscan. creq is not
> freed/assigned on all paths. Fix that.

CC stable?

 Luis

Re: [PATCH 1/2] net: wireless, fix lock imbalance

[Luis R. Rodriguez]

On Wed, Jan 6, 2010 at 8:35 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> One fail path in cfg80211_wext_siwscan omits to unlock rdev->mtx.
> Fix that.
>
> Trigerrable by "Scan for SSID" with long enough SSID (> 32).

Cc stable as well?

  Luis

Re: [PATCH 2/2] NET: wireless, fix memory leak

[Gertjan van Wingerde]

On 01/06/10 17:35, Jiri Slaby wrote:
> Stanse found a memory leak in cfg80211_wext_siwscan. creq is not
> freed/assigned on all paths. Fix that.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>  net/wireless/scan.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> index 9c50c85..3003d13 100644
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -685,7 +685,7 @@ int cfg80211_wext_siwscan(struct net_device *dev,
>  	/* No channels found? */
>  	if (!i) {
>  		err = -EINVAL;
> -		goto out;
> +		goto out_free;
>  	}
>  
>  	/* Set real number of channels specified in creq->channels[] */
> @@ -696,7 +696,7 @@ int cfg80211_wext_siwscan(struct net_device *dev,
>  		if (wrqu->data.flags & IW_SCAN_THIS_ESSID) {
>  			if (wreq->essid_len > IEEE80211_MAX_SSID_LEN) {
>  				err = -EINVAL;
> -				goto out;
> +				goto out_free;
>  			}
>  			memcpy(creq->ssids[0].ssid, wreq->essid, wreq->essid_len);
>  			creq->ssids[0].ssid_len = wreq->essid_len;
> @@ -717,6 +717,9 @@ int cfg80211_wext_siwscan(struct net_device *dev,
>   out:
>  	cfg80211_unlock_rdev(rdev);
>  	return err;
> +out_free:
> +	kfree(creq);
> +	goto out;
>  }
>  EXPORT_SYMBOL_GPL(cfg80211_wext_siwscan);
>  

This last part looks a bit strange. Why don't you put out_free label before out label,
and let it continue after the kfree, instead of jumping back to the out label.

---
Gertjan.

Re: [PATCH 2/2] NET: wireless, fix memory leak

[Jiri Slaby]

On 01/06/2010 08:07 PM, Gertjan van Wingerde wrote:
> On 01/06/10 17:35, Jiri Slaby wrote:
>> @@ -717,6 +717,9 @@ int cfg80211_wext_siwscan(struct net_device *dev,
>>   out:
>>  	cfg80211_unlock_rdev(rdev);
>>  	return err;
>> +out_free:
>> +	kfree(creq);
>> +	goto out;
>>  }
>>  EXPORT_SYMBOL_GPL(cfg80211_wext_siwscan);
>>  
> 
> This last part looks a bit strange. Why don't you put out_free label before out label,

Because `out' is a part of non-error flow, where the creq should not be
freed.

thanks,
-- 
js

Re: [PATCH 1/2] net: wireless, fix lock imbalance

[John W. Linville]

On Wed, Jan 06, 2010 at 05:35:42PM +0100, Jiri Slaby wrote:
> One fail path in cfg80211_wext_siwscan omits to unlock rdev->mtx.
> Fix that.
> 
> Trigerrable by "Scan for SSID" with long enough SSID (> 32).
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

Both this and the next one are already fixed in wireless-2.6...

commit 65486c8b30498dd274eea2c542696f22b63fe5b8
Author: Johannes Berg <johannes@sipsolutions.net>
Date:   Wed Dec 23 15:33:35 2009 +0100

    cfg80211: fix error path in cfg80211_wext_siwscan
    
    If there's an invalid channel or SSID, the code leaks
    the scan request. Always free the scan request, unless
    it was successfully given to the driver.
    
    Reported-by: Dan Carpenter <error27@gmail.com>
    Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
    Acked-by: Dan Carpenter <error27@gmail.com>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 1/2] net: wireless, fix lock imbalance

[Jiri Slaby]

On 01/06/2010 08:55 PM, John W. Linville wrote:
> On Wed, Jan 06, 2010 at 05:35:42PM +0100, Jiri Slaby wrote:
>> One fail path in cfg80211_wext_siwscan omits to unlock rdev->mtx.
>> Fix that.
>>
>> Trigerrable by "Scan for SSID" with long enough SSID (> 32).
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> 
> Both this and the next one are already fixed in wireless-2.6...

Ok, thanks.
-- 
js