lkml.org 
[lkml]   [2015]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/3] sysfs: Only accept read/write permissions for file attributes
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


\
 
 \ /
  Last update: 2015-01-20 17:21    [W:0.119 / U:1.140 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site