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

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

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