[PATCH] VFS: Sanity check mount flags passed to change_mnt_propagation()

[PATCH] VFS: Sanity check mount flags passed to change_mnt_propagation()

[Valerie Aurora]

do_change_type() is buggy when passed multiple MS_* flags.  Discovered
because mount(8) incorrectly adds MS_RDONLY flag to
shared/slave/private/unbindable mounts.  Karel Zak will fix the
mount(8) bug shortly.

A test program is attached.  Against Viro's #untested branch.

-VAL

commit 208ca52f69ea53cf0723b8492fe54ebf9a3bf36a
Author: Valerie Aurora <vaurora@redhat.com>
Date:   Thu Aug 26 11:07:22 2010 -0700

    VFS: Sanity check mount flags passed to change_mnt_propagation()
    
    Sanity check the flags passed to change_mnt_propagation().  Exactly
    one flag should be set.  Return EINVAL otherwise.
    
    Userspace can pass in arbitrary combinations of MS_* flags to mount().
    do_change_type() is called if any of MS_SHARED, MS_PRIVATE, MS_SLAVE,
    or MS_UNBINDABLE is set.  do_change_type() clears MS_REC and then
    calls change_mnt_propagation() with the rest of the user-supplied
    flags.  change_mnt_propagation() clearly assumes only one flag is set
    but do_change_type() does not check that this is true.  For example,
    mount() with flags MS_SHARED | MS_RDONLY does not actually make the
    mount shared or read-only but does clear MNT_UNBINDABLE.
    
    Signed-off-by: Valerie Aurora <vaurora@redhat.com>

