[PATCH 0/3] sysfs: Refine is_visible API

[PATCH 0/3] sysfs: Refine is_visible API

[Guenter Roeck]

Up to now, is_visible can only be used to either remove visibility
of a file entirely or to add permissions, but not to reduce permissions.
This makes it impossible, for example, to use DEVICE_ATTR_RW to define
file attributes and reduce permissions to read-only.

This behavior is undesirable and unnecessarily complicates code which
needs to reduce permissions; instead of just returning the desired
permissions, it has to ensure that the permissions in the attribute
variable declaration only reflect the minimal permissions ever needed.

Change semantics of is_visible to only use the permissions returned
from it instead of oring the returned value with the hard-coded
permissions.

The code now dumps a warning to the console if an is_visible function
returns unexpected permissions.

Also document struct attribute_group.

Tested with v3.19-rc5.

[PATCH 1/3] sysfs: Use only return value from is_visible for the file mode

[Guenter Roeck]

Up to now, is_visible can only be used to either remove visibility
of a file entirely or to add permissions, but not to reduce permissions.
This makes it impossible, for example, to use DEVICE_ATTR_RW to define
file attributes and reduce permissions to read-only.

This behavior is undesirable and unnecessarily complicates code which
needs to reduce permissions; instead of just returning the desired
permissions, it has to ensure that the permissions in the attribute
variable declaration only reflect the minimal permissions ever needed.

Change semantics of is_visible to only use the permissions returned
from it instead of oring the returned value with the hard-coded
permissions.

Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 fs/sysfs/group.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 7d2a860..305eccb 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -41,7 +41,7 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 
 	if (grp->attrs) {
 		for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
-			umode_t mode = 0;
+			umode_t mode = (*attr)->mode;
 
 			/*
 			 * In update mode, we're changing the permissions or
@@ -56,8 +56,7 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 					continue;
 			}
 			error = sysfs_add_file_mode_ns(parent, *attr, false,
-						       (*attr)->mode | mode,
-						       NULL);
+						       mode, NULL);
 			if (unlikely(error))
 				break;
 		}
-- 
2.1.0

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

[Guenter Roeck]

For sysfs file attributes, only read and write permisssions make sense.
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);
+
+			mode &= S_IRUGO | S_IWUGO | SYSFS_PREALLOC;
 			error = sysfs_add_file_mode_ns(parent, *attr, false,
 						       mode, NULL);
 			if (unlikely(error))
-- 
2.1.0

[PATCH 3/3] sysfs: Document struct attribute_group

[Guenter Roeck]

Document variables defined in struct attribute_group to ensure
correct usage.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 include/linux/sysfs.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index ddad161..99382c0 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -57,6 +57,21 @@ do {							\
 #define sysfs_attr_init(attr) do {} while (0)
 #endif
 
+/**
+ * struct attribute_group - data structure used to declare an attribute group.
+ * @name:	Optional: Attribute group name
+ *		If specified, the attribute group will be created in
+ *		a new subdirectory with this name.
+ * @is_visible:	Optional: Function to return permissions associated with an
+ *		attribute of the group. Will be called repeatedly for each
+ *		attribute in the group. Only read/write permissions as well as
+ *		SYSFS_PREALLOC are accepted. Must return 0 if an attribute is
+ *		not visible. The returned value will replace static permissions
+ *		defined in struct attribute or struct bin_attribute.
+ * @attrs:	Pointer to NULL terminated list of attributes.
+ * @bin_attrs:	Pointer to NULL terminated list of binary attributes.
+ *		Either attrs or bin_attrs or both must be provided.
+ */
 struct attribute_group {
 	const char		*name;
 	umode_t			(*is_visible)(struct kobject *,
-- 
2.1.0

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

[Vivien Didelot]

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

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

[Guenter Roeck]

On 01/19/2015 04:07 PM, Vivien Didelot wrote:
> 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").
>
Fine with me.

>> +
>> +                        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.

That seems to be too restrictive to me. There are several attributes
(I count 32) which permit group writes (search for "DEVICE_ATTR.*IWGRP").

>
> 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?
>

I have not seen it anywhere, except for execute permissions in
drivers/hid/hid-lg4ff.c (which should be fixed).

Of course, I may have missed some.

> 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);
>
The reason for WARN() was to give the implementer a strong incentive to fix it,
and to show the calling path. Only displaying the attribute name makes it
difficult to identify the culprit, at least for widely used attribute names.

>          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?

Why not ? Not our call to make.

Anyway, my goal was to keep things simple. Taking some bits from the default
and others from the return value of the is_visible function isn't simple,
even more so since your code would require the is_visible function to mask
out SYSFS_PREALLOC to avoid the warning.

[ Note that I don't like SYSFS_PREALLOC to start with; it overloads
   mode and, worse, is identical to S_IFIFO and part of the S_IFMT mask.
   But that is a different issue. ]

Thanks,
Guenter

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

[Vivien Didelot]

Hi Guenter,

>>> @@ -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").
>
> Fine with me.
>
>>> +
>>> +                        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.
>
> That seems to be too restrictive to me. There are several attributes
> (I count 32) which permit group writes (search for "DEVICE_ATTR.*IWGRP").
>
>>
>> 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?
>>
>
> I have not seen it anywhere, except for execute permissions in
> drivers/hid/hid-lg4ff.c (which should be fixed).

Fixed and merged ;)

> Of course, I may have missed some.

>> 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);
>>
> The reason for WARN() was to give the implementer a strong incentive to fix it,
> and to show the calling path. Only displaying the attribute name makes it
> difficult to identify the culprit, at least for widely used attribute names.

