From: Pali Rohár <[email protected]>
Both proc files are writeable and used for configuring cells. But
there is missing correct mode flag for writeable files. Without
this patch both proc files are read only.
Signed-off-by: Pali Rohár <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
fs/afs/proc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/afs/proc.c b/fs/afs/proc.c
index 526e4bbbde59..276cb6ed0b93 100644
--- a/fs/afs/proc.c
+++ b/fs/afs/proc.c
@@ -147,11 +147,11 @@ int afs_proc_init(void)
if (!proc_afs)
goto error_dir;
- 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);
if (!p)
goto error_cells;
- 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);
if (!p)
goto error_rootcell;
On Sun, Jan 26, 2014 at 4:27 AM, David Howells <[email protected]> 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?
Linus
* Linus Torvalds <[email protected]> wrote:
> On Sun, Jan 26, 2014 at 4:27 AM, David Howells <[email protected]> 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?
Perhaps we could also generate the most common variants as:
#define PERM__rw_r__r__ 0644
#define PERM__r________ 0400
#define PERM__r__r__r__ 0444
#define PERM__r_xr_xr_x 0555
etc.
or something similar, more or less matching the output of 'ls -l'?
That would also make security bugs in this area apparent at first
sight. The number of people who can recognize during review that
PERM_rw__w__w is probably unwise is probably two orders of magnitude
than those who can interpret octal 0622 at a glance.
Thanks,
Ingo
* Ingo Molnar <[email protected]> wrote:
> Perhaps we could also generate the most common variants as:
>
> #define PERM__rw_r__r__ 0644
> #define PERM__r________ 0400
> #define PERM__r__r__r__ 0444
> #define PERM__r_xr_xr_x 0555
>
> etc.
>
> or something similar, more or less matching the output of 'ls -l'?
>
> That would also make security bugs in this area apparent at first
> sight. The number of people who can recognize during review that
> PERM_rw__w__w is probably unwise is probably two orders of magnitude
> than those who can interpret octal 0622 at a glance.
^--higher
Thanks,
Ingo
* Ingo Molnar <[email protected]> wrote:
>
> * Linus Torvalds <[email protected]> wrote:
>
> > On Sun, Jan 26, 2014 at 4:27 AM, David Howells <[email protected]> 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?
>
> Perhaps we could also generate the most common variants as:
>
> #define PERM__rw_r__r__ 0644
> #define PERM__r________ 0400
> #define PERM__r__r__r__ 0444
> #define PERM__r_xr_xr_x 0555
>
> etc.
>
> or something similar, more or less matching the output of 'ls -l'?
Another variant of this would be to do the following macro:
PERM(R_X, R_X, R_X)
PERM(R__, R__, R__)
PERM(RW_, R__, R__)
With the advantage of separating the groups better and reducing the
number of constants needed.
Thanks,
Ingo
Ingo wrote:
> Perhaps we could also generate the most common variants as:
>
> #define PERM__rw_r__r__ 0644
You're not alone!
http://lkml.indiana.edu/hypermail/linux/kernel/0607.3/1325.html
But I think 0644 is obvious and the most right way.
Of course, proc should detect those (->write vs ->mode) and complain.
Something I never sent.
On Sun, Jan 26, 2014 at 9:25 PM, Ingo Molnar <[email protected]> wrote:
> * Ingo Molnar <[email protected]> wrote:
>> * Linus Torvalds <[email protected]> wrote:
>> > On Sun, Jan 26, 2014 at 4:27 AM, David Howells <[email protected]> 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?
>>
>> Perhaps we could also generate the most common variants as:
>>
>> #define PERM__rw_r__r__ 0644
>> #define PERM__r________ 0400
>> #define PERM__r__r__r__ 0444
>> #define PERM__r_xr_xr_x 0555
I like it (also without the PERM prefix, cfr. Alexey's old patch).
>> or something similar, more or less matching the output of 'ls -l'?
>
> Another variant of this would be to do the following macro:
>
> PERM(R_X, R_X, R_X)
> PERM(R__, R__, R__)
> PERM(RW_, R__, R__)
IMHO, this is again less outstanding.
> With the advantage of separating the groups better and reducing the
> number of constants needed.
Only a limited number of combinations is in active use, right?
It should be difficult to create e.g. world-writable files that are
not accessable
by the owner.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
* Geert Uytterhoeven <[email protected]> wrote:
> On Sun, Jan 26, 2014 at 9:25 PM, Ingo Molnar <[email protected]> wrote:
> > * Ingo Molnar <[email protected]> wrote:
> >> * Linus Torvalds <[email protected]> wrote:
> >> > On Sun, Jan 26, 2014 at 4:27 AM, David Howells <[email protected]> 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?
> >>
> >> Perhaps we could also generate the most common variants as:
> >>
> >> #define PERM__rw_r__r__ 0644
> >> #define PERM__r________ 0400
> >> #define PERM__r__r__r__ 0444
> >> #define PERM__r_xr_xr_x 0555
>
> I like it (also without the PERM prefix, cfr. Alexey's old patch).
>
> >> or something similar, more or less matching the output of 'ls -l'?
> >
> > Another variant of this would be to do the following macro:
> >
> > PERM(R_X, R_X, R_X)
> > PERM(R__, R__, R__)
> > PERM(RW_, R__, R__)
>
> IMHO, this is again less outstanding.
>
> > With the advantage of separating the groups better and reducing the
> > number of constants needed.
>
> 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.
I'm fine with Alexey's shorter variant as well.
Would someone be interested in sending a real patch for it, defining a
usable set of initial flags such as 0644, 0444, 0555 and 0600?
comet:~/tip> for N in $(git grep -E '\.\<mode\>.*=.*0' arch/x86/ kernel/ | cut -d: -f2-); do echo $N; done | sort | grep ^0[0-7] | cut -c1-4 | uniq -c | sort -n
1 0200
1 0666
5 0600
15 0555
16 0444
148 0644
I'd definitely convert most of kernel/ and arch/x86/ to use them.
Thanks,
Ingo
On Tue, Jan 28, 2014 at 1:04 PM, Ingo Molnar <[email protected]> wrote:
> * Geert Uytterhoeven <[email protected]> wrote:
>> On Sun, Jan 26, 2014 at 9:25 PM, Ingo Molnar <[email protected]> wrote:
>> > * Ingo Molnar <[email protected]> wrote:
>> >> * Linus Torvalds <[email protected]> wrote:
>> >> > On Sun, Jan 26, 2014 at 4:27 AM, David Howells <[email protected]> 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?
>> >>
>> >> Perhaps we could also generate the most common variants as:
>> >>
>> >> #define PERM__rw_r__r__ 0644
>> >> #define PERM__r________ 0400
>> >> #define PERM__r__r__r__ 0444
>> >> #define PERM__r_xr_xr_x 0555
>>
>> I like it (also without the PERM prefix, cfr. Alexey's old patch).
>>
>> >> or something similar, more or less matching the output of 'ls -l'?
>> >
>> > Another variant of this would be to do the following macro:
>> >
>> > PERM(R_X, R_X, R_X)
>> > PERM(R__, R__, R__)
>> > PERM(RW_, R__, R__)
>>
>> IMHO, this is again less outstanding.
>>
>> > With the advantage of separating the groups better and reducing the
>> > number of constants needed.
>>
>> 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..,
> I'm fine with Alexey's shorter variant as well.
>
> Would someone be interested in sending a real patch for it, defining a
> usable set of initial flags such as 0644, 0444, 0555 and 0600?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
* Geert Uytterhoeven <[email protected]> wrote:
> On Tue, Jan 28, 2014 at 1:04 PM, Ingo Molnar <[email protected]> wrote:
> > * Geert Uytterhoeven <[email protected]> wrote:
> >> On Sun, Jan 26, 2014 at 9:25 PM, Ingo Molnar <[email protected]> wrote:
> >> > * Ingo Molnar <[email protected]> wrote:
> >> >> * Linus Torvalds <[email protected]> wrote:
> >> >> > On Sun, Jan 26, 2014 at 4:27 AM, David Howells <[email protected]> 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?
> >> >>
> >> >> Perhaps we could also generate the most common variants as:
> >> >>
> >> >> #define PERM__rw_r__r__ 0644
> >> >> #define PERM__r________ 0400
> >> >> #define PERM__r__r__r__ 0444
> >> >> #define PERM__r_xr_xr_x 0555
> >>
> >> I like it (also without the PERM prefix, cfr. Alexey's old patch).
> >>
> >> >> or something similar, more or less matching the output of 'ls -l'?
> >> >
> >> > Another variant of this would be to do the following macro:
> >> >
> >> > PERM(R_X, R_X, R_X)
> >> > PERM(R__, R__, R__)
> >> > PERM(RW_, R__, R__)
> >>
> >> IMHO, this is again less outstanding.
> >>
> >> > With the advantage of separating the groups better and reducing the
> >> > number of constants needed.
> >>
> >> 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..,
I'd also flag raw octal use: they are easily mixed up and only a
relatively small group of people will read them as-is, most other
kernel/Linux people will only recognize anomalies in the rwxrwxrwx
notation.
So the number of reviewers who might spot bugs, either during patch
submission or later on reading the code increases.
Thanks,
Ingo
On Tue, 2014-01-28 at 13:17 +0100, Geert Uytterhoeven wrote:
> On Tue, Jan 28, 2014 at 1:04 PM, Ingo Molnar <[email protected]> wrote:
> > * Geert Uytterhoeven <[email protected]> wrote:
> >> On Sun, Jan 26, 2014 at 9:25 PM, Ingo Molnar <[email protected]> wrote:
> >> > * Ingo Molnar <[email protected]> wrote:
> >> >> * Linus Torvalds <[email protected]> wrote:
> >> >> > On Sun, Jan 26, 2014 at 4:27 AM, David Howells <[email protected]> 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
Linus Torvalds <[email protected]> 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.
True. Is it worth creating proc_create_special() that can create a non-regular
file and then making proc_create() only permit regular files (and complain if
the S_IFMT field is not zero)?
> 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".
Fine by me.
David
On Tue, Jan 28, 2014 at 08:20:12PM +0000, David Howells wrote:
> Linus Torvalds <[email protected]> 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.
>
> True. Is it worth creating proc_create_special() that can create a non-regular
> file and then making proc_create() only permit regular files (and complain if
> the S_IFMT field is not zero)?
We already do: in proc_create_data() we have
struct proc_dir_entry *pde;
if ((mode & S_IFMT) == 0)
mode |= S_IFREG;
if (!S_ISREG(mode)) {
WARN_ON(1); /* use proc_mkdir() */
return NULL;
}
proc_mkdir{,_data,_mode} are there for purpose. Nobody had been insane
enough to put FIFOs or sockets in procfs and anything else would need
additional data anyway. proc_symlink() is there, proc_mknod() isn't and
nobody has complained yet. Let's keep it that way, plese...
Al Viro <[email protected]> wrote:
> > ... and then making proc_create() only permit regular files (and complain
> > if the S_IFMT field is not zero)?
>
> We already do: in proc_create_data() we have
> struct proc_dir_entry *pde;
> if ((mode & S_IFMT) == 0)
> mode |= S_IFREG;
>
> if (!S_ISREG(mode)) {
> WARN_ON(1); /* use proc_mkdir() */
> return NULL;
> }
>
> proc_mkdir{,_data,_mode} are there for purpose. Nobody had been insane
> enough to put FIFOs or sockets in procfs and anything else would need
> additional data anyway. proc_symlink() is there, proc_mknod() isn't and
> nobody has complained yet. Let's keep it that way, plese...
Should we then change the proc_create_data() to do:
struct proc_dir_entry *pde;
if (mode & S_IFMT) {
WARN_ON(1); /* use proc_mkdir() */
return NULL;
}
mode |= S_IFREG;
and stop passing S_IFREG into it?
David
David Howells <[email protected]> writes:
> From: Pali Rohár <[email protected]>
>
> Both proc files are writeable and used for configuring cells. But
> there is missing correct mode flag for writeable files. Without
> this patch both proc files are read only.
Dumb question. Is this worth fixing? Should we perhaps instead remove
the write methods?
These files have been read-only since this code was merged in 2002.
Over a decade of not being used seems like a strong indication that no
one cares about the write path.
Eric
> Signed-off-by: Pali Rohár <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> ---
>
> fs/afs/proc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/afs/proc.c b/fs/afs/proc.c
> index 526e4bbbde59..276cb6ed0b93 100644
> --- a/fs/afs/proc.c
> +++ b/fs/afs/proc.c
> @@ -147,11 +147,11 @@ int afs_proc_init(void)
> if (!proc_afs)
> goto error_dir;
>
> - 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);
> if (!p)
> goto error_cells;
>
> - 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);
> if (!p)
> goto error_rootcell;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Thu, Jan 30, 2014 at 1:48 PM, Eric W. Biederman
<[email protected]> wrote:
>
> These files have been read-only since this code was merged in 2002.
> Over a decade of not being used seems like a strong indication that no
> one cares about the write path.
I think this is a pretty strong argument. Counter-arguments, anybody?
Linus
2014-01-30 Linus Torvalds <[email protected]>:
> On Thu, Jan 30, 2014 at 1:48 PM, Eric W. Biederman
> <[email protected]> wrote:
>>
>> These files have been read-only since this code was merged in 2002.
>> Over a decade of not being used seems like a strong indication that no
>> one cares about the write path.
>
> I think this is a pretty strong argument. Counter-arguments, anybody?
>
> Linus
Hi!
In afs documentation is written that you need to write to these files. See:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/filesystems/afs.txt#n82
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/filesystems/afs.txt#n159
Without cells file, you cannot specify other cell servers and you can
use only one rootcell which was specified in kernel cmdline. So for
mounting other server, you need to reboot kernel (if you compiled afs
driver statically) and without cells file there is no other option to
mount more afs servers... (or at least it is not written in that
documentation). So I think without write access it is hard or maybe
impossible to use afs driver.
But maybe, afs maintainers or other afs developers should write more
info about it (or update documentation if is old).
--
Pali Rohár
[email protected]
On Thu, Jan 30, 2014 at 2:15 PM, Pali Rohár <[email protected]> wrote:
>
> In afs documentation is written that you need to write to these files. See:
Well, but the afs documentation is clearly wrong, since the
"documented" procedure doesn't actually *work*.
So I don't think "it's documented" is a very strong argument.
Documentation is as useful as used toilet paper, if clearly nobody has
ever done what was "documented".
Linus
On Thu, Jan 30, 2014 at 02:27:15PM -0800, Linus Torvalds wrote:
> On Thu, Jan 30, 2014 at 2:15 PM, Pali Roh?r <[email protected]> wrote:
> >
> > In afs documentation is written that you need to write to these files. See:
>
> Well, but the afs documentation is clearly wrong, since the
> "documented" procedure doesn't actually *work*.
>
> So I don't think "it's documented" is a very strong argument.
> Documentation is as useful as used toilet paper, if clearly nobody has
> ever done what was "documented".
Don't most (all?) of the people actually using AFS use some out-of-tree thing instead ?
Dave
Pali Rohár <[email protected]> writes:
> 2014-01-30 Linus Torvalds <[email protected]>:
>> Eric W. Biederman <[email protected]> wrote:
>>> These files have been read-only since this code was merged in 2002.
>>> Over a decade of not being used seems like a strong indication that no
>>> one cares about the write path.
>> I think this is a pretty strong argument. Counter-arguments, anybody?
The current in-tree AFS module is still something of an experiment and is
not widely used by actual clients, essentially all of which are still
using the (old, ugly, frustratingly-difficult-to-maintain) out-of-tree
module. This is mostly because the in-kernel module is not yet
sufficiently mature to support a variety of use cases. I think this is a
(minor) step towards making it more mature.
> In afs documentation is written that you need to write to these files. See:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/filesystems/afs.txt#n82
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/filesystems/afs.txt#n159
> Without cells file, you cannot specify other cell servers and you can
> use only one rootcell which was specified in kernel cmdline. So for
> mounting other server, you need to reboot kernel (if you compiled afs
> driver statically) and without cells file there is no other option to
> mount more afs servers... (or at least it is not written in that
> documentation). So I think without write access it is hard or maybe
> impossible to use afs driver.
In the AFS world more generally, it is not common to change the root cell
without restarting the client. It *is*, however, very common to add
configuration for new cells on the fly. The most common implementation,
OpenAFS, has a command-line tool for root to do that (fs newcell). The
equivalent for the in-tree AFS module would be writing to this file, so to
support the fs newcell command with the in-tree module, this file would
need to be writable. This is a common action in some use cases.
By comparison, there is not a standard fs command to set the current local
cell, only to retrieve it. However, I suspect that's primarily due to
design limitations in the OpenAFS client. If it's not difficult to
support this operation in the in-tree kernel module, I think it would be a
good idea to do so early, since it's the kind of thing that could be
difficult to retroactively add later.
--
Russ Allbery ([email protected]) <http://www.eyrie.org/~eagle/>
Eric W. Biederman <[email protected]> wrote:
> These files have been read-only since this code was merged in 2002.
> Over a decade of not being used seems like a strong indication that no
> one cares about the write path.
Actually, things aren't as simple as they seem. Without the patch applied:
[root@andromeda ~]# ls -l /proc/fs/afs/cells
-r--r--r--. 1 root root 0 Jan 31 00:04 /proc/fs/afs/cells
[root@andromeda ~]# echo add your-file-system.com 204.29.154.37 >/proc/fs/afs/cells
[root@andromeda ~]#
You'll observe there is no error reported on the echo command.
Further, looking in dmesg, I see:
kAFS: Added new cell 'your-file-system.com'
So the file *is* writable, *despite* i_mode.
David
Further:
[root@andromeda ~]# touch /tmp/foo
[root@andromeda ~]# chmod 0444 /tmp/foo
[root@andromeda ~]# ls -l /tmp/foo
-r--r--r--. 1 root root 0 Jan 31 00:17 /tmp/foo
[root@andromeda ~]# echo hello >/tmp/foo
[root@andromeda ~]# ls -l /tmp/foo
-r--r--r--. 1 root root 6 Jan 31 00:17 /tmp/foo
[root@andromeda ~]#
But:
[root@andromeda ~]# su - dhowells
[dhowells@andromeda ~]$ touch /tmp/bar
[dhowells@andromeda ~]$ chmod 0444 /tmp/bar
[dhowells@andromeda ~]$ ls -l /tmp/bar
-r--r--r--. 1 dhowells dhowells 0 Jan 31 00:19 /tmp/bar
[dhowells@andromeda ~]$ echo hello >/tmp/bar
-bash: /tmp/bar: Permission denied
David
Linus Torvalds <[email protected]> wrote:
> I think this is a pretty strong argument. Counter-arguments, anybody?
Yes. CAP_DAC_READ_SEARCH.
David
David Howells <[email protected]> wrote:
> > I think this is a pretty strong argument. Counter-arguments, anybody?
>
> Yes. CAP_DAC_READ_SEARCH.
No, it would seem unlikely it's that, but I guess there's another capability
override because the process is owned by root.
David
David Howells <[email protected]> wrote:
> > > I think this is a pretty strong argument. Counter-arguments, anybody?
> >
> > Yes. CAP_DAC_READ_SEARCH.
>
> No, it would seem unlikely it's that, but I guess there's another capability
> override because the process is owned by root.
CAP_DAC_OVERRIDE, I think.
int generic_permission(struct inode *inode, int mask)
{
...
/*
* Read/write DACs are always overridable.
* Executable DACs are overridable when there is
* at least one exec bit set.
*/
if (!(mask & MAY_EXEC) || (inode->i_mode & S_IXUGO))
if (inode_capable(inode, CAP_DAC_OVERRIDE))
return 0;
...
}
David