Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1328242 > unrolled thread

[PATCH 0/2] Lustre debugfs fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] Lustre debugfs fixes green@linuxhacker.ru - 2016-02-06 08:40 +0100
    [PATCH 1/2] staging/lustre/libcfs: Properly handle debugfs read- and write-only files green@linuxhacker.ru - 2016-02-06 08:40 +0100
    Re: [PATCH 0/2] Lustre debugfs fixes Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-02-07 22:40 +0100
      Re: [PATCH 0/2] Lustre debugfs fixes Oleg Drokin <green@linuxhacker.ru> - 2016-02-08 01:00 +0100
        Re: [PATCH 0/2] Lustre debugfs fixes Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-02-08 03:30 +0100

#1328242 — [PATCH 0/2] Lustre debugfs fixes

From: Oleg Drokin <green@linuxhacker.ru>

These two patches tie some loose ends from the Lustre debugfs conversion,
but while investigating them I also accumulated some questions
that would be good to get answers for.

1. Unlike procfs, debugfs does not really guard your back and if root
comes in and tries to write to a readonly file (or read a write-only one),
it's allowed (as are permission changes too) as long as the appropriate write
(or read) method is provided.
So apparently there's whole class of bugs related to this, sample
exhibits are in e.g. acpi_ec_add_debugfs creating a totally noop module
parameter to control writes that does not really prevent any writes
(patch submitted separately).
But also things like wil_debugfs_create_iomem_x32 where when called from
e.g. wil6210_debugfs_init_offset, some read-only attributed get a generic
write method that would write straight to hardware registers (who knows
what would happen when you write there, possibly they are readonly, but
you are not getting an error).
At first it looked like an easy way to catch this would be to just check
for RO/WO mode with write/read handler set, but this is thwarted by
the simple attribute defines that always assign read and write methods,
but do the check internally for the get/set method instead.
But also some fault injection code that sets readonly access on some files,
but provides a fully functional write method that works as desired.

Would it make sense to redo the simple-attribute framework to easy such
cases detection (and also update writeable attributes to have permissions
reflecting this) and have a correspinding kernel debug compile option
to check for these?

2. I noticed we exported some of the presumably GPL-only debugfs
functionality with plain EXPORT_SYMBOL, so the second patch rectifies this.
Now, I also see that drm_debugfs_create_files allows anybody to
insert any debugfs file anywhere and it is a non-gpl EXPORT_SYMBOL as well,
should it be converted too, or is it sysfs access only that is restricted?

Oleg Drokin (2):
  staging/lustre/libcfs: Properly handle debugfs read- and write-only
    files
  staging/lustre/obdclass: export debugfs functionality for GPL only.

 drivers/staging/lustre/lustre/libcfs/module.c      | 27 ++++++++++++++++++++--
 .../lustre/lustre/obdclass/lprocfs_status.c        | 18 +++++++--------
 2 files changed, 34 insertions(+), 11 deletions(-)

-- 
2.1.0

[toc] | [next] | [standalone]

#1328243 — [PATCH 1/2] staging/lustre/libcfs: Properly handle debugfs read- and write-only files

From: Oleg Drokin <green@linuxhacker.ru>

It turns out that unlike procfs, debugfs does not really enforce
permissions for root (similar to regular filesystems), so we need
to ensure we are not providing ->write() method to read-only files
and ->read() method for write-only files at registration.

This fixes a couple of crashes on unexpected access.

Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
 drivers/staging/lustre/lustre/libcfs/module.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lustre/libcfs/module.c b/drivers/staging/lustre/lustre/libcfs/module.c
index d5a047b..611607a 100644
--- a/drivers/staging/lustre/lustre/libcfs/module.c
+++ b/drivers/staging/lustre/lustre/libcfs/module.c
@@ -502,13 +502,36 @@ static ssize_t lnet_debugfs_write(struct file *filp, const char __user *buf,
 	return error;
 }
 
