VFS: Sanity check mount flags passed to change_mnt_propagation()

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

index de402eb..4987c4c 100644
@@ -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);

Fixed, thanks for your bug report.

Karel

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com

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

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yes, thanks!

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

-VAL

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."

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

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

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

Not clever at all, I'm afraid.

Linus

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

index de402eb..ddc5565 100644
@@ -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);