diff mbox

[linux-cifs-client] cifs: make overriding of ownership conditional on new mount options

Message ID 1240331735-18675-1-git-send-email-jlayton@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Layton April 21, 2009, 4:35 p.m. UTC
We have a bit of a problem with the uid= option. The basic issue is that
it means too many things and has too many side-effects.

It's possible to allow an unprivileged user to mount a filesystem if the
user owns the mountpoint, /bin/mount is setuid root, and the mount is
set up in /etc/fstab with the "user" option.

When doing this though, /bin/mount automatically adds the "uid=" and
"gid=" options to the share. This is fortunate since the correct uid=
option is needed in order to tell the upcall what user's credcache to
use when generating the SPNEGO blob.

On a mount without unix extensions this is fine -- you generally will
want the files to be owned by the "owner" of the mount. The problem
comes in on a mount with unix extensions. With those enabled, the
uid/gid options cause the ownership of files to be overriden even though
the server is sending along the ownership info.

This means that it's not possible to have a mount by an unprivileged
user that shows the server's file ownership info. The result is also
inode permissions that have no reflection at all on the server. You
simply cannot separate ownership from the mode in this fashion.

This behavior also makes MultiuserMount option less usable. Once you
pass in the uid= option for a mount, then you can't use unix ownership
info and allow someone to share the mount.

While I'm not thrilled with it, the only solution I can see is to stop
making uid=/gid= force the overriding of ownership on mounts, and to add
new mount options that turn this behavior on.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/connect.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

Comments

simo April 27, 2009, 11:59 a.m. UTC | #1
On Tue, 2009-04-21 at 12:35 -0400, Jeff Layton wrote:
> We have a bit of a problem with the uid= option. The basic issue is
> that
> it means too many things and has too many side-effects.
> 
> It's possible to allow an unprivileged user to mount a filesystem if
> the
> user owns the mountpoint, /bin/mount is setuid root, and the mount is
> set up in /etc/fstab with the "user" option.
> 
> When doing this though, /bin/mount automatically adds the "uid=" and
> "gid=" options to the share. This is fortunate since the correct uid=
> option is needed in order to tell the upcall what user's credcache to
> use when generating the SPNEGO blob.
> 
> On a mount without unix extensions this is fine -- you generally will
> want the files to be owned by the "owner" of the mount. The problem
> comes in on a mount with unix extensions. With those enabled, the
> uid/gid options cause the ownership of files to be overriden even
> though
> the server is sending along the ownership info.
> 
> This means that it's not possible to have a mount by an unprivileged
> user that shows the server's file ownership info. The result is also
> inode permissions that have no reflection at all on the server. You
> simply cannot separate ownership from the mode in this fashion.
> 
> This behavior also makes MultiuserMount option less usable. Once you
> pass in the uid= option for a mount, then you can't use unix ownership
> info and allow someone to share the mount.
> 
> While I'm not thrilled with it, the only solution I can see is to stop
> making uid=/gid= force the overriding of ownership on mounts, and to
> add
> new mount options that turn this behavior on.

I too think we have no other choice.
I don't like the fact that existing users of cifs will see different
results with the same mount option on different kernels, but I also
don't see any other good way to do the change.

Ack, IMO.
Simo.
diff mbox

Patch

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index bacdef1..a420545 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1084,17 +1084,17 @@  cifs_parse_mount_options(char *options, const char *devname,
 				return 1;
 			}
 		} else if (strnicmp(data, "uid", 3) == 0) {
-			if (value && *value) {
+			if (value && *value)
 				vol->linux_uid =
 					simple_strtoul(value, &value, 0);
+		} else if (strnicmp(data, "forceuid", 8) == 0) {
 				vol->override_uid = 1;
-			}
 		} else if (strnicmp(data, "gid", 3) == 0) {
-			if (value && *value) {
+			if (value && *value)
 				vol->linux_gid =
 					simple_strtoul(value, &value, 0);
+		} else if (strnicmp(data, "forcegid", 8) == 0) {
 				vol->override_gid = 1;
-			}
 		} else if (strnicmp(data, "file_mode", 4) == 0) {
 			if (value && *value) {
 				vol->file_mode =