netdev - Re: [PATCH] wireless: Add missing locking to cfg80211_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080508221217.GA22808@sergelap.austin.ibm.com>
Date:	Thu, 8 May 2008 17:12:17 -0500
From:	"Serge E. Hallyn" <serue@...ibm.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Johannes Berg <johannes@...solutions.net>,
	"John W. Linville" <linville@...driver.com>,
	"David S. Miller" <davem@...emloft.net>,
	Benjamin Thery <benjamin.thery@...l.net>,
	linux-kernel@...r.kernel.org, Tejun Heo <htejun@...il.com>,
	Al Viro <viro@....linux.org.uk>,
	Daniel Lezcano <dlezcano@...ibm.com>,
	"Serge E. Hallyn" <serue@...ibm.com>,
	Pavel Emelyanov <xemul@...nvz.org>, netdev@...r.kernel.org,
	Greg KH <gregkh@...e.de>
Subject: Re: [PATCH] wireless: Add missing locking to cfg80211_dev_rename

Quoting Eric W. Biederman (ebiederm@...ssion.com):
> 
> device_rename only performs useful and race free validity
> checking at the optional sysfs level so depending on it
> for all of the validity checking in cfg80211_dev_rename
> is racy.
> 
> Instead implement all of the needed validity checking
> and locking in cfg80211_dev_rename.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@...ssion.com>

I'm about as far as you can get from either a sysfs/kobject or network
expert, but both of these patches as well as your general concept that
renames should be handled at sysfs seem to me to make perfect sense.

thanks,
-serge

> ---
>  net/wireless/core.c |   33 ++++++++++++++++++++++++++++-----
>  1 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/net/wireless/core.c b/net/wireless/core.c
> index 80afacd..f1da0b9 100644
> --- a/net/wireless/core.c
> +++ b/net/wireless/core.c
> @@ -143,8 +143,11 @@ void cfg80211_put_dev(struct cfg80211_registered_device *drv)
>  int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
>  			char *newname)
>  {
> +	struct cfg80211_registered_device *drv;
>  	int idx, taken = -1, result, digits;
> 
> +	mutex_lock(&cfg80211_drv_mutex);
> +
>  	/* prohibit calling the thing phy%d when %d is not its number */
>  	sscanf(newname, PHY_NAME "%d%n", &idx, &taken);
>  	if (taken == strlen(newname) && idx != rdev->idx) {
> @@ -156,14 +159,30 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
>  		 * deny the name if it is phy<idx> where <idx> is printed
>  		 * without leading zeroes. taken == strlen(newname) here
>  		 */
> +		result = -EINVAL;
>  		if (taken == strlen(PHY_NAME) + digits)
> -			return -EINVAL;
> +			goto out_unlock;
> +	}
> +
> +
> +	/* Ignore nop renames */
> +	result = 0;
> +	if (strcmp(newname, dev_name(&rdev->wiphy.dev)) == 0)
> +		goto out_unlock;
> +
> +	/* Ensure another device does not already have this name. */
> +	list_for_each_entry(drv, &cfg80211_drv_list, list) {
> +		result = -EINVAL;
> +		if (strcmp(newname, dev_name(&drv->wiphy.dev)) == 0)
> +			goto out_unlock;
>  	}
> 
> -	/* this will check for collisions */
> +	/* this will only check for collisions in sysfs
> +	 * which is not even always compiled in.
> +	 */
>  	result = device_rename(&rdev->wiphy.dev, newname);
>  	if (result)
> -		return result;
> +		goto out_unlock;
> 
>  	if (!debugfs_rename(rdev->wiphy.debugfsdir->d_parent,
>  			    rdev->wiphy.debugfsdir,
> @@ -172,9 +191,13 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
>  		printk(KERN_ERR "cfg80211: failed to rename debugfs dir to %s!\n",
>  		       newname);
> 
> -	nl80211_notify_dev_rename(rdev);
> +	result = 0;
> +out_unlock:
> +	mutex_unlock(&cfg80211_drv_mutex);
> +	if (result == 0)
> +		nl80211_notify_dev_rename(rdev);
> 
> -	return 0;
> +	return result;
>  }
> 
>  /* exported functions */
> -- 
> 1.5.3.rc6.17.g1911
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html