No objection with WARN(), I just decreased it to pr_warn() for testing.

>>          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?

> Why not ? Not our call to make.

I was concerned about attributes with group write permission, but you are
right, this is another discussion.

> Anyway, my goal was to keep things simple. Taking some bits from the default
> and others from the return value of the is_visible function isn't simple,
> even more so since your code would require the is_visible function to mask
> out SYSFS_PREALLOC to avoid the warning.

While I'm still not sure about the consequences of flipping this SYSFS_PREALLOC
bit at runtime, I do agree with your goal.

Then to keep it simple, the scope of is_visible could be limited to any bit
allowed at attribute declaration (using *_ATTR* macros). The compile-time check
macro VERIFY_OCTAL_PERMISSIONS() allows any bit but S_IWOTH. The scope can be
SYSFS_PREALLOC | 0775. (or 0664 if we want to avoid executables as well.)

[ This will prevent some follow-up patches "avoid world-writable sysfs files".
In the future, we may want a runtime equivalent of VERIFY_OCTAL_PERMISSIONS. ]

Thanks,
-v

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

[Guenter Roeck]

On Tue, Jan 20, 2015 at 10:44:01AM -0500, Vivien Didelot wrote:
> Hi Guenter,
> 
[ ... ]
> 
> > Anyway, my goal was to keep things simple. Taking some bits from the default
> > and others from the return value of the is_visible function isn't simple,
> > even more so since your code would require the is_visible function to mask
> > out SYSFS_PREALLOC to avoid the warning.
> 
> While I'm still not sure about the consequences of flipping this SYSFS_PREALLOC
> bit at runtime, I do agree with your goal.
> 
> Then to keep it simple, the scope of is_visible could be limited to any bit
> allowed at attribute declaration (using *_ATTR* macros). The compile-time check
> macro VERIFY_OCTAL_PERMISSIONS() allows any bit but S_IWOTH. The scope can be
> SYSFS_PREALLOC | 0775. (or 0664 if we want to avoid executables as well.)
> 
> [ This will prevent some follow-up patches "avoid world-writable sysfs files".
> In the future, we may want a runtime equivalent of VERIFY_OCTAL_PERMISSIONS. ]
> 
0775 and 0664 are both fine with me, with a preference for 0664. Before I
resubmit - Greg, any preference from your side ?

Thanks,
Guenter

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

[Greg Kroah-Hartman]

On Tue, Jan 20, 2015 at 09:13:12AM -0800, Guenter Roeck wrote:
> On Tue, Jan 20, 2015 at 10:44:01AM -0500, Vivien Didelot wrote:
> > Hi Guenter,
> > 
> [ ... ]
> > 
> > > Anyway, my goal was to keep things simple. Taking some bits from the default
> > > and others from the return value of the is_visible function isn't simple,
> > > even more so since your code would require the is_visible function to mask
> > > out SYSFS_PREALLOC to avoid the warning.
> > 
> > While I'm still not sure about the consequences of flipping this SYSFS_PREALLOC
> > bit at runtime, I do agree with your goal.
> > 
> > Then to keep it simple, the scope of is_visible could be limited to any bit
> > allowed at attribute declaration (using *_ATTR* macros). The compile-time check
> > macro VERIFY_OCTAL_PERMISSIONS() allows any bit but S_IWOTH. The scope can be
> > SYSFS_PREALLOC | 0775. (or 0664 if we want to avoid executables as well.)
> > 
> > [ This will prevent some follow-up patches "avoid world-writable sysfs files".
> > In the future, we may want a runtime equivalent of VERIFY_OCTAL_PERMISSIONS. ]
> > 
> 0775 and 0664 are both fine with me, with a preference for 0664. Before I
> resubmit - Greg, any preference from your side ?

I don't have the time to look at them this week, so feel free to fix up
what you know about and resend and I will get to them as soon as I can.

thanks,

greg k-h