[lustre-devel] [PATCH 0/2] Lustre debugfs fixes

[lustre-devel] [PATCH 0/2] Lustre debugfs fixes

[green at linuxhacker.ru]

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

[PATCH 0/2] Lustre debugfs fixes

[green]

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

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

[green at linuxhacker.ru]

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

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

[green]

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

[lustre-devel] [PATCH 2/2] staging/lustre/obdclass: export debugfs functionality for GPL only.

[green at linuxhacker.ru]

From: Oleg Drokin <green@linuxhacker.ru>

Turns out we mistakenly export some pretty-wide-reaching debugfs
functions as EXPORT_SYMBOL instead of EXPORT_SYMBOL_GPL as we should,
so this patch rectifies the situation.

Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
 .../staging/lustre/lustre/obdclass/lprocfs_status.c    | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index b65ad93..9a1434d 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -261,7 +261,7 @@ struct dentry *ldebugfs_add_simple(struct dentry *root,
 	}
 	return entry;
 }
-EXPORT_SYMBOL(ldebugfs_add_simple);
+EXPORT_SYMBOL_GPL(ldebugfs_add_simple);
 
 static struct file_operations lprocfs_generic_fops = { };
 
@@ -294,14 +294,14 @@ int ldebugfs_add_vars(struct dentry *parent,
 	}
 	return 0;
 }
-EXPORT_SYMBOL(ldebugfs_add_vars);
+EXPORT_SYMBOL_GPL(ldebugfs_add_vars);
 
 void ldebugfs_remove(struct dentry **entryp)
 {
 	debugfs_remove_recursive(*entryp);
 	*entryp = NULL;
 }
