Messages in this thread Patch in this message | | | Subject | Re: [PATCH] afs: proc cells and rootcell are writeable | From | Joe Perches <> | Date | Tue, 28 Jan 2014 09:34:40 -0800 |
| |
On Tue, 2014-01-28 at 13:17 +0100, Geert Uytterhoeven wrote: > On Tue, Jan 28, 2014 at 1:04 PM, Ingo Molnar <mingo@kernel.org> wrote: > > * Geert Uytterhoeven <geert@linux-m68k.org> wrote: > >> On Sun, Jan 26, 2014 at 9:25 PM, Ingo Molnar <mingo@kernel.org> wrote: > >> > * Ingo Molnar <mingo@kernel.org> wrote: > >> >> * Linus Torvalds <torvalds@linux-foundation.org> wrote: > >> >> > On Sun, Jan 26, 2014 at 4:27 AM, David Howells <dhowells@redhat.com> wrote: > >> >> > > - p = proc_create("cells", 0, proc_afs, &afs_proc_cells_fops); > >> >> > > + p = proc_create("cells", S_IFREG | S_IRUGO | S_IWUSR, proc_afs, &afs_proc_cells_fops); > >> >> > > - p = proc_create("rootcell", 0, proc_afs, &afs_proc_rootcell_fops); > >> >> > > + p = proc_create("rootcell", S_IFREG | S_IRUGO | S_IWUSR, proc_afs, &afs_proc_rootcell_fops); > >> >> > > >> >> > So the S_IFREG isn't necessary. > >> >> > > >> >> > And quite frankly, I personally think S_IRUGO | S_IWUSR is _less_ > >> >> > readable than 0644. It's damn hard to parse those random letter > >> >> > combinations, and at least I have to really think about it, in a way > >> >> > that the octal representation does *not* make me go "I have to think > >> >> > about that". > >> >> > > >> >> > So my personal preference would be to just see that simple 0644 in > >> >> > proc_create. Hmm?
I don't have an issue with octal uses here either.
I think just as clear if not clearer than defines like
#define {whatever_}rwxrwxrwx 0777 #define {whatever_}rw_r__r__ 0644 #define {whatever_}r________ 0400
etc...
> >> Only a limited number of combinations is in active use, right? > > Correct - and in fact that kind of limitation is also a security > > feature: using patterns _outside_ of the typical, already defined > > group of permission patterns would in itself be a 'is that really > > justified?' red flag during review. > > Then Joe (CCed :-) can write a checkpatch rule to flag all new users of the > I_S[RWX}* flags..,
Flagging all "odd" uses of octal too?
Perhaps a checkpatch rule might look something like: --- scripts/checkpatch.pl | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 0ea2a1e..bf278e0 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -328,6 +328,12 @@ our $typeTypedefs = qr{(?x: atomic_t )}; +our $permissions = qr{(?x: + S_I[RWX](?:USR|GRP|OTH)| + S_IRWX[UGO]| + S_I(?:RWX|ALL|R|W|X)UGO +)}; + our $logFunctions = qr{(?x: printk(?:_ratelimited|_once|)| (?:[a-z0-9]+_){1,2}(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)| @@ -4457,6 +4463,15 @@ sub process { WARN("EXPORTED_WORLD_WRITABLE", "Exporting world writable files is usually an error. Consider more restrictive permissions.\n" . $herecurr); } + +# check for uses of permissions S_I<FOO> defines + if ($line =~ /\b($permissions)\b/) { + my $perm = $1; + my $msg_type = \&WARN; + $msg_type = \&CHK if ($file); + &{$msg_type}("PERMISSIONS", + "Use of $perm is not preferred.\n" . $herecurr); + } } # If we have no input at all, then there is nothing to report on
| |