Messages in this thread | | | Date | Tue, 20 Jan 2015 10:44:01 -0500 (EST) | From | Vivien Didelot <> | Subject | Re: [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
| |