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