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

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: <643183939.413796.1421712438502.JavaMail.root@mail>
Date:	Mon, 19 Jan 2015 19:07:18 -0500 (EST)
From:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, kernel <kernel@...oirfairelinux.com>
Subject: Re: [PATCH 2/3] sysfs: Only accept read/write permissions for file
 attributes

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@...oirfairelinux.com>
> Signed-off-by: Guenter Roeck <linux@...ck-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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/