diff --git a/fs/namespace.c b/fs/namespace.c
index de402eb..4987c4c 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1484,13 +1484,32 @@ out_unlock:
 }
 
 /*
+ * Sanity check the flags to change_mnt_propagation.
+ */
+
+static int flags_to_propagation_type(int flags) {
+	int type = flags & ~MS_REC;
+
+	/* Fail if any non-propagation flags are set */
+	if (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
+		return 0;
+	/* Only one propagation flag should be set */
+	if (((type & (MS_SHARED)) && (type & ~MS_SHARED)) ||
+	    ((type & (MS_PRIVATE)) && (type & ~MS_PRIVATE)) ||
+	    ((type & (MS_SLAVE)) && (type & ~MS_SLAVE)) ||
+	    ((type & (MS_UNBINDABLE)) && (type & ~MS_UNBINDABLE)))
+		return 0;
+	return type;
+}
+
+/*
  * recursively change the type of the mountpoint.
  */
 static int do_change_type(struct path *path, int flag)
 {
 	struct vfsmount *m, *mnt = path->mnt;
 	int recurse = flag & MS_REC;
-	int type = flag & ~MS_REC;
+	int type;
 	int err = 0;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -1499,6 +1518,10 @@ static int do_change_type(struct path *path, int flag)
 	if (path->dentry != path->mnt->mnt_root)
 		return -EINVAL;
 
+	type = flags_to_propagation_type(flag);
+	if (!type)
+		return -EINVAL;
+
 	down_write(&namespace_sem);
 	if (type == MS_SHARED) {
 		err = invent_group_ids(mnt, recurse);

Re: [PATCH] VFS: Sanity check mount flags passed to change_mnt_propagation()

[Matthew Wilcox]

On Thu, Aug 26, 2010 at 04:03:18PM -0400, Valerie Aurora wrote:
> +	/* Fail if any non-propagation flags are set */
> +	if (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
> +		return 0;
> +	/* Only one propagation flag should be set */
> +	if (((type & (MS_SHARED)) && (type & ~MS_SHARED)) ||
> +	    ((type & (MS_PRIVATE)) && (type & ~MS_PRIVATE)) ||
> +	    ((type & (MS_SLAVE)) && (type & ~MS_SLAVE)) ||
> +	    ((type & (MS_UNBINDABLE)) && (type & ~MS_UNBINDABLE)))
> +		return 0;

Hrm.  I think we can do this a bit more pithily.

	/* Only one propagation flag should be set, and no others */
	if (hweight32(type) != 1 &&
	    (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
		return 0;

Too clever?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] VFS: Sanity check mount flags passed to change_mnt_propagation()

[Valerie Aurora]

On Thu, Aug 26, 2010 at 07:14:36PM -0600, Matthew Wilcox wrote:
> On Thu, Aug 26, 2010 at 04:03:18PM -0400, Valerie Aurora wrote:
> > +	/* Fail if any non-propagation flags are set */
> > +	if (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
> > +		return 0;
> > +	/* Only one propagation flag should be set */
> > +	if (((type & (MS_SHARED)) && (type & ~MS_SHARED)) ||
> > +	    ((type & (MS_PRIVATE)) && (type & ~MS_PRIVATE)) ||
> > +	    ((type & (MS_SLAVE)) && (type & ~MS_SLAVE)) ||
> > +	    ((type & (MS_UNBINDABLE)) && (type & ~MS_UNBINDABLE)))
> > +		return 0;
> 
> Hrm.  I think we can do this a bit more pithily.
> 
> 	/* Only one propagation flag should be set, and no others */
> 	if (hweight32(type) != 1 &&
> 	    (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
> 		return 0;
> 
> Too clever?

I was hoping someone would go find the best bitop for me, thanks. :)
hweight32() is an awkward name but the comment makes it clear.  I'm
happy with either.

Thanks for the help,

-VAL

Re: [PATCH] VFS: Sanity check mount flags passed to change_mnt_propagation()

[Bob Copeland]

On Fri, Aug 27, 2010 at 1:43 PM, Valerie Aurora <vaurora@redhat.com> wrote:
> On Thu, Aug 26, 2010 at 07:14:36PM -0600, Matthew Wilcox wrote:
>> Hrm.  I think we can do this a bit more pithily.
>>
>>       /* Only one propagation flag should be set, and no others */
>>       if (hweight32(type) != 1 &&
>>           (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
>>               return 0;
>>
>> Too clever?
>
> I was hoping someone would go find the best bitop for me, thanks. :)
> hweight32() is an awkward name but the comment makes it clear.  I'm
> happy with either.
>
> Thanks for the help,

Didn't read surrounding code, but is that supposed to be '||'?

Otherwise the case where only a single non-propagation flag is
set no longer returns 0...

-- 
Bob Copeland %% www.bobcopeland.com
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] VFS: Sanity check mount flags passed to change_mnt_propagation()

[Valerie Aurora]

On Fri, Aug 27, 2010 at 01:51:06PM -0400, Bob Copeland wrote:
> On Fri, Aug 27, 2010 at 1:43 PM, Valerie Aurora <vaurora@redhat.com> wrote:
> > On Thu, Aug 26, 2010 at 07:14:36PM -0600, Matthew Wilcox wrote:
> >> Hrm. ?I think we can do this a bit more pithily.
> >>
> >> ? ? ? /* Only one propagation flag should be set, and no others */
> >> ? ? ? if (hweight32(type) != 1 &&
> >> ? ? ? ? ? (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
> >> ? ? ? ? ? ? ? return 0;
> >>
> >> Too clever?
> >
> > I was hoping someone would go find the best bitop for me, thanks. :)
> > hweight32() is an awkward name but the comment makes it clear. ?I'm
> > happy with either.
> >
> > Thanks for the help,
> 
> Didn't read surrounding code, but is that supposed to be '||'?
> 
> Otherwise the case where only a single non-propagation flag is
> set no longer returns 0...

Yes, thanks!

I'll run the test program again before resubmitting. :)

-VAL

Re: [PATCH] VFS: Sanity check mount flags passed to change_mnt_propagation()

[Matthew Wilcox]

On Fri, Aug 27, 2010 at 01:51:06PM -0400, Bob Copeland wrote:
> On Fri, Aug 27, 2010 at 1:43 PM, Valerie Aurora <vaurora@redhat.com> wrote:
> > On Thu, Aug 26, 2010 at 07:14:36PM -0600, Matthew Wilcox wrote:
> >> Hrm. ?I think we can do this a bit more pithily.
> >>
> >> ? ? ? /* Only one propagation flag should be set, and no others */
> >> ? ? ? if (hweight32(type) != 1 &&
> >> ? ? ? ? ? (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
> >> ? ? ? ? ? ? ? return 0;
> >>
> >> Too clever?
> >
> > I was hoping someone would go find the best bitop for me, thanks. :)
> > hweight32() is an awkward name but the comment makes it clear. ?I'm
> > happy with either.
> >
> > Thanks for the help,
> 
> Didn't read surrounding code, but is that supposed to be '||'?
> 
> Otherwise the case where only a single non-propagation flag is
> set no longer returns 0...

Val's original code returned 0 as failure.  So a single non-propagation
flag set shouldn't return 0.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] VFS: Sanity check mount flags passed to change_mnt_propagation()

[Bob Copeland]

On Sat, Aug 28, 2010 at 04:57:07AM -0600, Matthew Wilcox wrote:
> > Didn't read surrounding code, but is that supposed to be '||'?
> > 
> > Otherwise the case where only a single non-propagation flag is
> > set no longer returns 0...
> 
> Val's original code returned 0 as failure.  So a single non-propagation
> flag set shouldn't return 0.

Not to belabor the point -- but her original code did return zero in
that case (2nd assertion fails):

#include <linux/fs.h>
#include <stdio.h>
#include <assert.h>

#define hweight32 dumb_hweight32

int dumb_hweight32(int a)
{
	int i;
	int weight = 0;

	for (i=0; i < 32; i++)
		weight += !!(a & (1 << i));

	return weight;
}

static int flags_to_propagation_type(int flags) {
	int type = flags & ~MS_REC;

	/* Fail if any non-propagation flags are set */
 	if (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
		return 0;
	/* Only one propagation flag should be set */
	if (((type & (MS_SHARED)) && (type & ~MS_SHARED)) ||
	   ((type & (MS_PRIVATE)) && (type & ~MS_PRIVATE)) ||
	   ((type & (MS_SLAVE)) && (type & ~MS_SLAVE)) ||
	   ((type & (MS_UNBINDABLE)) && (type & ~MS_UNBINDABLE)))
	       return 0;
	return type;
}

static int flags_to_propagation_type_2(int flags) {
 	int type = flags & ~MS_REC;

	/* Only one propagation flag should be set, and no others */
	if (hweight32(type) != 1 &&
	    (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE)))
		return 0;

	return type;
}

int main(int argc, char *argv[])
{
	int mytype = MS_RDONLY;

	assert(flags_to_propagation_type(mytype) == 0);
	assert(flags_to_propagation_type_2(mytype) == 0);
}

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] VFS: Sanity check mount flags passed to change_mnt_propagation()

[Linus Torvalds]

On Thu, Aug 26, 2010 at 6:14 PM, Matthew Wilcox <matthew@wil.cx> wrote:
>
> Hrm.  I think we can do this a bit more pithily.

Well, perhaps, but please NOT the way you suggest.

>
>        /* Only one propagation flag should be set, and no others */
>        if (hweight32(type) != 1

Guys, stop with "teh crazy".

What the f*ck kind of expression is that? We don't do this kind of
crap. Even if it's possible that the compiler might be able to
optimize it, it's just crazy to call "hweight32()" for something like
this.

Please think about what you're really after for a second, and realize
that "one bit set" is just another way of saying "power of two". And
then sit back, relax, and realize that there are way better ways to
say "is this is a power of two" than counting bits, for chrissake!

Just a simple "is_power_of_2()" would work. But for anybody who cares
about how it works, here's how to see if there is just a single bit
set:

   n & (n-1)

and then zero is a special case. _Why_ it works is left as an exercise
for the reader, because it's an interesting trick that becomes obvious
once you start thinking about how borrowing works in binary
arithmetic, and what that "subtract one" does for the borrow case for
a power-of-two value vs a non-power-of-two. You can also think about
why zero is a special case, and why you might sometimes consider it an
acceptable value (it's basically the overflow case for shifting bits).

> Too clever?

Not clever at all, I'm afraid.

                     Linus

Re: [PATCH] VFS: Sanity check mount flags passed to change_mnt_propagation()

[Valerie Aurora]

On Sat, Aug 28, 2010 at 02:23:51PM -0700, Linus Torvalds wrote:
> On Thu, Aug 26, 2010 at 6:14 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> >
> > ? ? ? ?/* Only one propagation flag should be set, and no others */
> > ? ? ? ?if (hweight32(type) != 1
> 
> Guys, stop with "teh crazy".
> 
> What the f*ck kind of expression is that? We don't do this kind of
> crap. Even if it's possible that the compiler might be able to
> optimize it, it's just crazy to call "hweight32()" for something like
> this.
> 
> Please think about what you're really after for a second, and realize
> that "one bit set" is just another way of saying "power of two". And
> then sit back, relax, and realize that there are way better ways to
> say "is this is a power of two" than counting bits, for chrissake!

I considered is_power_of_2() initially but rejected it as not
particularly readable.  But hey, this is VFS code!

-VAL

commit ce3708ab514d850b0d0939f3fa6c64daad306a15
Author: Valerie Aurora <vaurora@redhat.com>
Date:   Thu Aug 26 11:07:22 2010 -0700

    VFS: Sanity check mount flags passed to change_mnt_propagation()
    
    Sanity check the flags passed to change_mnt_propagation().  Exactly
    one flag should be set.  Return EINVAL otherwise.
    
    Userspace can pass in arbitrary combinations of MS_* flags to mount().
    do_change_type() is called if any of MS_SHARED, MS_PRIVATE, MS_SLAVE,
    or MS_UNBINDABLE is set.  do_change_type() clears MS_REC and then
    calls change_mnt_propagation() with the rest of the user-supplied
    flags.  change_mnt_propagation() clearly assumes only one flag is set
    but do_change_type() does not check that this is true.  For example,
    mount() with flags MS_SHARED | MS_RDONLY does not actually make the
    mount shared or read-only but does clear MNT_UNBINDABLE.
    
    Signed-off-by: Valerie Aurora <vaurora@redhat.com>

diff --git a/fs/namespace.c b/fs/namespace.c
index de402eb..ddc5565 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1484,13 +1484,29 @@ out_unlock:
 }
 
 /*
+ * Sanity check the flags to change_mnt_propagation.
+ */
+
+static int flags_to_propagation_type(int flags) {
+	int type = flags & ~MS_REC;
+
+	/* Fail if any non-propagation flags are set */
+	if (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
+		return 0;
+	/* Only one propagation flag should be set */
+	if (!is_power_of_2(type))
+		return 0;
+	return type;
+}
+
+/*
  * recursively change the type of the mountpoint.
  */
 static int do_change_type(struct path *path, int flag)
 {
 	struct vfsmount *m, *mnt = path->mnt;
 	int recurse = flag & MS_REC;
-	int type = flag & ~MS_REC;
+	int type;
 	int err = 0;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -1499,6 +1515,10 @@ static int do_change_type(struct path *path, int flag)
 	if (path->dentry != path->mnt->mnt_root)
 		return -EINVAL;
 
+	type = flags_to_propagation_type(flag);
+	if (!type)
+		return -EINVAL;
+
 	down_write(&namespace_sem);
 	if (type == MS_SHARED) {
 		err = invent_group_ids(mnt, recurse);

Re: [PATCH] VFS: Sanity check mount flags passed to change_mnt_propagation()

[Karel Zak]

On Thu, Aug 26, 2010 at 04:03:18PM -0400, Valerie Aurora wrote:
> shared/slave/private/unbindable mounts.  Karel Zak will fix the
> mount(8) bug shortly.

 Fixed, thanks for your bug report.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com