LinuxLists.cc - debugfs & vfs file permission issue?

[permalink] [raw]

Subject: Re: debugfs & vfs file permission issue?

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

2009-01-06 05:55:14

[permalink] [raw]

Subject: Re: debugfs & vfs file permission issue?

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

2009-01-06 06:21:25

[permalink] [raw]

Subject: Re: debugfs & vfs file permission issue?

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

2009-01-06 06:32:41

[permalink] [raw]

Subject: Re: debugfs & vfs file permission issue?

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

2009-01-06 12:06:32

[permalink] [raw]

Subject: Re: debugfs & vfs file permission issue?

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

2009-01-06 15:13:42

[permalink] [raw]

Subject: Re: debugfs & vfs file permission issue?

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 <[email protected]>

---

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);

2009-01-06 15:21:13

[permalink] [raw]

Subject: Re: debugfs & vfs file permission issue?

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

2009-01-06 21:21:00

[permalink] [raw]

Subject: Re: debugfs & vfs file permission issue?

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

2009-01-06 23:13:20

[permalink] [raw]

Subject: Re: debugfs & vfs file permission issue?

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

2009-01-07 03:31:08

[permalink] [raw]

Subject: Re: debugfs & vfs file permission issue?

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

2009-01-25 21:35:45

[permalink] [raw]

Subject: Re: debugfs & vfs file permission issue?

2009-06-02 07:01:01

[permalink] [raw]

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

From: Robin Getz <[email protected]>

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 <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
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

2009-06-02 23:30:14