[PATCH] afs: proc cells and rootcell are writeable

[PATCH] afs: proc cells and rootcell are writeable

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 817 bytes --]

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.

diff --git a/fs/afs/proc.c b/fs/afs/proc.c
index 526e4bb..276cb6e 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;
 
-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] afs: proc cells and rootcell are writeable

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 954 bytes --]

On Wednesday 20 November 2013 14:30:55 Pali Rohár wrote:
> 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.
> 
> diff --git a/fs/afs/proc.c b/fs/afs/proc.c
> index 526e4bb..276cb6e 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;

Hello, can somebody review my patch?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] afs: proc cells and rootcell are writeable

[Andrew Morton]

On Wed, 20 Nov 2013 14:30:55 +0100 Pali Roh__r <pali.rohar@gmail.com> wrote:

> 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.
> 
> diff --git a/fs/afs/proc.c b/fs/afs/proc.c
> index 526e4bb..276cb6e 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;

Please send a signed-off-by: for this, as per
Documentation/SubmittingPatches, section 12.

David ack?

Re: [PATCH] afs: proc cells and rootcell are writeable

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1320 bytes --]

On Monday 16 December 2013 08:00:04 Andrew Morton wrote:
> On Wed, 20 Nov 2013 14:30:55 +0100 Pali Roh__r 
<pali.rohar@gmail.com> wrote:
> > 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.
> > 
> > diff --git a/fs/afs/proc.c b/fs/afs/proc.c
> > index 526e4bb..276cb6e 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;
> 
> Please send a signed-off-by: for this, as per
> Documentation/SubmittingPatches, section 12.
> 
> David ack?

Sorry, I forgot to add Signed-off-by. Here it is:

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] afs: proc cells and rootcell are writeable

[David Howells]

Andrew Morton <akpm@linux-foundation.org> wrote:

> David ack?

I've signed it off and added here:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/commit/?h=afs&id=8de69dbba9012693d4f9e7a7e3c12a0b467f85f3

David

Re: [PATCH] afs: proc cells and rootcell are writeable

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 638 bytes --]

On Tuesday 17 December 2013 19:31:05 David Howells wrote:
> Andrew Morton <akpm@linux-foundation.org> wrote:
> > David ack?
> 
> I've signed it off and added here:
> 
> 	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs
> .git/commit/?h=afs&id=8de69dbba9012693d4f9e7a7e3c12a0b467f85f3
> 
> David

Can you propose this patch also for stable branches? Without this 
patch it is not possible to write to cells and rootcell files 
which means that it is not possible to add new servers to cells, 
so it is not possible to access afs disks and afs driver is 
unusable...

-- 
Pali Rohár
pali.rohar@gmail.com


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH] afs: proc cells and rootcell are writeable

[David Howells]

From: Pali Rohár <pali.rohar@gmail.com>

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 <pali.rohar@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 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;
 

Re: [PATCH] afs: proc cells and rootcell are writeable

[Linus Torvalds]

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?

                Linus

Re: [PATCH] afs: proc cells and rootcell are writeable

[Ingo Molnar]

* 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?

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

Re: [PATCH] afs: proc cells and rootcell are writeable

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> 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

Re: [PATCH] afs: proc cells and rootcell are writeable

[Ingo Molnar]

* 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?
> 
> 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

Re: [PATCH] afs: proc cells and rootcell are writeable

[Alexey Dobriyan]

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.

Re: [PATCH] afs: proc cells and rootcell are writeable

[Geert Uytterhoeven]

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?
>>
>> 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 -- geert@linux-m68k.org

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

Re: [PATCH] afs: proc cells and rootcell are writeable

[Ingo Molnar]

* 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?
> >>
> >> 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

Re: [PATCH] afs: proc cells and rootcell are writeable

[Geert Uytterhoeven]

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?
>> >>
>> >> 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 -- geert@linux-m68k.org

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

Re: [PATCH] afs: proc cells and rootcell are writeable

[Ingo Molnar]

* Geert Uytterhoeven <geert@linux-m68k.org> 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?
> >> >>
> >> >> 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

Re: [PATCH] afs: proc cells and rootcell are writeable

[Joe Perches]

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

Re: [PATCH] afs: proc cells and rootcell are writeable

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> 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

Re: [PATCH] afs: proc cells and rootcell are writeable

[Al Viro]

On Tue, Jan 28, 2014 at 08:20:12PM +0000, David Howells wrote:
> Linus Torvalds <torvalds@linux-foundation.org> 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...

Re: [PATCH] afs: proc cells and rootcell are writeable

[David Howells]

Al Viro <viro@ZenIV.linux.org.uk> 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

Re: [PATCH] afs: proc cells and rootcell are writeable

[Eric W. Biederman]

David Howells <dhowells@redhat.com> writes:

> From: Pali Rohár <pali.rohar@gmail.com>
>
> 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 <pali.rohar@gmail.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] afs: proc cells and rootcell are writeable

[Linus Torvalds]

On Thu, Jan 30, 2014 at 1:48 PM, Eric W. Biederman
<ebiederm@xmission.com> 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

Re: [PATCH] afs: proc cells and rootcell are writeable

[Pali Rohár]

2014-01-30 Linus Torvalds <torvalds@linux-foundation.org>:
> On Thu, Jan 30, 2014 at 1:48 PM, Eric W. Biederman
> <ebiederm@xmission.com> 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
pali.rohar@gmail.com

Re: [PATCH] afs: proc cells and rootcell are writeable

[Linus Torvalds]

On Thu, Jan 30, 2014 at 2:15 PM, Pali Rohár <pali.rohar@gmail.com> 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

Re: [PATCH] afs: proc cells and rootcell are writeable

[Russ Allbery]

Pali Rohár <pali.rohar@gmail.com> writes:
> 2014-01-30 Linus Torvalds <torvalds@linux-foundation.org>:
>> Eric W. Biederman <ebiederm@xmission.com> 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 (eagle@eyrie.org)              <http://www.eyrie.org/~eagle/>

Re: [PATCH] afs: proc cells and rootcell are writeable

[Dave Jones]

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 <pali.rohar@gmail.com> 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

Re: [PATCH] afs: proc cells and rootcell are writeable

[David Howells]

Eric W. Biederman <ebiederm@xmission.com> 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

Re: [PATCH] afs: proc cells and rootcell are writeable

[David Howells]

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

Re: [PATCH] afs: proc cells and rootcell are writeable

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> I think this is a pretty strong argument. Counter-arguments, anybody?

Yes.  CAP_DAC_READ_SEARCH.

David

Re: [PATCH] afs: proc cells and rootcell are writeable

[David Howells]

David Howells <dhowells@redhat.com> 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

Re: [PATCH] afs: proc cells and rootcell are writeable

[David Howells]

David Howells <dhowells@redhat.com> 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