linux-kernel - Re: [PATCH net-next v3 5/9] device: add device_change

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]

Date:   Thu, 20 Feb 2020 12:25:13 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Christian Brauner <christian.brauner@...ntu.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Pavel Machek <pavel@....cz>, Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH net-next v3 5/9] device: add device_change_owner()

On Tue, Feb 18, 2020 at 05:29:39PM +0100, Christian Brauner wrote:
> Add a helper to change the owner of a device's sysfs entries. This
> needs to happen when the ownership of a device is changed, e.g. when
> moving network devices between network namespaces.
> This function will be used to correctly account for ownership changes,
> e.g. when moving network devices between network namespaces.
> 
> Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
> ---
> /* v2 */
> unchanged
> 
> /* v3 */
> -  Greg Kroah-Hartman <gregkh@...uxfoundation.org>:
>    - Add explicit uid/gid parameters.
> ---
>  drivers/base/core.c    | 80 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |  1 +
>  2 files changed, 81 insertions(+)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 42a672456432..ec0d5e8cfd0f 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3458,6 +3458,86 @@ int device_move(struct device *dev, struct device *new_parent,
>  }
>  EXPORT_SYMBOL_GPL(device_move);
>  
> +static int device_attrs_change_owner(struct device *dev, kuid_t kuid,
> +				     kgid_t kgid)
> +{
> +	struct kobject *kobj = &dev->kobj;
> +	struct class *class = dev->class;
> +	const struct device_type *type = dev->type;
> +	int error;
> +
> +	if (class) {
> +		error = sysfs_groups_change_owner(kobj, class->dev_groups, kuid,
> +						  kgid);
> +		if (error)
> +			return error;
> +	}
> +
> +	if (type) {
> +		error = sysfs_groups_change_owner(kobj, type->groups, kuid,
> +						  kgid);
> +		if (error)
> +			return error;
> +	}
> +
> +	error = sysfs_groups_change_owner(kobj, dev->groups, kuid, kgid);
> +	if (error)
> +		return error;
> +
> +	if (device_supports_offline(dev) && !dev->offline_disabled) {
> +		error = sysfs_file_change_owner_by_name(
> +			kobj, dev_attr_online.attr.name, kuid, kgid);
> +		if (error)
> +			return error;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * device_change_owner - change the owner of an existing device.

The "owner" and what else gets changed here?  Please document this
better.


> + * @dev: device.
> + * @kuid: new owner's kuid
> + * @kgid: new owner's kgid
> + */
> +int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> +{
> +	int error;
> +	struct kobject *kobj = &dev->kobj;
> +
> +	dev = get_device(dev);
> +	if (!dev)
> +		return -EINVAL;
> +
> +	error = sysfs_change_owner(kobj, kuid, kgid);

the kobject of the device is changed, good.

> +	if (error)
> +		goto out;
> +
> +	error = sysfs_file_change_owner_by_name(kobj, dev_attr_uevent.attr.name,
> +						kuid, kgid);

Why call out the uevent file explicitly here?

> +	if (error)
> +		goto out;
> +
> +	error = device_attrs_change_owner(dev, kuid, kgid);
> +	if (error)
> +		goto out;

Doesn't this also change the uevent file?

> +
> +#ifdef CONFIG_BLOCK
> +	if (sysfs_deprecated && dev->class == &block_class)
> +		goto out;
> +#endif

Ugh, we still need this?

> +
> +	error = sysfs_link_change_owner(&dev->class->p->subsys.kobj, &dev->kobj,
> +					dev_name(dev), kuid, kgid);

Now what is this changing?

Again, more documentation please as to exactly what is being changed in
this function is needed.

thanks,

greg k-h