[linux-cifs-client] cifs: tighten up default file_mode/dir_mode
diff mbox

Message ID 1238931226-4616-1-git-send-email-jlayton@redhat.com
State New, archived
Headers show

Commit Message

Jeff Layton April 5, 2009, 11:33 a.m. UTC
The current default file mode is 02767 and dir mode is 0777. This is
extremely "loose". Given that CIFS is a single-user protocol, these
permissions allow anyone to use the mount -- in effect, giving anyone on
the machine access to the credentials used to mount the share.

Change this by making the default permissions restrict access to only
the uid of the share.

Note that this patch also removes the mandatory locking flags from the
default file_mode. After having looked at how these flags are used by
the kernel, I don't think that keeping them as the default offers any
real benefit. That flag combination makes it so that the kernel enforces
mandatory locking.

Since the server is going to do that for us anyway, I don't think we
want the client to enforce this by default on applications that just
want advisory locks. Anyone that does want this behavior can always
enable it by setting the file_mode appropriately.

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

Comments

simo April 5, 2009, 2:19 p.m. UTC | #1
On Sun, 2009-04-05 at 07:33 -0400, Jeff Layton wrote:
> 
> Note that this patch also removes the mandatory locking flags from the
> default file_mode. After having looked at how these flags are used by
> the kernel, I don't think that keeping them as the default offers any
> real benefit. That flag combination makes it so that the kernel
> enforces
> mandatory locking.
> 
> Since the server is going to do that for us anyway, I don't think we
> want the client to enforce this by default on applications that just
> want advisory locks. Anyone that does want this behavior can always
> enable it by setting the file_mode appropriately.

If we don't enforce it, what happen on the client side?
Do we do a synchronous write to the server to find out if the app was
actually able to write? Or are we going to tell the app the write was
fine to find out only later that we were not able to write back to the
server ?

Simo.
Jeff Layton April 5, 2009, 4:03 p.m. UTC | #2
On Sun, 05 Apr 2009 14:19:08 +0000
simo <idra@samba.org> wrote:

> On Sun, 2009-04-05 at 07:33 -0400, Jeff Layton wrote:
> > 
> > Note that this patch also removes the mandatory locking flags from the
> > default file_mode. After having looked at how these flags are used by
> > the kernel, I don't think that keeping them as the default offers any
> > real benefit. That flag combination makes it so that the kernel
> > enforces
> > mandatory locking.
> > 
> > Since the server is going to do that for us anyway, I don't think we
> > want the client to enforce this by default on applications that just
> > want advisory locks. Anyone that does want this behavior can always
> > enable it by setting the file_mode appropriately.
> 
> If we don't enforce it, what happen on the client side?
> Do we do a synchronous write to the server to find out if the app was
> actually able to write? Or are we going to tell the app the write was
> fine to find out only later that we were not able to write back to the
> server ?

This really only changes the behavior if there is a lock set by another
task on the client itself. If there's a lock set on the server, then
the client isn't really aware of it anyway and can't enforce it.

So it really comes down to what the expected behavior is when a process
on the box is holding a lock on the file and another process wants to
do I/O to it. IMO, it makes sense not to have the kernel attempt to
enforce mandatory locking unless someone specifically wants it. They
can still get it by setting the mode appropriately.

Patch
diff mbox

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 6926023..efaad03 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -826,9 +826,9 @@  cifs_parse_mount_options(char *options, const char *devname,
 	vol->target_rfc1001_name[0] = 0;
 	vol->linux_uid = current_uid();  /* use current_euid() instead? */
 	vol->linux_gid = current_gid();
-	vol->dir_mode = S_IRWXUGO;
-	/* 2767 perms indicate mandatory locking support */
-	vol->file_mode = (S_IRWXUGO | S_ISGID) & (~S_IXGRP);
+
+	/* default to only allowing access to owner of the mount */
+	vol->dir_mode = vol->file_mode = S_IRWXU;
 
 	/* vol->retry default is 0 (i.e. "soft" limited retry not hard retry) */
 	vol->rw = true;