-EXPORT_SYMBOL(ldebugfs_remove);
+EXPORT_SYMBOL_GPL(ldebugfs_remove);
 
 struct dentry *ldebugfs_register(const char *name,
 				 struct dentry *parent,
@@ -327,7 +327,7 @@ struct dentry *ldebugfs_register(const char *name,
 out:
 	return entry;
 }
-EXPORT_SYMBOL(ldebugfs_register);
+EXPORT_SYMBOL_GPL(ldebugfs_register);
 
 /* Generic callbacks */
 int lprocfs_rd_uint(struct seq_file *m, void *data)
@@ -942,7 +942,7 @@ int lprocfs_obd_setup(struct obd_device *obd, struct lprocfs_vars *list,
 
 	return rc;
 }
-EXPORT_SYMBOL(lprocfs_obd_setup);
+EXPORT_SYMBOL_GPL(lprocfs_obd_setup);
 
 int lprocfs_obd_cleanup(struct obd_device *obd)
 {
@@ -957,7 +957,7 @@ int lprocfs_obd_cleanup(struct obd_device *obd)
 
 	return 0;
 }
-EXPORT_SYMBOL(lprocfs_obd_cleanup);
+EXPORT_SYMBOL_GPL(lprocfs_obd_cleanup);
 
 int lprocfs_stats_alloc_one(struct lprocfs_stats *stats, unsigned int cpuid)
 {
@@ -1219,7 +1219,7 @@ int ldebugfs_register_stats(struct dentry *parent, const char *name,
 
 	return 0;
 }
-EXPORT_SYMBOL(ldebugfs_register_stats);
+EXPORT_SYMBOL_GPL(ldebugfs_register_stats);
 
 void lprocfs_counter_init(struct lprocfs_stats *stats, int index,
 			  unsigned conf, const char *name, const char *units)
@@ -1446,7 +1446,7 @@ int ldebugfs_seq_create(struct dentry *parent,
 
 	return 0;
 }
-EXPORT_SYMBOL(ldebugfs_seq_create);
+EXPORT_SYMBOL_GPL(ldebugfs_seq_create);
 
 int ldebugfs_obd_seq_create(struct obd_device *dev,
 			    const char *name,
@@ -1457,7 +1457,7 @@ int ldebugfs_obd_seq_create(struct obd_device *dev,
 	return ldebugfs_seq_create(dev->obd_debugfs_entry, name,
 				   mode, seq_fops, data);
 }
-EXPORT_SYMBOL(ldebugfs_obd_seq_create);
+EXPORT_SYMBOL_GPL(ldebugfs_obd_seq_create);
 
 void lprocfs_oh_tally(struct obd_histogram *oh, unsigned int value)
 {
-- 
2.1.0

[PATCH 2/2] staging/lustre/obdclass: export debugfs functionality for GPL only.

[green]

From: Oleg Drokin <green@linuxhacker.ru>

Turns out we mistakenly export some pretty-wide-reaching debugfs
functions as EXPORT_SYMBOL instead of EXPORT_SYMBOL_GPL as we should,
so this patch rectifies the situation.

Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
 .../staging/lustre/lustre/obdclass/lprocfs_status.c    | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index b65ad93..9a1434d 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -261,7 +261,7 @@ struct dentry *ldebugfs_add_simple(struct dentry *root,
 	}
 	return entry;
 }
-EXPORT_SYMBOL(ldebugfs_add_simple);
+EXPORT_SYMBOL_GPL(ldebugfs_add_simple);
 
 static struct file_operations lprocfs_generic_fops = { };
 
@@ -294,14 +294,14 @@ int ldebugfs_add_vars(struct dentry *parent,
 	}
 	return 0;
 }
-EXPORT_SYMBOL(ldebugfs_add_vars);
+EXPORT_SYMBOL_GPL(ldebugfs_add_vars);
 
 void ldebugfs_remove(struct dentry **entryp)
 {
 	debugfs_remove_recursive(*entryp);
 	*entryp = NULL;
 }
-EXPORT_SYMBOL(ldebugfs_remove);
+EXPORT_SYMBOL_GPL(ldebugfs_remove);
 
 struct dentry *ldebugfs_register(const char *name,
 				 struct dentry *parent,
@@ -327,7 +327,7 @@ struct dentry *ldebugfs_register(const char *name,
 out:
 	return entry;
 }
-EXPORT_SYMBOL(ldebugfs_register);
+EXPORT_SYMBOL_GPL(ldebugfs_register);
 
 /* Generic callbacks */
 int lprocfs_rd_uint(struct seq_file *m, void *data)
@@ -942,7 +942,7 @@ int lprocfs_obd_setup(struct obd_device *obd, struct lprocfs_vars *list,
 
 	return rc;
 }
-EXPORT_SYMBOL(lprocfs_obd_setup);
+EXPORT_SYMBOL_GPL(lprocfs_obd_setup);
 
 int lprocfs_obd_cleanup(struct obd_device *obd)
 {
@@ -957,7 +957,7 @@ int lprocfs_obd_cleanup(struct obd_device *obd)
 
 	return 0;
 }
-EXPORT_SYMBOL(lprocfs_obd_cleanup);
+EXPORT_SYMBOL_GPL(lprocfs_obd_cleanup);
 
 int lprocfs_stats_alloc_one(struct lprocfs_stats *stats, unsigned int cpuid)
 {
@@ -1219,7 +1219,7 @@ int ldebugfs_register_stats(struct dentry *parent, const char *name,
 
 	return 0;
 }
-EXPORT_SYMBOL(ldebugfs_register_stats);
+EXPORT_SYMBOL_GPL(ldebugfs_register_stats);
 
 void lprocfs_counter_init(struct lprocfs_stats *stats, int index,
 			  unsigned conf, const char *name, const char *units)
@@ -1446,7 +1446,7 @@ int ldebugfs_seq_create(struct dentry *parent,
 
 	return 0;
 }
-EXPORT_SYMBOL(ldebugfs_seq_create);
+EXPORT_SYMBOL_GPL(ldebugfs_seq_create);
 
 int ldebugfs_obd_seq_create(struct obd_device *dev,
 			    const char *name,
@@ -1457,7 +1457,7 @@ int ldebugfs_obd_seq_create(struct obd_device *dev,
 	return ldebugfs_seq_create(dev->obd_debugfs_entry, name,
 				   mode, seq_fops, data);
 }
-EXPORT_SYMBOL(ldebugfs_obd_seq_create);
+EXPORT_SYMBOL_GPL(ldebugfs_obd_seq_create);
 
 void lprocfs_oh_tally(struct obd_histogram *oh, unsigned int value)
 {
-- 
2.1.0

[lustre-devel] [PATCH 0/2] Lustre debugfs fixes

[Greg Kroah-Hartman]

On Sat, Feb 06, 2016 at 02:01:49AM -0500, green at 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

Re: [PATCH 0/2] Lustre debugfs fixes

[Greg Kroah-Hartman]

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

[lustre-devel] [PATCH 0/2] Lustre debugfs fixes

[Oleg Drokin]

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

> On Sat, Feb 06, 2016 at 02:01:49AM -0500, green at 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

Re: [PATCH 0/2] Lustre debugfs fixes

[Oleg Drokin]

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

[lustre-devel] [PATCH 0/2] Lustre debugfs fixes

[Greg Kroah-Hartman]

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 at 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

Re: [PATCH 0/2] Lustre debugfs fixes

[Greg Kroah-Hartman]

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