-static const struct file_operations lnet_debugfs_file_operations = {
+static const struct file_operations lnet_debugfs_file_operations_rw = {
 	.open		= simple_open,
 	.read		= lnet_debugfs_read,
 	.write		= lnet_debugfs_write,
 	.llseek		= default_llseek,
 };
 
+static const struct file_operations lnet_debugfs_file_operations_ro = {
+	.open		= simple_open,
+	.read		= lnet_debugfs_read,
+	.llseek		= default_llseek,
+};
+
+static const struct file_operations lnet_debugfs_file_operations_wo = {
+	.open		= simple_open,
+	.write		= lnet_debugfs_write,
+	.llseek		= default_llseek,
+};
+
+static const struct file_operations *lnet_debugfs_fops_select(umode_t mode)
+{
+	if (!(mode & S_IWUGO))
+		return &lnet_debugfs_file_operations_ro;
+
+	if (!(mode & S_IRUGO))
+		return &lnet_debugfs_file_operations_wo;
+
+	return &lnet_debugfs_file_operations_rw;
+}
+
 void lustre_insert_debugfs(struct ctl_table *table,
 			   const struct lnet_debugfs_symlink_def *symlinks)
 {
@@ -525,7 +548,7 @@ void lustre_insert_debugfs(struct ctl_table *table,
 	for (; table->procname; table++)
 		debugfs_create_file(table->procname, table->mode,
 				    lnet_debugfs_root, table,
-				    &lnet_debugfs_file_operations);
+				    lnet_debugfs_fops_select(table->mode));
 
 	for (; symlinks && symlinks->name; symlinks++)
 		debugfs_create_symlink(symlinks->name, lnet_debugfs_root,
-- 
2.1.0

[toc] | [prev] | [next] | [standalone]

#1328606

On Sat, Feb 06, 2016 at 02:01:49AM -0500, green@linuxhacker.ru wrote:
> From: Oleg Drokin <green@linuxhacker.ru>
> 
> These two patches tie some loose ends from the Lustre debugfs conversion,
> but while investigating them I also accumulated some questions
> that would be good to get answers for.
> 
> 1. Unlike procfs, debugfs does not really guard your back and if root
> comes in and tries to write to a readonly file (or read a write-only one),
> it's allowed (as are permission changes too) as long as the appropriate write
> (or read) method is provided.
> So apparently there's whole class of bugs related to this, sample
> exhibits are in e.g. acpi_ec_add_debugfs creating a totally noop module
> parameter to control writes that does not really prevent any writes
> (patch submitted separately).
> But also things like wil_debugfs_create_iomem_x32 where when called from
> e.g. wil6210_debugfs_init_offset, some read-only attributed get a generic
> write method that would write straight to hardware registers (who knows
> what would happen when you write there, possibly they are readonly, but
> you are not getting an error).
> At first it looked like an easy way to catch this would be to just check
> for RO/WO mode with write/read handler set, but this is thwarted by
> the simple attribute defines that always assign read and write methods,
> but do the check internally for the get/set method instead.
> But also some fault injection code that sets readonly access on some files,
> but provides a fully functional write method that works as desired.
> 
> Would it make sense to redo the simple-attribute framework to easy such
> cases detection (and also update writeable attributes to have permissions
> reflecting this) and have a correspinding kernel debug compile option
> to check for these?

If a developer provides the write hooks for a debugfs file, and
userspace changes the permissions to write to it, why would you prevent
this?  Perhaps this is what is intended.

Remember, debugfs is only for debugging stuff, never rely on it for
actual device/system use.

> 2. I noticed we exported some of the presumably GPL-only debugfs
> functionality with plain EXPORT_SYMBOL, so the second patch rectifies this.

Thanks.

> Now, I also see that drm_debugfs_create_files allows anybody to
> insert any debugfs file anywhere and it is a non-gpl EXPORT_SYMBOL as well,
> should it be converted too, or is it sysfs access only that is restricted?

No, these should be as well, a patch for that would be great, thanks.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1328658

On Feb 7, 2016, at 4:39 PM, Greg Kroah-Hartman wrote:

> On Sat, Feb 06, 2016 at 02:01:49AM -0500, green@linuxhacker.ru wrote:
>> From: Oleg Drokin <green@linuxhacker.ru>
>> 
>> These two patches tie some loose ends from the Lustre debugfs conversion,
>> but while investigating them I also accumulated some questions
>> that would be good to get answers for.
>> 
>> 1. Unlike procfs, debugfs does not really guard your back and if root
>> comes in and tries to write to a readonly file (or read a write-only one),
>> it's allowed (as are permission changes too) as long as the appropriate write
>> (or read) method is provided.
>> So apparently there's whole class of bugs related to this, sample
>> exhibits are in e.g. acpi_ec_add_debugfs creating a totally noop module
>> parameter to control writes that does not really prevent any writes
>> (patch submitted separately).
>> But also things like wil_debugfs_create_iomem_x32 where when called from
>> e.g. wil6210_debugfs_init_offset, some read-only attributed get a generic
>> write method that would write straight to hardware registers (who knows
>> what would happen when you write there, possibly they are readonly, but
>> you are not getting an error).
>> At first it looked like an easy way to catch this would be to just check
>> for RO/WO mode with write/read handler set, but this is thwarted by
>> the simple attribute defines that always assign read and write methods,
>> but do the check internally for the get/set method instead.
>> But also some fault injection code that sets readonly access on some files,
>> but provides a fully functional write method that works as desired.
>> 
>> Would it make sense to redo the simple-attribute framework to easy such
>> cases detection (and also update writeable attributes to have permissions
>> reflecting this) and have a correspinding kernel debug compile option
>> to check for these?
> 
> If a developer provides the write hooks for a debugfs file, and
> userspace changes the permissions to write to it, why would you prevent
> this?  Perhaps this is what is intended.

Well, it works differently for procfs where you cannot really change the permissions.
I understand the developer might envision permission changes (or even ignoring of
permissions by root) and there is such a code out there even.
But DEFINE_SIMPLE_ATTRIBUTE code for one always provides both read and write methods
and then internally checks if get and set are available and returns -EACCESS if not.
That's totally ok too (other than -EACCESS that I think it not permitted as an errno
from read/write with EINVAL being used for that), I am just trying to figure out if
there is a way to more automatically detect cases where the write or read access is
not desired and is left there by mistake.

> Remember, debugfs is only for debugging stuff, never rely on it for
> actual device/system use.

Yes, I understand that. But debugfs is on by default pretty much anywhere and if
there's a bug that lets you write to some hardware register you were supposed to only
read and get some elevated privileges, or you read some write-only file and crash
the kernel - that's bad too.
We have kernel debugging options to check for all sorts of stuff, like use after free,
double unlocks and so on.
This sounded like another class of bugs that would have benefitted from that.

>> Now, I also see that drm_debugfs_create_files allows anybody to
>> insert any debugfs file anywhere and it is a non-gpl EXPORT_SYMBOL as well,
>> should it be converted too, or is it sysfs access only that is restricted?
> No, these should be as well, a patch for that would be great, thanks.

Done.

Bye,
    Oleg

[toc] | [prev] | [next] | [standalone]

#1328707

On Sun, Feb 07, 2016 at 06:51:09PM -0500, Oleg Drokin wrote:
> 
> On Feb 7, 2016, at 4:39 PM, Greg Kroah-Hartman wrote:
> 
> > On Sat, Feb 06, 2016 at 02:01:49AM -0500, green@linuxhacker.ru wrote:
> >> From: Oleg Drokin <green@linuxhacker.ru>
> >> 
> >> These two patches tie some loose ends from the Lustre debugfs conversion,
> >> but while investigating them I also accumulated some questions
> >> that would be good to get answers for.
> >> 
> >> 1. Unlike procfs, debugfs does not really guard your back and if root
> >> comes in and tries to write to a readonly file (or read a write-only one),
> >> it's allowed (as are permission changes too) as long as the appropriate write
> >> (or read) method is provided.
> >> So apparently there's whole class of bugs related to this, sample
> >> exhibits are in e.g. acpi_ec_add_debugfs creating a totally noop module
> >> parameter to control writes that does not really prevent any writes
> >> (patch submitted separately).
> >> But also things like wil_debugfs_create_iomem_x32 where when called from
> >> e.g. wil6210_debugfs_init_offset, some read-only attributed get a generic
> >> write method that would write straight to hardware registers (who knows
> >> what would happen when you write there, possibly they are readonly, but
> >> you are not getting an error).
> >> At first it looked like an easy way to catch this would be to just check
> >> for RO/WO mode with write/read handler set, but this is thwarted by
> >> the simple attribute defines that always assign read and write methods,
> >> but do the check internally for the get/set method instead.
> >> But also some fault injection code that sets readonly access on some files,
> >> but provides a fully functional write method that works as desired.
> >> 
> >> Would it make sense to redo the simple-attribute framework to easy such
> >> cases detection (and also update writeable attributes to have permissions
> >> reflecting this) and have a correspinding kernel debug compile option
> >> to check for these?
> > 
> > If a developer provides the write hooks for a debugfs file, and
> > userspace changes the permissions to write to it, why would you prevent
> > this?  Perhaps this is what is intended.
> 
> Well, it works differently for procfs where you cannot really change the permissions.
> I understand the developer might envision permission changes (or even ignoring of
> permissions by root) and there is such a code out there even.
> But DEFINE_SIMPLE_ATTRIBUTE code for one always provides both read and write methods
> and then internally checks if get and set are available and returns -EACCESS if not.
> That's totally ok too (other than -EACCESS that I think it not permitted as an errno
> from read/write with EINVAL being used for that), I am just trying to figure out if
> there is a way to more automatically detect cases where the write or read access is
> not desired and is left there by mistake.

I don't really know, the SIMPLE_ATTRIBUTE() stuff was just a way to try
to reduce a ton of boiler-plate code that was all over the place.  I
doubt anyone thought of this when it was added (I know I didn't.)

> > Remember, debugfs is only for debugging stuff, never rely on it for
> > actual device/system use.
> 
> Yes, I understand that. But debugfs is on by default pretty much anywhere

But it's root-only, thankfully.  If only ftrace wouldn't use it, distros
could turn it off entirely :(

> and if there's a bug that lets you write to some hardware register you
> were supposed to only read and get some elevated privileges, or you
> read some write-only file and crash the kernel - that's bad too.

I agree, again, which is why it's mounted root-only.  Also, bad things
happen when you remove debugfs files while someone has a file open :)

thanks,

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web