Re: [PATCH 2/3] sysfs: Only accept read/write permissions for file attributes

[Vivien Didelot <vivien.didelot@savoirfairelinux.com>]

Hi Guenter,

> For sysfs file attributes, only read and write permisssions make sense.

Minor typo, there's an extra 's' to permissions.

> Mask provided attribute permissions accordingly and send a warning
> to the console if invalid permission bits are set.
> 
> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  fs/sysfs/group.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 305eccb..0de6473 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -55,6 +55,12 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>                                  if (!mode)
>                                          continue;
>                          }
> +
> +                        WARN(mode & ~(S_IRUGO | S_IWUGO | SYSFS_PREALLOC),
> +                             "Attribute %s: Invalid permission 0x%x\n",
> +                             (*attr)->name, mode);

To print permissions, I would suggest unsigned octal ("0%o").

> +
> +                        mode &= S_IRUGO | S_IWUGO | SYSFS_PREALLOC;

As readable attributes are created with S_IRUGO and writable attributes are
created with S_IWUSR, I would limit the scope of is_visible to only:
S_IRUGO | S_IWUSR. Write permission for group and others feels wrong.

Then, I think we may want to keep the extra bits (all mode bits > 0777) from
the default attribute mode. Can they be used for sysfs attributes?

My suggestion is something like this:

        /* Limit the scope to S_IRUGO | S_IWUSR */
        if (mode & ~(S_IRUGO | S_IWUSR))
                pr_warn("Attribute %s: Invalid permissions 0%o\n",
                        (*attr)->name, mode);

        mode &= S_IRUGO | S_IWUSR;

        /* Use only returned bits and defaults > 0777 */
        mode |= (*attr)->mode & ~S_IRWXUGO;

>                          error = sysfs_add_file_mode_ns(parent, *attr, false,
>                                                         mode, NULL);
>                          if (unlikely(error))

The code hitting this warning actually is drivers/pci/pci-sysfs.c, which
declares write-only attributes with S_IWUSR|S_IWGRP (0220). Is that correct to
have write access for group for these attributes?

Thanks,
-v