debugfs & vfs file permission issue?

debugfs & vfs file permission issue?

[Robin Getz]

On 2.6.28-rc2, If I create a debugfs file with

    debugfs_create_x16("SPORT1_TX", 0200 , parent, 0xFFC00910);

Although the file shows up as write only (no read):

root:/> ls -l /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
--w-------    1 root     root            0 Jan  1  
2007 /sys/kernel/debug/blackfin/SPORT/SPORT1_TX

root:/> cat /sys/kernel/debug/blackfin/SPORT/SPORT1_TX

Still works - and causes the read to occur, which crashes :(

System MMR Error
.....

I can do the same to any file by hand too.

root:/> ls -l /sys/kernel/debug/blackfin/RTC/RTC_STAT
-rw-------    1 root     root            0 Jan  1  
2007 /sys/kernel/debug/blackfin/RTC/RTC_STAT
root:/> cat /sys/kernel/debug/blackfin/RTC/RTC_STAT
0xb81d3181
root:/> chmod 0000 /sys/kernel/debug/blackfin/RTC/RTC_STAT
root:/> ls -l /sys/kernel/debug/blackfin/RTC/RTC_STAT
----------    1 root     root            0 Jan  1  
2007 /sys/kernel/debug/blackfin/RTC/RTC_STAT
root:/> cat /sys/kernel/debug/blackfin/RTC/RTC_STAT
0xb81d31a5

?

>From what I can tell from the call trace:

cat (userspace)
  system_call (into kernel)
    sys_read
      vfs_read
        simple_attr_read
          debugfs_u16_get
            crash


I would think that vfs_read should fail....

./fs/read_write.c:vfs_read()

        if (!(file->f_mode & FMODE_READ))
                return -EBADF; 

I don't understand while the inode that is created 

fs/debugfs/inode.c:debugfs_get_inode()

     inode->i_mode = mode; 

isn't setting the f_mode properly.

Any suggestions?

Thanks

Re: debugfs & vfs file permission issue?

[Greg KH]

On Mon, Jan 05, 2009 at 09:57:07PM -0500, Robin Getz wrote:
> On 2.6.28-rc2, If I create a debugfs file with
> 
>     debugfs_create_x16("SPORT1_TX", 0200 , parent, 0xFFC00910);

Um, are you are passing in a pointer to a known memory location
properly?  Why would it be ok for the kernel to directly read that
location?

> Although the file shows up as write only (no read):
> 
> root:/> ls -l /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
> --w-------    1 root     root            0 Jan  1  
> 2007 /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
> 
> root:/> cat /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
> 
> Still works - and causes the read to occur, which crashes :(

You're root, you can do anything :)


> System MMR Error
> .....
> 
> I can do the same to any file by hand too.
> 
> root:/> ls -l /sys/kernel/debug/blackfin/RTC/RTC_STAT
> -rw-------    1 root     root            0 Jan  1  
> 2007 /sys/kernel/debug/blackfin/RTC/RTC_STAT
> root:/> cat /sys/kernel/debug/blackfin/RTC/RTC_STAT
> 0xb81d3181
> root:/> chmod 0000 /sys/kernel/debug/blackfin/RTC/RTC_STAT
> root:/> ls -l /sys/kernel/debug/blackfin/RTC/RTC_STAT
> ----------    1 root     root            0 Jan  1  
> 2007 /sys/kernel/debug/blackfin/RTC/RTC_STAT
> root:/> cat /sys/kernel/debug/blackfin/RTC/RTC_STAT
> 0xb81d31a5
> 
> ?
> 
> From what I can tell from the call trace:
> 
> cat (userspace)
>   system_call (into kernel)
>     sys_read
>       vfs_read
>         simple_attr_read
>           debugfs_u16_get
>             crash
> 
> 
> I would think that vfs_read should fail....
> 
> ./fs/read_write.c:vfs_read()
> 
>         if (!(file->f_mode & FMODE_READ))
>                 return -EBADF; 

I think f_mode is the mode of the file descriptor, not the inode you are
looking at.  You told userspace to open the file in read mode, right?
So this is why it passes.

Hope this helps,

greg k-h

Re: debugfs & vfs file permission issue?

[Mike Frysinger]

On Tue, Jan 6, 2009 at 00:48, Greg KH wrote:
> On Mon, Jan 05, 2009 at 09:57:07PM -0500, Robin Getz wrote:
>> On 2.6.28-rc2, If I create a debugfs file with
>>
>>     debugfs_create_x16("SPORT1_TX", 0200 , parent, 0xFFC00910);
>
> Um, are you are passing in a pointer to a known memory location
> properly?  Why would it be ok for the kernel to directly read that
> location?

it's a nommu system and the 0xffc00000+ addresses are always available

>> Although the file shows up as write only (no read):
>>
>> root:/> ls -l /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
>> --w-------    1 root     root            0 Jan  1
>> 2007 /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
>>
>> root:/> cat /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
>>
>> Still works - and causes the read to occur, which crashes :(
>
> You're root, you can do anything :)

any thoughts on how to declare debugfs files that are read or write
only ?  we'll have to add new helper functions or have it be a
parameter or declare our own debugfs file ?
-mike

Re: debugfs & vfs file permission issue?

[Greg KH]

On Tue, Jan 06, 2009 at 12:54:58AM -0500, Mike Frysinger wrote:
> On Tue, Jan 6, 2009 at 00:48, Greg KH wrote:
> > On Mon, Jan 05, 2009 at 09:57:07PM -0500, Robin Getz wrote:
> >> On 2.6.28-rc2, If I create a debugfs file with
> >>
> >>     debugfs_create_x16("SPORT1_TX", 0200 , parent, 0xFFC00910);
> >
> > Um, are you are passing in a pointer to a known memory location
> > properly?  Why would it be ok for the kernel to directly read that
> > location?
> 
> it's a nommu system and the 0xffc00000+ addresses are always available

Writable but not readable?  Isn't hardware fun :)

> >> Although the file shows up as write only (no read):
> >>
> >> root:/> ls -l /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
> >> --w-------    1 root     root            0 Jan  1
> >> 2007 /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
> >>
> >> root:/> cat /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
> >>
> >> Still works - and causes the read to occur, which crashes :(
> >
> > You're root, you can do anything :)
> 
> any thoughts on how to declare debugfs files that are read or write
> only ?  we'll have to add new helper functions or have it be a
> parameter or declare our own debugfs file ?

Just use debugfs_create_file() and use your own read/write functions to
prevent a read or write from happening no matter what.  No new debugfs
infrastructure should be needed.

good luck,

greg k-h

Re: debugfs & vfs file permission issue?

[Mike Frysinger]

On Tue, Jan 6, 2009 at 01:19, Greg KH wrote:
> On Tue, Jan 06, 2009 at 12:54:58AM -0500, Mike Frysinger wrote:
>> On Tue, Jan 6, 2009 at 00:48, Greg KH wrote:
>> > On Mon, Jan 05, 2009 at 09:57:07PM -0500, Robin Getz wrote:
>> >> On 2.6.28-rc2, If I create a debugfs file with
>> >>
>> >>     debugfs_create_x16("SPORT1_TX", 0200 , parent, 0xFFC00910);
>> >
>> > Um, are you are passing in a pointer to a known memory location
>> > properly?  Why would it be ok for the kernel to directly read that
>> > location?
>>
>> it's a nommu system and the 0xffc00000+ addresses are always available
>
> Writable but not readable?  Isn't hardware fun :)

yeah and we have some readable but not writable addresses too :/

>> >> Although the file shows up as write only (no read):
>> >>
>> >> root:/> ls -l /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
>> >> --w-------    1 root     root            0 Jan  1
>> >> 2007 /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
>> >>
>> >> root:/> cat /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
>> >>
>> >> Still works - and causes the read to occur, which crashes :(
>> >
>> > You're root, you can do anything :)
>>
>> any thoughts on how to declare debugfs files that are read or write
>> only ?  we'll have to add new helper functions or have it be a
>> parameter or declare our own debugfs file ?
>
> Just use debugfs_create_file() and use your own read/write functions to
> prevent a read or write from happening no matter what.  No new debugfs
> infrastructure should be needed.

would it be useful for this to be a common function ?  probably not so
much considering (i'm assuming) no one else has asked so far for such
a beast ...
-mike

Re: debugfs & vfs file permission issue?

[Robin Getz]

On Tue 6 Jan 2009 01:32, Mike Frysinger pondered:
> On Tue, Jan 6, 2009 at 01:19, Greg KH wrote:
> > On Tue, Jan 06, 2009 at 12:54:58AM -0500, Mike Frysinger wrote:
> >> On Tue, Jan 6, 2009 at 00:48, Greg KH wrote:
> >> > On Mon, Jan 05, 2009 at 09:57:07PM -0500, Robin Getz wrote:
> >> >> On 2.6.28-rc2, If I create a debugfs file with
> >> >>
> >> >>     debugfs_create_x16("SPORT1_TX", 0200 , parent, 0xFFC00910);
> >> >
> >> > Um, are you are passing in a pointer to a known memory location
> >> > properly?  Why would it be ok for the kernel to directly read that
> >> > location?
> >>
> >> it's a nommu system and the 0xffc00000+ addresses are always available
> >
> > Writable but not readable?  Isn't hardware fun :)
> 
> yeah and we have some readable but not writable addresses too :/
> 
> >> >> Although the file shows up as write only (no read):
> >> >>
> >> >> root:/> ls -l /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
> >> >> --w-------    1 root     root            0 Jan  1
> >> >> 2007 /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
> >> >>
> >> >> root:/> cat /sys/kernel/debug/blackfin/SPORT/SPORT1_TX
> >> >>
> >> >> Still works - and causes the read to occur, which crashes :(
> >> >
> >> > You're root, you can do anything :)
> >>
> >> any thoughts on how to declare debugfs files that are read or write
> >> only ?  we'll have to add new helper functions or have it be a
> >> parameter or declare our own debugfs file ?
> >
> > Just use debugfs_create_file() and use your own read/write functions to
> > prevent a read or write from happening no matter what.  No new debugfs
> > infrastructure should be needed.
> 
> would it be useful for this to be a common function ?  probably not so
> much considering (i'm assuming) no one else has asked so far for such
> a beast ...

Today there is:

file.c:DEFINE_SIMPLE_ATTRIBUTE(fops_u8, debugfs_u8_get, debugfs_u8_set, "%llu\n");
file.c:DEFINE_SIMPLE_ATTRIBUTE(fops_u16, debugfs_u16_get, debugfs_u16_set, "%llu\n");
file.c:DEFINE_SIMPLE_ATTRIBUTE(fops_u32, debugfs_u32_get, debugfs_u32_set, "%llu\n");
file.c:DEFINE_SIMPLE_ATTRIBUTE(fops_u64, debugfs_u64_get, debugfs_u64_set, "%llu\n");
file.c:DEFINE_SIMPLE_ATTRIBUTE(fops_x8, debugfs_u8_get, debugfs_u8_set, "0x%02llx\n");
file.c:DEFINE_SIMPLE_ATTRIBUTE(fops_x16, debugfs_u16_get, debugfs_u16_set, "0x%04llx\n");
file.c:DEFINE_SIMPLE_ATTRIBUTE(fops_x32, debugfs_u32_get, debugfs_u32_set, "0x%08llx\n");

adding a readonly, and writeonly, and ensuring that when you call 
debugfs_create_*, the mode is checked, and the "correct" fops are set 
doesn't seem like it would be a bad idea? This would enforce the
kernel programmer's view on the world, and not allow pesky root users
to override things....

Greg - would you take something like that?

Or do you just want us to it as you suggested previously? and don't use the 
existing debugfs_create_* functions directly on wonky hardware registers?

Thanks
-Robin

Re: debugfs & vfs file permission issue?

[Robin Getz]

On Tue 6 Jan 2009 07:05, Robin Getz suggested:
>
> adding a readonly, and writeonly, and ensuring that when you call 
> debugfs_create_*, the mode is checked, and the "correct" fops are set 
> doesn't seem like it would be a bad idea? This would enforce the
> kernel programmer's view on the world, and not allow pesky root users
> to override things....
> 
> Greg - would you take something like that?

How about this?

Feel free to nak it - we can do the same thing where we are calling the
debugfs_create_* functions - this just makes it cleaner in my opinion.

---

In many SOC implementations there are hardware registers can be read only, 
or write only. This extends the debugfs to enforce the file permissions for
these types of registers, by providing a set of fops which are read only
or write only. This assumes that the kernel developer knows more about the
hardware than the user (even root users) - which is normally true.


Signed-off-by: Robin Getz <rgetz@blackfin.uclinux.org>

---

This fixes things, which use to crash (SPORT1_TX is a write only hardware
register).

root:/sys/kernel/debug/blackfin/SPORT> ls -l SPORT1_TX
--w-------    1 root     root            0 Jan  1  2007 SPORT1_TX
root:/sys/kernel/debug/blackfin/SPORT> cat SPORT1_TX
cat: read error: Permission denied
root:/sys/kernel/debug/blackfin/SPORT> chmod +r SPORT1_TX
root:/sys/kernel/debug/blackfin/SPORT> ls -l SPORT1_TX
-rw-r--r--    1 root     root            0 Jan  1  2007 SPORT1_TX
root:/sys/kernel/debug/blackfin/SPORT> cat SPORT1_TX
cat: read error: Permission denied

Without this patch, things crash - but as Greg suggested - the same thing can
be done other places.


 fs/debugfs/file.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)



Index: fs/debugfs/file.c
===================================================================
--- fs/debugfs/file.c	(revision 5943)
+++ fs/debugfs/file.c	(working copy)
@@ -67,6 +67,8 @@
 	return 0;
 }
 DEFINE_SIMPLE_ATTRIBUTE(fops_u8, debugfs_u8_get, debugfs_u8_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u8_ro, debugfs_u8_get, NULL, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u8_wo, NULL, debugfs_u8_set, "%llu\n");
 
 /**
  * debugfs_create_u8 - create a debugfs file that is used to read and write an unsigned 8-bit value
@@ -95,6 +97,13 @@
 struct dentry *debugfs_create_u8(const char *name, mode_t mode,
 				 struct dentry *parent, u8 *value)
 {
+	/*if there are no write bits set make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u8_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u8_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_u8);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_u8);
@@ -110,6 +119,8 @@
 	return 0;
 }
 DEFINE_SIMPLE_ATTRIBUTE(fops_u16, debugfs_u16_get, debugfs_u16_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u16_ro, debugfs_u16_get, NULL, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u16_wo, NULL, debugfs_u16_set, "%llu\n");
 
 /**
  * debugfs_create_u16 - create a debugfs file that is used to read and write an unsigned 16-bit value
@@ -138,6 +149,13 @@
 struct dentry *debugfs_create_u16(const char *name, mode_t mode,
 				  struct dentry *parent, u16 *value)
 {
+	/*if there are no write bits set make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u16_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u16_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_u16);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_u16);
@@ -153,6 +171,8 @@
 	return 0;
 }
 DEFINE_SIMPLE_ATTRIBUTE(fops_u32, debugfs_u32_get, debugfs_u32_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u32_ro, debugfs_u32_get, NULL, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u32_wo, NULL, debugfs_u32_set, "%llu\n");
 
 /**
  * debugfs_create_u32 - create a debugfs file that is used to read and write an unsigned 32-bit value
@@ -181,6 +201,13 @@
 struct dentry *debugfs_create_u32(const char *name, mode_t mode,
 				 struct dentry *parent, u32 *value)
 {
+	/*if there are no write bits set make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u32_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u32_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_u32);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_u32);
@@ -197,6 +224,8 @@
 	return 0;
 }
 DEFINE_SIMPLE_ATTRIBUTE(fops_u64, debugfs_u64_get, debugfs_u64_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u64_ro, debugfs_u64_get, NULL, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u64_wo, NULL, debugfs_u64_set, "%llu\n");
 
 /**
  * debugfs_create_u64 - create a debugfs file that is used to read and write an unsigned 64-bit value
@@ -225,15 +254,28 @@
 struct dentry *debugfs_create_u64(const char *name, mode_t mode,
 				 struct dentry *parent, u64 *value)
 {
+	/*if there are no write bits set make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u64_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u64_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_u64);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_u64);
 
 DEFINE_SIMPLE_ATTRIBUTE(fops_x8, debugfs_u8_get, debugfs_u8_set, "0x%02llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x8_ro, debugfs_u8_get, NULL, "0x%02llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x8_wo, NULL, debugfs_u8_set, "0x%02llx\n");
 
 DEFINE_SIMPLE_ATTRIBUTE(fops_x16, debugfs_u16_get, debugfs_u16_set, "0x%04llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x16_ro, debugfs_u16_get, NULL, "0x%04llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x16_wo, NULL, debugfs_u16_set, "0x%04llx\n");
 
 DEFINE_SIMPLE_ATTRIBUTE(fops_x32, debugfs_u32_get, debugfs_u32_set, "0x%08llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x32_ro, debugfs_u32_get, NULL, "0x%08llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x32_wo, NULL, debugfs_u32_set, "0x%08llx\n");
 
 /*
  * debugfs_create_x{8,16,32} - create a debugfs file that is used to read and write an unsigned {8,16,32}-bit value
@@ -256,6 +298,13 @@
 struct dentry *debugfs_create_x8(const char *name, mode_t mode,
 				 struct dentry *parent, u8 *value)
 {
+	/*if there are no write bits set make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x8_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x8_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_x8);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_x8);
@@ -273,6 +322,13 @@
 struct dentry *debugfs_create_x16(const char *name, mode_t mode,
 				 struct dentry *parent, u16 *value)
 {
+	/*if there are no write bits set make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x16_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x16_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_x16);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_x16);
@@ -290,6 +346,13 @@
 struct dentry *debugfs_create_x32(const char *name, mode_t mode,
 				 struct dentry *parent, u32 *value)
 {
+	/*if there are no write bits set make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x32_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x32_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_x32);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_x32);

Re: debugfs & vfs file permission issue?

[Mike Frysinger]

On Tue, Jan 6, 2009 at 10:12, Robin Getz wrote:
> On Tue 6 Jan 2009 07:05, Robin Getz suggested:
>> adding a readonly, and writeonly, and ensuring that when you call
>> debugfs_create_*, the mode is checked, and the "correct" fops are set
>> doesn't seem like it would be a bad idea? This would enforce the
>> kernel programmer's view on the world, and not allow pesky root users
>> to override things....
>>
>> Greg - would you take something like that?
>
> How about this?
>
> Feel free to nak it - we can do the same thing where we are calling the
> debugfs_create_* functions - this just makes it cleaner in my opinion.
>
> ---
>
> In many SOC implementations there are hardware registers can be read only,
> or write only. This extends the debugfs to enforce the file permissions for
> these types of registers, by providing a set of fops which are read only
> or write only. This assumes that the kernel developer knows more about the
> hardware than the user (even root users) - which is normally true.

we want it for cpu registers, but i dont see any reason why this
wouldnt also apply to external devices attached via memory interfaces
... fifos and such ...
-mike

Re: debugfs & vfs file permission issue?

[Robin Getz]

On Tue 6 Jan 2009 10:20, Mike Frysinger pondered:
> On Tue, Jan 6, 2009 at 10:12, Robin Getz wrote:
> > On Tue 6 Jan 2009 07:05, Robin Getz suggested:
> >> adding a readonly, and writeonly, and ensuring that when you call
> >> debugfs_create_*, the mode is checked, and the "correct" fops are set
> >> doesn't seem like it would be a bad idea? This would enforce the
> >> kernel programmer's view on the world, and not allow pesky root users
> >> to override things....
> >>
> >> Greg - would you take something like that?
> >
> > How about this?
> >
> > Feel free to nak it - we can do the same thing where we are calling the
> > debugfs_create_* functions - this just makes it cleaner in my opinion.
> >
> > ---
> >
> > In many SOC implementations there are hardware registers can be read only,
> > or write only. This extends the debugfs to enforce the file permissions for
> > these types of registers, by providing a set of fops which are read only
> > or write only. This assumes that the kernel developer knows more about the
> > hardware than the user (even root users) - which is normally true.
> 
> we want it for cpu registers, but i dont see any reason why this
> wouldnt also apply to external devices attached via memory interfaces
> ... fifos and such ...

Yes - Although the existing use case is for SOC on chip registers - it applies
to anything where the kernel developer really doesn't want to allow the 
user to do an access that will cause a negative side effect (crash, effect
fifos, etc).

-Robin

Re: debugfs & vfs file permission issue?

[Greg KH]

On Tue, Jan 06, 2009 at 10:12:38AM -0500, Robin Getz wrote:
> On Tue 6 Jan 2009 07:05, Robin Getz suggested:
> >
> > adding a readonly, and writeonly, and ensuring that when you call 
> > debugfs_create_*, the mode is checked, and the "correct" fops are set 
> > doesn't seem like it would be a bad idea? This would enforce the
> > kernel programmer's view on the world, and not allow pesky root users
> > to override things....
> > 
> > Greg - would you take something like that?
> 
> How about this?
> 
> Feel free to nak it - we can do the same thing where we are calling the
> debugfs_create_* functions - this just makes it cleaner in my opinion.

I like it, want me to queue it up?

thanks,

greg k-h

Re: debugfs & vfs file permission issue?

[Robin Getz]

On Tue 6 Jan 2009 18:11, Greg KH pondered:
> On Tue, Jan 06, 2009 at 10:12:38AM -0500, Robin Getz wrote:
> > On Tue 6 Jan 2009 07:05, Robin Getz suggested:
> > >
> > > adding a readonly, and writeonly, and ensuring that when you call 
> > > debugfs_create_*, the mode is checked, and the "correct" fops are
> > > set doesn't seem like it would be a bad idea? This would enforce the
> > > kernel programmer's view on the world, and not allow pesky root
> > > users to override things....
> > > 
> > > Greg - would you take something like that?
> > 
> > How about this?
> 
> I like it, want me to queue it up?

Please & thanks.

-Robin

Re: debugfs & vfs file permission issue?

[Greg KH]

On Tue, Jan 06, 2009 at 10:12:38AM -0500, Robin Getz wrote:
> On Tue 6 Jan 2009 07:05, Robin Getz suggested:
> >
> > adding a readonly, and writeonly, and ensuring that when you call 
> > debugfs_create_*, the mode is checked, and the "correct" fops are set 
> > doesn't seem like it would be a bad idea? This would enforce the
> > kernel programmer's view on the world, and not allow pesky root users
> > to override things....
> > 
> > Greg - would you take something like that?
> 
> How about this?
> 
> Feel free to nak it - we can do the same thing where we are calling the
> debugfs_create_* functions - this just makes it cleaner in my opinion.

I like the patch, but there are no changes to the debugfs.h file, which
I think you need.

Care to resend it with the needed header file changes so that I can
apply it?

thanks,

greg k-h

[PATCH] debugfs: use specified mode to possibly mark files read/write only

[Mike Frysinger]

From: Robin Getz <rgetz@blackfin.uclinux.org>

In many SoC implementations there are hardware registers can be read or
write only.  This extends the debugfs to enforce the file permissions for
these types of registers by providing a set of fops which are read or
write only.  This assumes that the kernel developer knows more about the
hardware than the user (even root users) -- which is normally true.

Signed-off-by: Robin Getz <rgetz@blackfin.uclinux.org>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
you mention debugfs header file changes, but i don't see what would need
changing ?  the mode checking is all done internally ...

 fs/debugfs/file.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 33a9012..f10091f 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -67,6 +67,8 @@ static int debugfs_u8_get(void *data, u64 *val)
 	return 0;
 }
 DEFINE_SIMPLE_ATTRIBUTE(fops_u8, debugfs_u8_get, debugfs_u8_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u8_ro, debugfs_u8_get, NULL, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u8_wo, NULL, debugfs_u8_set, "%llu\n");
 
 /**
  * debugfs_create_u8 - create a debugfs file that is used to read and write an unsigned 8-bit value
@@ -95,6 +97,13 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_u8, debugfs_u8_get, debugfs_u8_set, "%llu\n");
 struct dentry *debugfs_create_u8(const char *name, mode_t mode,
 				 struct dentry *parent, u8 *value)
 {
+	/* if there are no write bits set, make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u8_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u8_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_u8);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_u8);
@@ -110,6 +119,8 @@ static int debugfs_u16_get(void *data, u64 *val)
 	return 0;
 }
 DEFINE_SIMPLE_ATTRIBUTE(fops_u16, debugfs_u16_get, debugfs_u16_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u16_ro, debugfs_u16_get, NULL, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u16_wo, NULL, debugfs_u16_set, "%llu\n");
 
 /**
  * debugfs_create_u16 - create a debugfs file that is used to read and write an unsigned 16-bit value
@@ -138,6 +149,13 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_u16, debugfs_u16_get, debugfs_u16_set, "%llu\n");
 struct dentry *debugfs_create_u16(const char *name, mode_t mode,
 				  struct dentry *parent, u16 *value)
 {
+	/* if there are no write bits set, make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u16_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u16_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_u16);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_u16);
@@ -153,6 +171,8 @@ static int debugfs_u32_get(void *data, u64 *val)
 	return 0;
 }
 DEFINE_SIMPLE_ATTRIBUTE(fops_u32, debugfs_u32_get, debugfs_u32_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u32_ro, debugfs_u32_get, NULL, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u32_wo, NULL, debugfs_u32_set, "%llu\n");
 
 /**
  * debugfs_create_u32 - create a debugfs file that is used to read and write an unsigned 32-bit value
@@ -181,6 +201,13 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_u32, debugfs_u32_get, debugfs_u32_set, "%llu\n");
 struct dentry *debugfs_create_u32(const char *name, mode_t mode,
 				 struct dentry *parent, u32 *value)
 {
+	/* if there are no write bits set, make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u32_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u32_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_u32);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_u32);
@@ -197,6 +224,8 @@ static int debugfs_u64_get(void *data, u64 *val)
 	return 0;
 }
 DEFINE_SIMPLE_ATTRIBUTE(fops_u64, debugfs_u64_get, debugfs_u64_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u64_ro, debugfs_u64_get, NULL, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_u64_wo, NULL, debugfs_u64_set, "%llu\n");
 
 /**
  * debugfs_create_u64 - create a debugfs file that is used to read and write an unsigned 64-bit value
@@ -225,15 +254,28 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_u64, debugfs_u64_get, debugfs_u64_set, "%llu\n");
 struct dentry *debugfs_create_u64(const char *name, mode_t mode,
 				 struct dentry *parent, u64 *value)
 {
+	/* if there are no write bits set, make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u64_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_u64_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_u64);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_u64);
 
 DEFINE_SIMPLE_ATTRIBUTE(fops_x8, debugfs_u8_get, debugfs_u8_set, "0x%02llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x8_ro, debugfs_u8_get, NULL, "0x%02llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x8_wo, NULL, debugfs_u8_set, "0x%02llx\n");
 
 DEFINE_SIMPLE_ATTRIBUTE(fops_x16, debugfs_u16_get, debugfs_u16_set, "0x%04llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x16_ro, debugfs_u16_get, NULL, "0x%04llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x16_wo, NULL, debugfs_u16_set, "0x%04llx\n");
 
 DEFINE_SIMPLE_ATTRIBUTE(fops_x32, debugfs_u32_get, debugfs_u32_set, "0x%08llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x32_ro, debugfs_u32_get, NULL, "0x%08llx\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_x32_wo, NULL, debugfs_u32_set, "0x%08llx\n");
 
 /*
  * debugfs_create_x{8,16,32} - create a debugfs file that is used to read and write an unsigned {8,16,32}-bit value
@@ -256,6 +298,13 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_x32, debugfs_u32_get, debugfs_u32_set, "0x%08llx\n"
 struct dentry *debugfs_create_x8(const char *name, mode_t mode,
 				 struct dentry *parent, u8 *value)
 {
+	/* if there are no write bits set, make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x8_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x8_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_x8);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_x8);
@@ -273,6 +322,13 @@ EXPORT_SYMBOL_GPL(debugfs_create_x8);
 struct dentry *debugfs_create_x16(const char *name, mode_t mode,
 				 struct dentry *parent, u16 *value)
 {
+	/* if there are no write bits set, make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x16_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x16_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_x16);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_x16);
@@ -290,6 +346,13 @@ EXPORT_SYMBOL_GPL(debugfs_create_x16);
 struct dentry *debugfs_create_x32(const char *name, mode_t mode,
 				 struct dentry *parent, u32 *value)
 {
+	/* if there are no write bits set, make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x32_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value, &fops_x32_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_x32);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_x32);
-- 
1.6.3.1

Re: [PATCH] debugfs: use specified mode to possibly mark files read/write only

[Greg KH]

On Tue, Jun 02, 2009 at 03:00:47AM -0400, Mike Frysinger wrote:
> From: Robin Getz <rgetz@blackfin.uclinux.org>
> 
> In many SoC implementations there are hardware registers can be read or
> write only.  This extends the debugfs to enforce the file permissions for
> these types of registers by providing a set of fops which are read or
> write only.  This assumes that the kernel developer knows more about the
> hardware than the user (even root users) -- which is normally true.
> 
> Signed-off-by: Robin Getz <rgetz@blackfin.uclinux.org>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> ---
> you mention debugfs header file changes, but i don't see what would need
> changing ?  the mode checking is all done internally ...

Looks good to me.