Re: [PATCH 2/3] sysfs: Only accept read/write permissions for file attributes
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 -- 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/
Re: [PATCH 2/3] sysfs: Only accept read/write permissions for file attributes
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 -- 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/
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
--
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/
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 -- 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/
Re: [PATCH 2/3] sysfs: Only accept read/write permissions for file attributes
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 -- 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/
Re: [PATCH 2/3] sysfs: Only accept read/write permissions for file attributes
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 -- 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/
Re: [PATCH 2/3] sysfs: Only accept read/write permissions for file attributes
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
Signed-off-by: Guenter Roeck
---
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
--
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/
Re: [PATCH 2/3] sysfs: Only accept read/write permissions for file attributes
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
> Signed-off-by: Guenter Roeck
> ---
> 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
--
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/
Re: [PATCH 2/3] sysfs: Only accept read/write permissions for file attributes
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.dide...@savoirfairelinux.com Signed-off-by: Guenter Roeck li...@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 -- 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/
Re: [PATCH 2/3] sysfs: Only accept read/write permissions for file attributes
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.dide...@savoirfairelinux.com Signed-off-by: Guenter Roeck li...@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 -- 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/
