[RFC PATCH] CIFS posix acl permission checking

[RFC PATCH] CIFS posix acl permission checking

[Jon Severinsson]

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

Hello

Early this weak I sent a patch implementing posix acl permission checking in 
the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
as I was unaware of the linux-cifs-client list. I later tried to submit it to 
linux-cifs-client as well, but my message seems to have been lost in the 
moderation queue, so I subscribed and am trying again.

I don't believe my patch is perfect, but I think it's a good start, and would 
like some comments from more experienced cifs developers to be able to get it 
into shape for inclusion in the kernel. 

I did get some comments from Matthew Wilcox at linux-fsdev, but unfortunately 
he never followed up on my response, so I'm including some unresolved 
questions I still have, as well as attaching the patch for further comments.

Best Regards
Jon Severinsson

On Monday 01 March 2010 19:33:41, Jon Severinsson wrote:
> On Monday 01 March 2010 18:03:49, Matthew Wilcox wrote:
>> You've included ifdefs around the check_acl entry in inode_operations,
>> *and* inside the definition of cifs_check_acl.  You only need to do
>> one or the other, and opinion is divided on which is better.
> 
> While I did recognize the redundancy, I decided to follow the same
> convention the other functions in xattr.c did, and include ifdefs at both
> locations.
> 
> I also considered the possible reasons for the existing functions to do
> both, and and came up with two reasons. The first simply being the paradigm
> of defensive programming, always check before doing a call, but never assume
> that the check has been done before being called. The second one is that of
> performance. The ifdefs has to be in cifs_check_acl to protect against other
> callers (while this patch doesn't introduce any, it doesn't prevent further
> patches from adding them), and not including the ifdefs in inode_operations
> would mean a completely useless function call when a feature was turned off
> at compile time. The second one is a micro-optimization I don't really care
> fore, but defensive programming I do respect.
> 
> With this in mind, what do you recommend, double protection, breaking
> convention or changing the existing code?
>
>>> +int cifs_check_acl(struct inode *inode, int mask)
>>> +{
>>> +	int rc = -EOPNOTSUPP;
>>> +#ifdef CONFIG_CIFS_XATTR
>>> +#ifdef CONFIG_CIFS_POSIX
>>> +	struct dentry *dentry;
>>> +	size_t buf_size;
>>> +	void *ea_value = NULL;
>>> +	ssize_t ea_size;
>>> +	struct posix_acl *acl = NULL;
>>
>> I don't think you need to initialise ea_value, do you?
> 
> While currently correct, I find it a good idea to immediately null any
> pointer that is freed in the exit section. Otherwise it is very easy to
> forget to do that the day someone adds a goto prior to the first
> assignment, and not nulling then can have unintended consequences in
> unrelated code. That being said, if you say that the kernel community
> frowns upon that kind of defensive programming I will definitely remove
> it.


[-- Attachment #2: cifs-acl-permission-check-v2.patch --]
[-- Type: text/x-patch, Size: 4882 bytes --]

commit fa0b9415cda17b31966542101bc4ceb0c97c87cb
Author: Jon Severinsson <jon@severinsson.net>
Date:   Mon Mar 1 19:24:30 2010 +0100

    [CIFS] Adds support for permision checking vs. posix acl.
    
    CIFS already supports getting and setting acls through getfacl and setfacl, but
    prior to this patch, any acls was ignored when doing permission checking.

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 29f1da7..0605e11 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
 		on the client (above and beyond ACL on servers) for
 		servers which do not support setting and viewing mode bits,
 		so allowing client to check permissions is useful */
-		return generic_permission(inode, mask, NULL);
+		return generic_permission(inode, mask, inode->i_op->check_acl);
 }
 
 static struct kmem_cache *cifs_inode_cachep;
@@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
@@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
@@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index ac2b24c..6409a83 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
 			 int buflen);
 extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
 			const char *symname);
+
+/* Functions related to extended attributes */
 extern int	cifs_removexattr(struct dentry *, const char *);
 extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
 			size_t, int);
 extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
 extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
+extern int cifs_check_acl(struct inode *inode, int mask);
+
 extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
 
 #ifdef CONFIG_CIFS_EXPERIMENTAL
diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
index a75afa3..a07633b 100644
--- a/fs/cifs/xattr.c
+++ b/fs/cifs/xattr.c
@@ -374,3 +374,71 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
 #endif
 	return rc;
 }
+
+int cifs_check_acl(struct inode *inode, int mask)
+{
+	int rc = -EOPNOTSUPP;
+#ifdef CONFIG_CIFS_XATTR
+#ifdef CONFIG_CIFS_POSIX
+	struct dentry *dentry;
+	size_t buf_size;
+	void *ea_value = NULL;
+	ssize_t ea_size;
+	struct posix_acl *acl = NULL;
+
+	/* CIFS gets acl from server by path, and thus needs a dentry rather than
+	   an inode. Note that the path of each dentry will point to the same inode
+	   on the backing fs at the server, so their acls will be the same, and it
+	   doesn't matter which one we pick, so just pick the fist. */
+	if (!list_empty(&inode->i_dentry))
+		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
+	else
+		return -EINVAL;
+
+	/* Try to fit the extended attribute corresponding to the posix acl in 4k
+	   memory. 4k was chosen because it always fits in a single page, and is
+	   the maximum on a default ext2/3/4 backing fs. */
+	buf_size = 4096;
+	ea_value = kmalloc(buf_size, GFP_KERNEL);
+	if (!ea_value) {
+		rc = -EAGAIN;
+		goto check_acl_exit;
+	}
+	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
+
+	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
+	if (ea_size == -ERANGE) {
+		kfree(ea_value);
+		buf_size = 65536;
+		ea_value = kmalloc(buf_size, GFP_KERNEL);
+		if (!ea_value) {
+			rc = -EAGAIN;
+			goto check_acl_exit;
+		}
+		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
+	}
+
+	/* If we didn't get any extended attribute, set the error code and exit */
+	if (ea_size <= 0) {
+		rc = -EAGAIN;
+		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
+			rc = ea_size;
+		goto check_acl_exit;
+	}
+
+	/* Set the appropriate return value. Adapted from ext4. */
+	acl = posix_acl_from_xattr(ea_value, ea_size);
+	if (IS_ERR(acl))
+		rc = PTR_ERR(acl);
+	else if (acl)
+		rc = posix_acl_permission(inode, acl, mask);
+	else
+		rc = -EAGAIN;
+
+check_acl_exit:
+	posix_acl_release(acl);
+	kfree(ea_value);
+#endif /* CONFIG_CIFS_POSIX */
+#endif /* CONFIG_CIFS_XATTR */
+	return rc;
+}

[RFC PATCH] CIFS posix acl permission checking

[Jon Severinsson]

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

Hello

Early this weak I sent a patch implementing posix acl permission checking in 
the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
as I was unaware of the linux-cifs-client list. I later tried to submit it to 
linux-cifs-client as well, but my message seems to have been lost in the 
moderation queue, so I subscribed and am trying again.

I don't believe my patch is perfect, but I think it's a good start, and would 
like some comments from more experienced cifs developers to be able to get it 
into shape for inclusion in the kernel. 

I did get some comments from Matthew Wilcox at linux-fsdev, but unfortunately 
he never followed up on my response, so I'm including some unresolved 
questions I still have, as well as attaching the patch for further comments.

Best Regards
Jon Severinsson

On Monday 01 March 2010 19:33:41, Jon Severinsson wrote:
> On Monday 01 March 2010 18:03:49, Matthew Wilcox wrote:
>> You've included ifdefs around the check_acl entry in inode_operations,
>> *and* inside the definition of cifs_check_acl.  You only need to do
>> one or the other, and opinion is divided on which is better.
> 
> While I did recognize the redundancy, I decided to follow the same
> convention the other functions in xattr.c did, and include ifdefs at both
> locations.
> 
> I also considered the possible reasons for the existing functions to do
> both, and and came up with two reasons. The first simply being the paradigm
> of defensive programming, always check before doing a call, but never assume
> that the check has been done before being called. The second one is that of
> performance. The ifdefs has to be in cifs_check_acl to protect against other
> callers (while this patch doesn't introduce any, it doesn't prevent further
> patches from adding them), and not including the ifdefs in inode_operations
> would mean a completely useless function call when a feature was turned off
> at compile time. The second one is a micro-optimization I don't really care
> fore, but defensive programming I do respect.
> 
> With this in mind, what do you recommend, double protection, breaking
> convention or changing the existing code?
>
>>> +int cifs_check_acl(struct inode *inode, int mask)
>>> +{
>>> +	int rc = -EOPNOTSUPP;
>>> +#ifdef CONFIG_CIFS_XATTR
>>> +#ifdef CONFIG_CIFS_POSIX
>>> +	struct dentry *dentry;
>>> +	size_t buf_size;
>>> +	void *ea_value = NULL;
>>> +	ssize_t ea_size;
>>> +	struct posix_acl *acl = NULL;
>>
>> I don't think you need to initialise ea_value, do you?
> 
> While currently correct, I find it a good idea to immediately null any
> pointer that is freed in the exit section. Otherwise it is very easy to
> forget to do that the day someone adds a goto prior to the first
> assignment, and not nulling then can have unintended consequences in
> unrelated code. That being said, if you say that the kernel community
> frowns upon that kind of defensive programming I will definitely remove
> it.


[-- Attachment #2: cifs-acl-permission-check-v2.patch --]
[-- Type: text/x-patch, Size: 4882 bytes --]

commit fa0b9415cda17b31966542101bc4ceb0c97c87cb
Author: Jon Severinsson <jon@severinsson.net>
Date:   Mon Mar 1 19:24:30 2010 +0100

    [CIFS] Adds support for permision checking vs. posix acl.
    
    CIFS already supports getting and setting acls through getfacl and setfacl, but
    prior to this patch, any acls was ignored when doing permission checking.

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 29f1da7..0605e11 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
 		on the client (above and beyond ACL on servers) for
 		servers which do not support setting and viewing mode bits,
 		so allowing client to check permissions is useful */
-		return generic_permission(inode, mask, NULL);
+		return generic_permission(inode, mask, inode->i_op->check_acl);
 }
 
 static struct kmem_cache *cifs_inode_cachep;
@@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
@@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
@@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index ac2b24c..6409a83 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
 			 int buflen);
 extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
 			const char *symname);
+
+/* Functions related to extended attributes */
 extern int	cifs_removexattr(struct dentry *, const char *);
 extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
 			size_t, int);
 extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
 extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
+extern int cifs_check_acl(struct inode *inode, int mask);
+
 extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
 
 #ifdef CONFIG_CIFS_EXPERIMENTAL
diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
index a75afa3..a07633b 100644
--- a/fs/cifs/xattr.c
+++ b/fs/cifs/xattr.c
@@ -374,3 +374,71 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
 #endif
 	return rc;
 }
+
+int cifs_check_acl(struct inode *inode, int mask)
+{
+	int rc = -EOPNOTSUPP;
+#ifdef CONFIG_CIFS_XATTR
+#ifdef CONFIG_CIFS_POSIX
+	struct dentry *dentry;
+	size_t buf_size;
+	void *ea_value = NULL;
+	ssize_t ea_size;
+	struct posix_acl *acl = NULL;
+
+	/* CIFS gets acl from server by path, and thus needs a dentry rather than
+	   an inode. Note that the path of each dentry will point to the same inode
+	   on the backing fs at the server, so their acls will be the same, and it
+	   doesn't matter which one we pick, so just pick the fist. */
+	if (!list_empty(&inode->i_dentry))
+		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
+	else
+		return -EINVAL;
+
+	/* Try to fit the extended attribute corresponding to the posix acl in 4k
+	   memory. 4k was chosen because it always fits in a single page, and is
+	   the maximum on a default ext2/3/4 backing fs. */
+	buf_size = 4096;
+	ea_value = kmalloc(buf_size, GFP_KERNEL);
+	if (!ea_value) {
+		rc = -EAGAIN;
+		goto check_acl_exit;
+	}
+	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
+
+	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
+	if (ea_size == -ERANGE) {
+		kfree(ea_value);
+		buf_size = 65536;
+		ea_value = kmalloc(buf_size, GFP_KERNEL);
+		if (!ea_value) {
+			rc = -EAGAIN;
+			goto check_acl_exit;
+		}
+		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
+	}
+
+	/* If we didn't get any extended attribute, set the error code and exit */
+	if (ea_size <= 0) {
+		rc = -EAGAIN;
+		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
+			rc = ea_size;
+		goto check_acl_exit;
+	}
+
+	/* Set the appropriate return value. Adapted from ext4. */
+	acl = posix_acl_from_xattr(ea_value, ea_size);
+	if (IS_ERR(acl))
+		rc = PTR_ERR(acl);
+	else if (acl)
+		rc = posix_acl_permission(inode, acl, mask);
+	else
+		rc = -EAGAIN;
+
+check_acl_exit:
+	posix_acl_release(acl);
+	kfree(ea_value);
+#endif /* CONFIG_CIFS_POSIX */
+#endif /* CONFIG_CIFS_XATTR */
+	return rc;
+}

[-- Attachment #3: Type: text/plain, Size: 172 bytes --]

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@lists.samba.org
https://lists.samba.org/mailman/listinfo/linux-cifs-client

Re: [linux-cifs-client] [RFC PATCH] CIFS posix acl permission checking

[simo]

On Thu, 2010-03-04 at 11:50 +0100, Jon Severinsson wrote:
> Hello
> 
> Early this weak I sent a patch implementing posix acl permission checking in 
> the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
> as I was unaware of the linux-cifs-client list. I later tried to submit it to 
> linux-cifs-client as well, but my message seems to have been lost in the 
> moderation queue, so I subscribed and am trying again.
> 
> I don't believe my patch is perfect, but I think it's a good start, and would 
> like some comments from more experienced cifs developers to be able to get it 
> into shape for inclusion in the kernel. 
> 
> I did get some comments from Matthew Wilcox at linux-fsdev, but unfortunately 
> he never followed up on my response, so I'm including some unresolved 
> questions I still have, as well as attaching the patch for further comments.

Hi Jon,
although you did a good job with the code itself, I have to say that I
think the approach is just wrong. Checking ACLs on the client is simply
the wrong way to go. It is just racy and it is not authoritative anyway.

It is like trying to look up a path using a cached directory listing and
then try to open by inode. It simply doesn't work, by the time you do
that, things may have been completely changed on the server.

And we are not counting the problem that with CIFS (and samba in
particular) the client have no way to know what are the real credentials
assigned to the session and that the client may have no idea what the
users and groups in the ACL are (if client and server do not use exactly
the same user database).

The right way is to let the server enforce ACLs, and use multisessions
mounts if multiple users are involved. Time would be better spent
working in that direction IMO.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo@samba.org>
Principal Software Engineer at Red Hat, Inc. <simo@redhat.com>

Re: [RFC PATCH] CIFS posix acl permission checking

[simo]

On Thu, 2010-03-04 at 11:50 +0100, Jon Severinsson wrote:
> Hello
> 
> Early this weak I sent a patch implementing posix acl permission checking in 
> the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
> as I was unaware of the linux-cifs-client list. I later tried to submit it to 
> linux-cifs-client as well, but my message seems to have been lost in the 
> moderation queue, so I subscribed and am trying again.
> 
> I don't believe my patch is perfect, but I think it's a good start, and would 
> like some comments from more experienced cifs developers to be able to get it 
> into shape for inclusion in the kernel. 
> 
> I did get some comments from Matthew Wilcox at linux-fsdev, but unfortunately 
> he never followed up on my response, so I'm including some unresolved 
> questions I still have, as well as attaching the patch for further comments.

Hi Jon,
although you did a good job with the code itself, I have to say that I
think the approach is just wrong. Checking ACLs on the client is simply
the wrong way to go. It is just racy and it is not authoritative anyway.

It is like trying to look up a path using a cached directory listing and
then try to open by inode. It simply doesn't work, by the time you do
that, things may have been completely changed on the server.

And we are not counting the problem that with CIFS (and samba in
particular) the client have no way to know what are the real credentials
assigned to the session and that the client may have no idea what the
users and groups in the ACL are (if client and server do not use exactly
the same user database).

The right way is to let the server enforce ACLs, and use multisessions
mounts if multiple users are involved. Time would be better spent
working in that direction IMO.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo@samba.org>
Principal Software Engineer at Red Hat, Inc. <simo@redhat.com>

Re: [RFC PATCH] CIFS posix acl permission checking

[Jon Severinsson]

Den Thursday 04 March 2010 14:44:22 skrev du:
> although you did a good job with the code itself, I have to say that I
> think the approach is just wrong. Checking ACLs on the client is simply
> the wrong way to go. It is just racy and it is not authoritative anyway.

The problem with this approach is that the server checks the permissions using 
the credentials of the user doing the mount, not the user trying to access the 
file. This leaves a big gaping security hole on any multiuser system, and even 
on single user systems you sill loose any benefits from privilege separation 
(such as running system daemons as an unprivileged user).

So while it is possible to do it that way (by including the noperm mount 
option when mounting), it is generally a bad idea to do so.

As you are going to do permission checking on the client side anyway, I do not 
see the problem consulting the acl when doing so. As not consulting the acl 
only restricts access to files I'm supposed to have access to (and indeed has 
access to if using smbclient), it only results in me being forced to use the 
uid, gid, file_mode, and/or dir_mode mount options, at witch point I can't see 
whether I have access or not other than by trying to do the access, as it will 
always look as if I have access.

> It is like trying to look up a path using a cached directory listing and
> then try to open by inode. It simply doesn't work, by the time you do
> that, things may have been completely changed on the server.

I'm afraid I'm not sure exactly what you mean by this section, could you 
please clarify.

> And we are not counting the problem that with CIFS (and samba in
> particular) the client have no way to know what are the real credentials
> assigned to the session and that the client may have no idea what the
> users and groups in the ACL are (if client and server do not use exactly
> the same user database).

Of course this only works as intended if the client and server shares the same 
user database. In my case that is through ldap, but could as well be nis, 
winbind or some other similar nss module. However, that is the case already, 
without any acl, as UNIX permission bits have exactly the same problem. If you 
are mounting something from a server with a different user database, you'll 
need to use the uid, gid, file_mode, and/or dir_mode mount options, but that is 
no different from the situation without this patch.

> The right way is to let the server enforce ACLs, and use multisessions
> mounts if multiple users are involved. Time would be better spent
> working in that direction IMO.

If by multisession mounts you mean something like nfs, where the client tells 
the server what user is trying to do the access, that can only work if either 
the server blindly trusts the client (as in nfs3) or with some complex 
cryptographic token infrastructure (as in nfs4 with kerberos authentication). 
The first case is simply not very secure, and a cryptographic token 
infrastructure is needlessly complex. If someone were to work on making 
setting up a cryptographic token infrastructure simpler I would love that, but 
I'm not even remotely qualified to do so, and in the meantime the current samba 
infrastructure fulfils all my need, except that the cifs module erroneously 
refuses me the rights I'm granted through an acl.

> Simo.

Regards
Jon Severinsson

Re: [linux-cifs-client] [RFC PATCH] CIFS posix acl permission checking

[simo]

On Thu, 2010-03-04 at 16:21 +0100, Jon Severinsson wrote:
> Den Thursday 04 March 2010 14:44:22 skrev du:
> > although you did a good job with the code itself, I have to say that I
> > think the approach is just wrong. Checking ACLs on the client is simply
> > the wrong way to go. It is just racy and it is not authoritative anyway.
> 
> The problem with this approach is that the server checks the permissions using 
> the credentials of the user doing the mount, not the user trying to access the 
> file. This leaves a big gaping security hole on any multiuser system, and even 
> on single user systems you sill loose any benefits from privilege separation 
> (such as running system daemons as an unprivileged user).

Letting a different user access the mount point *is* a security
violation in itself. The CIFS security model lies in per user sessions.
The right way to fix the problem is multi-session mounts. Allowing a
different user to use a user session is a violation of the security
model of CIFS.

Trying to put a bandaid on a gross policy and security violation is not
really useful :)

> So while it is possible to do it that way (by including the noperm mount 
> option when mounting), it is generally a bad idea to do so.
> 
> As you are going to do permission checking on the client side anyway,

This is the problem, you shouldn't. CIFS is not NFS, you are abusing
your access credentials simply because they are stored in the kernel.

>  I do not 
> see the problem consulting the acl when doing so. As not consulting the acl 
> only restricts access to files I'm supposed to have access to (and indeed has 
> access to if using smbclient), it only results in me being forced to use the 
> uid, gid, file_mode, and/or dir_mode mount options, at witch point I can't see 
> whether I have access or not other than by trying to do the access, as it will 
> always look as if I have access.

Sorry, but just piling up errors on top of other errors does not really
make the situation better.

> > And we are not counting the problem that with CIFS (and samba in
> > particular) the client have no way to know what are the real credentials
> > assigned to the session and that the client may have no idea what the
> > users and groups in the ACL are (if client and server do not use exactly
> > the same user database).
> 
> Of course this only works as intended if the client and server shares the same 
> user database. In my case that is through ldap, but could as well be nis, 
> winbind or some other similar nss module. However, that is the case already, 
> without any acl, as UNIX permission bits have exactly the same problem. If you 
> are mounting something from a server with a different user database, you'll 
> need to use the uid, gid, file_mode, and/or dir_mode mount options, but that is 
> no different from the situation without this patch.

Yes, the current situation is not right either, and definitely need
fixing.

> > The right way is to let the server enforce ACLs, and use multisessions
> > mounts if multiple users are involved. Time would be better spent
> > working in that direction IMO.
> 
> If by multisession mounts you mean something like nfs, where the client tells 
> the server what user is trying to do the access, that can only work if either 
> the server blindly trusts the client (as in nfs3) or with some complex 
> cryptographic token infrastructure (as in nfs4 with kerberos authentication). 

No, you cannot "tell" the server who you are like in NFS, this is the
main point. CIFS has a security model that is different from (old) NFS.
In CIFS each session is authenticated, that's why you should have
multi-session if you want to access the same mount point as different
users. Like NFSv4 you should authenticate each user first, create it's
own session and then let him in, or deny access completely (or degrade
access to the guest user if the remote server allow you so).

> The first case is simply not very secure,

The first case is simply not possible with CIFS.

>  and a cryptographic token infrastructure is needlessly complex.

Yet there is no real alternative, short of changing the CIFS model to
allow the server to trust the client machine.

> If someone were to work on making 
> setting up a cryptographic token infrastructure simpler I would love that, but 
> I'm not even remotely qualified to do so, and in the meantime the current samba 
> infrastructure fulfils all my need, except that the cifs module erroneously 
> refuses me the rights I'm granted through an acl.

We already have the cifs.upcall thing, which is the prerequisite to get
multi-session work at least with kerberos. And it can be easily extended
to allow also to use NTLM cached credentials.

That is a goal that would be better pursuing.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo@samba.org>
Principal Software Engineer at Red Hat, Inc. <simo@redhat.com>

Re: [RFC PATCH] CIFS posix acl permission checking

[simo]

On Thu, 2010-03-04 at 16:21 +0100, Jon Severinsson wrote:
> Den Thursday 04 March 2010 14:44:22 skrev du:
> > although you did a good job with the code itself, I have to say that I
> > think the approach is just wrong. Checking ACLs on the client is simply
> > the wrong way to go. It is just racy and it is not authoritative anyway.
> 
> The problem with this approach is that the server checks the permissions using 
> the credentials of the user doing the mount, not the user trying to access the 
> file. This leaves a big gaping security hole on any multiuser system, and even 
> on single user systems you sill loose any benefits from privilege separation 
> (such as running system daemons as an unprivileged user).

Letting a different user access the mount point *is* a security
violation in itself. The CIFS security model lies in per user sessions.
The right way to fix the problem is multi-session mounts. Allowing a
different user to use a user session is a violation of the security
model of CIFS.

Trying to put a bandaid on a gross policy and security violation is not
really useful :)

> So while it is possible to do it that way (by including the noperm mount 
> option when mounting), it is generally a bad idea to do so.
> 
> As you are going to do permission checking on the client side anyway,

This is the problem, you shouldn't. CIFS is not NFS, you are abusing
your access credentials simply because they are stored in the kernel.

>  I do not 
> see the problem consulting the acl when doing so. As not consulting the acl 
> only restricts access to files I'm supposed to have access to (and indeed has 
> access to if using smbclient), it only results in me being forced to use the 
> uid, gid, file_mode, and/or dir_mode mount options, at witch point I can't see 
> whether I have access or not other than by trying to do the access, as it will 
> always look as if I have access.

Sorry, but just piling up errors on top of other errors does not really
make the situation better.

> > And we are not counting the problem that with CIFS (and samba in
> > particular) the client have no way to know what are the real credentials
> > assigned to the session and that the client may have no idea what the
> > users and groups in the ACL are (if client and server do not use exactly
> > the same user database).
> 
> Of course this only works as intended if the client and server shares the same 
> user database. In my case that is through ldap, but could as well be nis, 
> winbind or some other similar nss module. However, that is the case already, 
> without any acl, as UNIX permission bits have exactly the same problem. If you 
> are mounting something from a server with a different user database, you'll 
> need to use the uid, gid, file_mode, and/or dir_mode mount options, but that is 
> no different from the situation without this patch.

Yes, the current situation is not right either, and definitely need
fixing.

> > The right way is to let the server enforce ACLs, and use multisessions
> > mounts if multiple users are involved. Time would be better spent
> > working in that direction IMO.
> 
> If by multisession mounts you mean something like nfs, where the client tells 
> the server what user is trying to do the access, that can only work if either 
> the server blindly trusts the client (as in nfs3) or with some complex 
> cryptographic token infrastructure (as in nfs4 with kerberos authentication). 

No, you cannot "tell" the server who you are like in NFS, this is the
main point. CIFS has a security model that is different from (old) NFS.
In CIFS each session is authenticated, that's why you should have
multi-session if you want to access the same mount point as different
users. Like NFSv4 you should authenticate each user first, create it's
own session and then let him in, or deny access completely (or degrade
access to the guest user if the remote server allow you so).

> The first case is simply not very secure,

The first case is simply not possible with CIFS.

>  and a cryptographic token infrastructure is needlessly complex.

Yet there is no real alternative, short of changing the CIFS model to
allow the server to trust the client machine.

> If someone were to work on making 
> setting up a cryptographic token infrastructure simpler I would love that, but 
> I'm not even remotely qualified to do so, and in the meantime the current samba 
> infrastructure fulfils all my need, except that the cifs module erroneously 
> refuses me the rights I'm granted through an acl.

We already have the cifs.upcall thing, which is the prerequisite to get
multi-session work at least with kerberos. And it can be easily extended
to allow also to use NTLM cached credentials.

That is a goal that would be better pursuing.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo@samba.org>
Principal Software Engineer at Red Hat, Inc. <simo@redhat.com>

Re: [linux-cifs-client] [RFC PATCH] CIFS posix acl permission checking

[Jeff Layton]

On Thu, 4 Mar 2010 11:50:04 +0100
Jon Severinsson <jon@severinsson.net> wrote:

> Hello
> 
> Early this weak I sent a patch implementing posix acl permission checking in 
> the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
> as I was unaware of the linux-cifs-client list. I later tried to submit it to 
> linux-cifs-client as well, but my message seems to have been lost in the 
> moderation queue, so I subscribed and am trying again.
> 
> I don't believe my patch is perfect, but I think it's a good start, and would 
> like some comments from more experienced cifs developers to be able to get it 
> into shape for inclusion in the kernel. 
> 
> I did get some comments from Matthew Wilcox at linux-fsdev, but unfortunately 
> he never followed up on my response, so I'm including some unresolved 
> questions I still have, as well as attaching the patch for further comments.
> 
> Best Regards
> Jon Severinsson
>

(cc'ing Michael Adam since he's been working on a similar patch)

I've looked over the patch and it's pretty good for a first attempt.
There are a few minor problems with it, but it's a reasonable first
pass.

The issues I have are with the approach -- you've stepped into a bit of
a minefield with regards to CIFS' design. The issues that Simo brings up
however are quite valid. Let me just state outright that permission
checking on the client is a completely bogus concept. There are a
couple of problems with this approach:

1) Someone could change the permissions on the server between the time
you check them and when you do your operation. Yes, I know that CIFS
already does this for permission bits, but that's dumb too.

2) Ownership matters. This patch will have no effect on how files get
created. They still get created with the owner set to the user who's
credentials were used, and you can't change them afterward (since users
can't "chown" files to other users). Now, it is possible to mount a
samba server with root's credentials. Then you can use the "setuids"
mount option to make chown's work and you *might* get files created the
way you intend. I really, really do not recommend this though -- it's a
bad idea to allow any user to share root's server credentials.

I'm convinced, after working on it for more than 3 years that the *only*
proper fix for the nightmare that is CIFS permissions is to make CIFS
use proper credentials in the first place. CIFS is currently completely
broken in this regard -- it's designed such that all accesses to the
mount use the same set of credentials, and that's just plain wrong.

Fixing this entails establishing new SMB sessions on the fly whenever a
user wants to do an on the wire operation. Obviously, we can't prompt
for passwords for this, so we need to limit this to krb5 creds or come
up with a way for people to stash credentials for the kernel to use for
this purpose. I'm about 1/3 of the way into a first draft of a patchset
to do this, but it won't be fixed anytime soon and may not be suitable
for CIFS with SMB2 getting development focus now.

Now, all of that said...I don't have a specific issue with adding a
patch to do this. I think this approach is completely ass-backwards and
broken, but we do already attempt to enforce permission bits on the
client currently. Adding checks for POSIX acl's don't make this (much)
worse.

The one thing I don't think we want though is to turn this on by
default since it means an extra round trip to the server every time
you want to check permissions. This needs to be "switchable" somehow --
possibly via mount option (maybe -o checkposixacls or something?)

> commit fa0b9415cda17b31966542101bc4ceb0c97c87cb
> Author: Jon Severinsson <jon@severinsson.net>
> Date:   Mon Mar 1 19:24:30 2010 +0100
> 
>     [CIFS] Adds support for permision checking vs. posix acl.
>     
>     CIFS already supports getting and setting acls through getfacl and setfacl, but
>     prior to this patch, any acls was ignored when doing permission checking.
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 29f1da7..0605e11 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
>  		on the client (above and beyond ACL on servers) for
>  		servers which do not support setting and viewing mode bits,
>  		so allowing client to check permissions is useful */
> -		return generic_permission(inode, mask, NULL);
> +		return generic_permission(inode, mask, inode->i_op->check_acl);
>  }
>  
>  static struct kmem_cache *cifs_inode_cachep;
> @@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
>  	.getxattr = cifs_getxattr,
>  	.listxattr = cifs_listxattr,
>  	.removexattr = cifs_removexattr,
> +#ifdef CONFIG_CIFS_POSIX
> +	.check_acl = cifs_check_acl,
> +#endif
>  #endif
>  };
>  
> @@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
>  	.getxattr = cifs_getxattr,
>  	.listxattr = cifs_listxattr,
>  	.removexattr = cifs_removexattr,
> +#ifdef CONFIG_CIFS_POSIX
> +	.check_acl = cifs_check_acl,
> +#endif
>  #endif
>  };
>  
> @@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
>  	.getxattr = cifs_getxattr,
>  	.listxattr = cifs_listxattr,
>  	.removexattr = cifs_removexattr,
> +#ifdef CONFIG_CIFS_POSIX
> +	.check_acl = cifs_check_acl,
> +#endif
>  #endif
>  };
>  
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index ac2b24c..6409a83 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
>  			 int buflen);
>  extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
>  			const char *symname);
> +
> +/* Functions related to extended attributes */
>  extern int	cifs_removexattr(struct dentry *, const char *);
>  extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
>  			size_t, int);
>  extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
>  extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
> +extern int cifs_check_acl(struct inode *inode, int mask);
> +
>  extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
>  
>  #ifdef CONFIG_CIFS_EXPERIMENTAL
> diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> index a75afa3..a07633b 100644
> --- a/fs/cifs/xattr.c
> +++ b/fs/cifs/xattr.c
> @@ -374,3 +374,71 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
>  #endif
>  	return rc;
>  }
> +
> +int cifs_check_acl(struct inode *inode, int mask)
> +{
> +	int rc = -EOPNOTSUPP;
> +#ifdef CONFIG_CIFS_XATTR
> +#ifdef CONFIG_CIFS_POSIX
> +	struct dentry *dentry;
> +	size_t buf_size;
> +	void *ea_value = NULL;
> +	ssize_t ea_size;
> +	struct posix_acl *acl = NULL;
> +
> +	/* CIFS gets acl from server by path, and thus needs a dentry rather than
> +	   an inode. Note that the path of each dentry will point to the same inode
> +	   on the backing fs at the server, so their acls will be the same, and it
> +	   doesn't matter which one we pick, so just pick the fist. */

^^^ comments should follow kernel coding styles. Be sure to run your
patch through scripts/checkpatch.pl too.

> +	if (!list_empty(&inode->i_dentry))
> +		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);

		^^^^ you should probably dget this dentry to take a
		reference to it and then put it when you're finished
		with it. It could vanish out of the cache before you
		have a chance to use it otherwise.

> +	else
> +		return -EINVAL;
> +
> +	/* Try to fit the extended attribute corresponding to the posix acl in 4k
> +	   memory. 4k was chosen because it always fits in a single page, and is
> +	   the maximum on a default ext2/3/4 backing fs. */
> +	buf_size = 4096;
> +	ea_value = kmalloc(buf_size, GFP_KERNEL);
> +	if (!ea_value) {
> +		rc = -EAGAIN;
> +		goto check_acl_exit;
> +	}
> +	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> +
> +	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
> +	if (ea_size == -ERANGE) {
> +		kfree(ea_value);
> +		buf_size = 65536;
> +		ea_value = kmalloc(buf_size, GFP_KERNEL);
> +		if (!ea_value) {
> +			rc = -EAGAIN;
> +			goto check_acl_exit;
> +		}
> +		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> +	}
> +
> +	/* If we didn't get any extended attribute, set the error code and exit */
> +	if (ea_size <= 0) {
> +		rc = -EAGAIN;
> +		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
> +			rc = ea_size;
> +		goto check_acl_exit;
> +	}
> +
> +	/* Set the appropriate return value. Adapted from ext4. */
> +	acl = posix_acl_from_xattr(ea_value, ea_size);
> +	if (IS_ERR(acl))
> +		rc = PTR_ERR(acl);
> +	else if (acl)
> +		rc = posix_acl_permission(inode, acl, mask);
> +	else
> +		rc = -EAGAIN;
> +
> +check_acl_exit:
> +	posix_acl_release(acl);
> +	kfree(ea_value);
> +#endif /* CONFIG_CIFS_POSIX */
> +#endif /* CONFIG_CIFS_XATTR */
> +	return rc;
> +}

The rest of the patch looks reasonable but I agree with Simo that all
of our time would be better spent working to make CIFS use proper
credentials on the wire.

-- 
Jeff Layton <jlayton@samba.org>

Re: [RFC PATCH] CIFS posix acl permission checking

[Jeff Layton]

On Thu, 4 Mar 2010 11:50:04 +0100
Jon Severinsson <jon@severinsson.net> wrote:

> Hello
> 
> Early this weak I sent a patch implementing posix acl permission checking in 
> the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
> as I was unaware of the linux-cifs-client list. I later tried to submit it to 
> linux-cifs-client as well, but my message seems to have been lost in the 
> moderation queue, so I subscribed and am trying again.
> 
> I don't believe my patch is perfect, but I think it's a good start, and would 
> like some comments from more experienced cifs developers to be able to get it 
> into shape for inclusion in the kernel. 
> 
> I did get some comments from Matthew Wilcox at linux-fsdev, but unfortunately 
> he never followed up on my response, so I'm including some unresolved 
> questions I still have, as well as attaching the patch for further comments.
> 
> Best Regards
> Jon Severinsson
>

(cc'ing Michael Adam since he's been working on a similar patch)

I've looked over the patch and it's pretty good for a first attempt.
There are a few minor problems with it, but it's a reasonable first
pass.

The issues I have are with the approach -- you've stepped into a bit of
a minefield with regards to CIFS' design. The issues that Simo brings up
however are quite valid. Let me just state outright that permission
checking on the client is a completely bogus concept. There are a
couple of problems with this approach:

1) Someone could change the permissions on the server between the time
you check them and when you do your operation. Yes, I know that CIFS
already does this for permission bits, but that's dumb too.

2) Ownership matters. This patch will have no effect on how files get
created. They still get created with the owner set to the user who's
credentials were used, and you can't change them afterward (since users
can't "chown" files to other users). Now, it is possible to mount a
samba server with root's credentials. Then you can use the "setuids"
mount option to make chown's work and you *might* get files created the
way you intend. I really, really do not recommend this though -- it's a
bad idea to allow any user to share root's server credentials.

I'm convinced, after working on it for more than 3 years that the *only*
proper fix for the nightmare that is CIFS permissions is to make CIFS
use proper credentials in the first place. CIFS is currently completely
broken in this regard -- it's designed such that all accesses to the
mount use the same set of credentials, and that's just plain wrong.

Fixing this entails establishing new SMB sessions on the fly whenever a
user wants to do an on the wire operation. Obviously, we can't prompt
for passwords for this, so we need to limit this to krb5 creds or come
up with a way for people to stash credentials for the kernel to use for
this purpose. I'm about 1/3 of the way into a first draft of a patchset
to do this, but it won't be fixed anytime soon and may not be suitable
for CIFS with SMB2 getting development focus now.

Now, all of that said...I don't have a specific issue with adding a
patch to do this. I think this approach is completely ass-backwards and
broken, but we do already attempt to enforce permission bits on the
client currently. Adding checks for POSIX acl's don't make this (much)
worse.

The one thing I don't think we want though is to turn this on by
default since it means an extra round trip to the server every time
you want to check permissions. This needs to be "switchable" somehow --
possibly via mount option (maybe -o checkposixacls or something?)

> commit fa0b9415cda17b31966542101bc4ceb0c97c87cb
> Author: Jon Severinsson <jon@severinsson.net>
> Date:   Mon Mar 1 19:24:30 2010 +0100
> 
>     [CIFS] Adds support for permision checking vs. posix acl.
>     
>     CIFS already supports getting and setting acls through getfacl and setfacl, but
>     prior to this patch, any acls was ignored when doing permission checking.
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 29f1da7..0605e11 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
>  		on the client (above and beyond ACL on servers) for
>  		servers which do not support setting and viewing mode bits,
>  		so allowing client to check permissions is useful */
> -		return generic_permission(inode, mask, NULL);
> +		return generic_permission(inode, mask, inode->i_op->check_acl);
>  }
>  
>  static struct kmem_cache *cifs_inode_cachep;
> @@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
>  	.getxattr = cifs_getxattr,
>  	.listxattr = cifs_listxattr,
>  	.removexattr = cifs_removexattr,
> +#ifdef CONFIG_CIFS_POSIX
> +	.check_acl = cifs_check_acl,
> +#endif
>  #endif
>  };
>  
> @@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
>  	.getxattr = cifs_getxattr,
>  	.listxattr = cifs_listxattr,
>  	.removexattr = cifs_removexattr,
> +#ifdef CONFIG_CIFS_POSIX
> +	.check_acl = cifs_check_acl,
> +#endif
>  #endif
>  };
>  
> @@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
>  	.getxattr = cifs_getxattr,
>  	.listxattr = cifs_listxattr,
>  	.removexattr = cifs_removexattr,
> +#ifdef CONFIG_CIFS_POSIX
> +	.check_acl = cifs_check_acl,
> +#endif
>  #endif
>  };
>  
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index ac2b24c..6409a83 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
>  			 int buflen);
>  extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
>  			const char *symname);
> +
> +/* Functions related to extended attributes */
>  extern int	cifs_removexattr(struct dentry *, const char *);
>  extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
>  			size_t, int);
>  extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
>  extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
> +extern int cifs_check_acl(struct inode *inode, int mask);
> +
>  extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
>  
>  #ifdef CONFIG_CIFS_EXPERIMENTAL
> diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> index a75afa3..a07633b 100644
> --- a/fs/cifs/xattr.c
> +++ b/fs/cifs/xattr.c
> @@ -374,3 +374,71 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
>  #endif
>  	return rc;
>  }
> +
> +int cifs_check_acl(struct inode *inode, int mask)
> +{
> +	int rc = -EOPNOTSUPP;
> +#ifdef CONFIG_CIFS_XATTR
> +#ifdef CONFIG_CIFS_POSIX
> +	struct dentry *dentry;
> +	size_t buf_size;
> +	void *ea_value = NULL;
> +	ssize_t ea_size;
> +	struct posix_acl *acl = NULL;
> +
> +	/* CIFS gets acl from server by path, and thus needs a dentry rather than
> +	   an inode. Note that the path of each dentry will point to the same inode
> +	   on the backing fs at the server, so their acls will be the same, and it
> +	   doesn't matter which one we pick, so just pick the fist. */

^^^ comments should follow kernel coding styles. Be sure to run your
patch through scripts/checkpatch.pl too.

> +	if (!list_empty(&inode->i_dentry))
> +		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);

		^^^^ you should probably dget this dentry to take a
		reference to it and then put it when you're finished
		with it. It could vanish out of the cache before you
		have a chance to use it otherwise.

> +	else
> +		return -EINVAL;
> +
> +	/* Try to fit the extended attribute corresponding to the posix acl in 4k
> +	   memory. 4k was chosen because it always fits in a single page, and is
> +	   the maximum on a default ext2/3/4 backing fs. */
> +	buf_size = 4096;
> +	ea_value = kmalloc(buf_size, GFP_KERNEL);
> +	if (!ea_value) {
> +		rc = -EAGAIN;
> +		goto check_acl_exit;
> +	}
> +	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> +
> +	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
> +	if (ea_size == -ERANGE) {
> +		kfree(ea_value);
> +		buf_size = 65536;
> +		ea_value = kmalloc(buf_size, GFP_KERNEL);
> +		if (!ea_value) {
> +			rc = -EAGAIN;
> +			goto check_acl_exit;
> +		}
> +		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> +	}
> +
> +	/* If we didn't get any extended attribute, set the error code and exit */
> +	if (ea_size <= 0) {
> +		rc = -EAGAIN;
> +		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
> +			rc = ea_size;
> +		goto check_acl_exit;
> +	}
> +
> +	/* Set the appropriate return value. Adapted from ext4. */
> +	acl = posix_acl_from_xattr(ea_value, ea_size);
> +	if (IS_ERR(acl))
> +		rc = PTR_ERR(acl);
> +	else if (acl)
> +		rc = posix_acl_permission(inode, acl, mask);
> +	else
> +		rc = -EAGAIN;
> +
> +check_acl_exit:
> +	posix_acl_release(acl);
> +	kfree(ea_value);
> +#endif /* CONFIG_CIFS_POSIX */
> +#endif /* CONFIG_CIFS_XATTR */
> +	return rc;
> +}

The rest of the patch looks reasonable but I agree with Simo that all
of our time would be better spent working to make CIFS use proper
credentials on the wire.

-- 
Jeff Layton <jlayton@samba.org>

Re: [linux-cifs-client] [RFC PATCH] CIFS posix acl permission checking

[Jeremy Allison]

On Thu, Mar 04, 2010 at 10:51:53AM -0500, simo wrote:
> 
> Letting a different user access the mount point *is* a security
> violation in itself. The CIFS security model lies in per user sessions.
> The right way to fix the problem is multi-session mounts. Allowing a
> different user to use a user session is a violation of the security
> model of CIFS.

Multi-session mounts are the only sane fix. This is what Windows
does in their redirectory (when a process with different credentials
traverses into a mount point a new sessionsetup is done to get remote
credentials).

Jeremy.

Re: [RFC PATCH] CIFS posix acl permission checking

[Jeremy Allison]

On Thu, Mar 04, 2010 at 10:51:53AM -0500, simo wrote:
> 
> Letting a different user access the mount point *is* a security
> violation in itself. The CIFS security model lies in per user sessions.
> The right way to fix the problem is multi-session mounts. Allowing a
> different user to use a user session is a violation of the security
> model of CIFS.

Multi-session mounts are the only sane fix. This is what Windows
does in their redirectory (when a process with different credentials
traverses into a mount point a new sessionsetup is done to get remote
credentials).

Jeremy.

Re: [linux-cifs-client] [RFC PATCH] CIFS posix acl permission checking

[Michael Adam]

[-- Attachment #1.1: Type: text/plain, Size: 11118 bytes --]

Hi,

yes, the coincidence is really funny!

I have been working on a similar patch, which I attach just for
reference. It is just in a proof-of-concept stage with some
printk calls for my personal debugging, and the code is not yet
conditional.

The main difference is that I used the CIFSSMBGetPosixACL call
instead of handcrafting the reading of the xtended attribute.

I have been chatting extensively chatting with Jeff about the
implementation of multi session/user mounts, which is definitely
the way to go but much more coding work.

I also noticed the problem of file ownership, which should
in principle be solvabel by useing the "setuids" mount option,
but this seemd to have no effect in my test setup with uml
linux running from a 2.6.32 git checkout.

Cheers - Michael

Jeff Layton wrote:
> On Thu, 4 Mar 2010 11:50:04 +0100
> Jon Severinsson <jon@severinsson.net> wrote:
> 
> > Hello
> > 
> > Early this weak I sent a patch implementing posix acl permission checking in 
> > the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
> > as I was unaware of the linux-cifs-client list. I later tried to submit it to 
> > linux-cifs-client as well, but my message seems to have been lost in the 
> > moderation queue, so I subscribed and am trying again.
> > 
> > I don't believe my patch is perfect, but I think it's a good start, and would 
> > like some comments from more experienced cifs developers to be able to get it 
> > into shape for inclusion in the kernel. 
> > 
> > I did get some comments from Matthew Wilcox at linux-fsdev, but unfortunately 
> > he never followed up on my response, so I'm including some unresolved 
> > questions I still have, as well as attaching the patch for further comments.
> > 
> > Best Regards
> > Jon Severinsson
> >
> 
> (cc'ing Michael Adam since he's been working on a similar patch)
> 
> I've looked over the patch and it's pretty good for a first attempt.
> There are a few minor problems with it, but it's a reasonable first
> pass.
> 
> The issues I have are with the approach -- you've stepped into a bit of
> a minefield with regards to CIFS' design. The issues that Simo brings up
> however are quite valid. Let me just state outright that permission
> checking on the client is a completely bogus concept. There are a
> couple of problems with this approach:
> 
> 1) Someone could change the permissions on the server between the time
> you check them and when you do your operation. Yes, I know that CIFS
> already does this for permission bits, but that's dumb too.
> 
> 2) Ownership matters. This patch will have no effect on how files get
> created. They still get created with the owner set to the user who's
> credentials were used, and you can't change them afterward (since users
> can't "chown" files to other users). Now, it is possible to mount a
> samba server with root's credentials. Then you can use the "setuids"
> mount option to make chown's work and you *might* get files created the
> way you intend. I really, really do not recommend this though -- it's a
> bad idea to allow any user to share root's server credentials.
> 
> I'm convinced, after working on it for more than 3 years that the *only*
> proper fix for the nightmare that is CIFS permissions is to make CIFS
> use proper credentials in the first place. CIFS is currently completely
> broken in this regard -- it's designed such that all accesses to the
> mount use the same set of credentials, and that's just plain wrong.
> 
> Fixing this entails establishing new SMB sessions on the fly whenever a
> user wants to do an on the wire operation. Obviously, we can't prompt
> for passwords for this, so we need to limit this to krb5 creds or come
> up with a way for people to stash credentials for the kernel to use for
> this purpose. I'm about 1/3 of the way into a first draft of a patchset
> to do this, but it won't be fixed anytime soon and may not be suitable
> for CIFS with SMB2 getting development focus now.
> 
> Now, all of that said...I don't have a specific issue with adding a
> patch to do this. I think this approach is completely ass-backwards and
> broken, but we do already attempt to enforce permission bits on the
> client currently. Adding checks for POSIX acl's don't make this (much)
> worse.
> 
> The one thing I don't think we want though is to turn this on by
> default since it means an extra round trip to the server every time
> you want to check permissions. This needs to be "switchable" somehow --
> possibly via mount option (maybe -o checkposixacls or something?)
> 
> > commit fa0b9415cda17b31966542101bc4ceb0c97c87cb
> > Author: Jon Severinsson <jon@severinsson.net>
> > Date:   Mon Mar 1 19:24:30 2010 +0100
> > 
> >     [CIFS] Adds support for permision checking vs. posix acl.
> >     
> >     CIFS already supports getting and setting acls through getfacl and setfacl, but
> >     prior to this patch, any acls was ignored when doing permission checking.
> > 
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 29f1da7..0605e11 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
> >  		on the client (above and beyond ACL on servers) for
> >  		servers which do not support setting and viewing mode bits,
> >  		so allowing client to check permissions is useful */
> > -		return generic_permission(inode, mask, NULL);
> > +		return generic_permission(inode, mask, inode->i_op->check_acl);
> >  }
> >  
> >  static struct kmem_cache *cifs_inode_cachep;
> > @@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > @@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > @@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> > index ac2b24c..6409a83 100644
> > --- a/fs/cifs/cifsfs.h
> > +++ b/fs/cifs/cifsfs.h
> > @@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
> >  			 int buflen);
> >  extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
> >  			const char *symname);
> > +
> > +/* Functions related to extended attributes */
> >  extern int	cifs_removexattr(struct dentry *, const char *);
> >  extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
> >  			size_t, int);
> >  extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
> >  extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
> > +extern int cifs_check_acl(struct inode *inode, int mask);
> > +
> >  extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
> >  
> >  #ifdef CONFIG_CIFS_EXPERIMENTAL
> > diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> > index a75afa3..a07633b 100644
> > --- a/fs/cifs/xattr.c
> > +++ b/fs/cifs/xattr.c
> > @@ -374,3 +374,71 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
> >  #endif
> >  	return rc;
> >  }
> > +
> > +int cifs_check_acl(struct inode *inode, int mask)
> > +{
> > +	int rc = -EOPNOTSUPP;
> > +#ifdef CONFIG_CIFS_XATTR
> > +#ifdef CONFIG_CIFS_POSIX
> > +	struct dentry *dentry;
> > +	size_t buf_size;
> > +	void *ea_value = NULL;
> > +	ssize_t ea_size;
> > +	struct posix_acl *acl = NULL;
> > +
> > +	/* CIFS gets acl from server by path, and thus needs a dentry rather than
> > +	   an inode. Note that the path of each dentry will point to the same inode
> > +	   on the backing fs at the server, so their acls will be the same, and it
> > +	   doesn't matter which one we pick, so just pick the fist. */
> 
> ^^^ comments should follow kernel coding styles. Be sure to run your
> patch through scripts/checkpatch.pl too.
> 
> > +	if (!list_empty(&inode->i_dentry))
> > +		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
> 
> 		^^^^ you should probably dget this dentry to take a
> 		reference to it and then put it when you're finished
> 		with it. It could vanish out of the cache before you
> 		have a chance to use it otherwise.
> 
> > +	else
> > +		return -EINVAL;
> > +
> > +	/* Try to fit the extended attribute corresponding to the posix acl in 4k
> > +	   memory. 4k was chosen because it always fits in a single page, and is
> > +	   the maximum on a default ext2/3/4 backing fs. */
> > +	buf_size = 4096;
> > +	ea_value = kmalloc(buf_size, GFP_KERNEL);
> > +	if (!ea_value) {
> > +		rc = -EAGAIN;
> > +		goto check_acl_exit;
> > +	}
> > +	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> > +
> > +	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
> > +	if (ea_size == -ERANGE) {
> > +		kfree(ea_value);
> > +		buf_size = 65536;
> > +		ea_value = kmalloc(buf_size, GFP_KERNEL);
> > +		if (!ea_value) {
> > +			rc = -EAGAIN;
> > +			goto check_acl_exit;
> > +		}
> > +		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> > +	}
> > +
> > +	/* If we didn't get any extended attribute, set the error code and exit */
> > +	if (ea_size <= 0) {
> > +		rc = -EAGAIN;
> > +		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
> > +			rc = ea_size;
> > +		goto check_acl_exit;
> > +	}
> > +
> > +	/* Set the appropriate return value. Adapted from ext4. */
> > +	acl = posix_acl_from_xattr(ea_value, ea_size);
> > +	if (IS_ERR(acl))
> > +		rc = PTR_ERR(acl);
> > +	else if (acl)
> > +		rc = posix_acl_permission(inode, acl, mask);
> > +	else
> > +		rc = -EAGAIN;
> > +
> > +check_acl_exit:
> > +	posix_acl_release(acl);
> > +	kfree(ea_value);
> > +#endif /* CONFIG_CIFS_POSIX */
> > +#endif /* CONFIG_CIFS_XATTR */
> > +	return rc;
> > +}
> 
> The rest of the patch looks reasonable but I agree with Simo that all
> of our time would be better spent working to make CIFS use proper
> credentials on the wire.
> 
> -- 
> Jeff Layton <jlayton@samba.org>
> --
> 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/

[-- Attachment #1.2: 0001-cifs-prototype-implementation-of-client-side-posix-a.patch --]
[-- Type: text/x-patch, Size: 3998 bytes --]

From 159de5f89c010dd2fe082d8b8109e5e3c8633462 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox@samba.org>
Date: Thu, 25 Feb 2010 17:06:03 +0100
Subject: [PATCH] cifs: prototype implementation of client-side posix acl checks.

---
 fs/cifs/cifsfs.c |  114 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 113 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 29f1da7..cb31ecd 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -47,6 +47,8 @@
 #include <linux/key-type.h>
 #include "dns_resolve.h"
 #include "cifs_spnego.h"
+#include <linux/posix_acl.h>
+#include <linux/posix_acl_xattr.h>
 #define CIFS_MAGIC_NUMBER 0xFF534D42	/* the first four bytes of SMB PDUs */
 
 #ifdef CONFIG_CIFS_QUOTA
@@ -269,6 +271,116 @@ cifs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	return 0;
 }
 
+static int cifs_check_acl(struct inode *inode, int mask)
+{
+	ssize_t rc = -EOPNOTSUPP;
+	struct cifs_sb_info *cifs_sb;
+	struct super_block *sb;
+	struct cifsTconInfo *pTcon;
+	int xid;
+	char *full_path;
+	void *ea_value = NULL;
+	size_t buf_size = 0;
+	struct dentry *dentry;
+	struct posix_acl *acl;
+
+printk(KERN_NOTICE "cifs_check_acl: entering\n");
+
+	sb = inode->i_sb;
+	if (sb == NULL) {
+		return -EIO;
+	}
+
+	cifs_sb = CIFS_SB(sb);
+	pTcon = cifs_sb->tcon;
+
+	xid = GetXid();
+
+	if (inode->i_dentry.next == NULL) {
+		/* no dentry found - deny access */
+		FreeXid(xid);
+		return -EACCES;
+	}
+
+	dentry = list_first_entry(&(inode->i_dentry), struct dentry, d_alias);
+	if (dentry == NULL) {
+		FreeXid(xid);
+		return -EACCES;
+	}
+
+printk(KERN_NOTICE "cifs_check_acl: got first dentry\n");
+
+	full_path = build_path_from_dentry(dentry);
+	if (full_path == NULL) {
+		rc = -ENOMEM;
+		FreeXid(xid);
+		return rc;
+	}
+
+printk(KERN_NOTICE "cifs_check_acl: got path '%s'\n", full_path);
+
+	if (!(sb->s_flags & MS_POSIXACL)) {
+		rc = -EOPNOTSUPP;
+		goto out;
+	}
+
+	ea_value = kmalloc(4096, GFP_KERNEL);
+	if (ea_value == NULL) {
+		rc = -ENOMEM;
+		goto out;
+	}
+	buf_size = (size_t)4096;
+
+printk(KERN_NOTICE "cifs_check_acl: allocated ea buffer (4096)\n");
+
+	rc = CIFSSMBGetPosixACL(xid, pTcon, full_path, ea_value,
+			buf_size, ACL_TYPE_ACCESS,
+			cifs_sb->local_nls,
+			cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
+	if (rc == -1) {
+printk(KERN_NOTICE "cifs_check_acl: error calling CIFSSMBGetPosixACL\n");
+		goto out;
+	}
+
+printk(KERN_NOTICE "cifs_check_ack: CIFSSMBGetPosixACL returned %d\n", rc);
+
+	acl = posix_acl_from_xattr(ea_value, rc);
+
+	if (acl == NULL) {
+printk(KERN_NOTICE "cifs_check_acl: error converting acl from ea buffer: NULL ACL\n");
+		rc = -EAGAIN;
+		goto out;
+	}
+
+	if (IS_ERR(acl)) {
+printk(KERN_NOTICE "cifs_check_acl: error converting acl from ea buffer: IS_ERR(acl): %d\n", -PTR_ERR(acl));
+		rc = PTR_ERR(acl);
+		goto out;
+	}
+
+	rc = posix_acl_valid(acl);
+	if (rc != 0) {
+printk(KERN_NOTICE "cifs_check_acl: acl is invalid\n");
+		goto out;
+	}
+
+	rc = posix_acl_permission(inode, acl, mask);
+	posix_acl_release(acl);
+
+printk(KERN_NOTICE "cifs_check_acl: posix_acl_permission check gave: %d\n", rc);
+
+out:
+	kfree(full_path);
+	FreeXid(xid);
+	if (ea_value != NULL) {
+		kfree(ea_value);
+	}
+
+printk(KERN_NOTICE "cifs_check_acl: done, returning %d\n", rc);
+
+	return rc;
+}
+
 static int cifs_permission(struct inode *inode, int mask)
 {
 	struct cifs_sb_info *cifs_sb;
@@ -284,7 +396,7 @@ static int cifs_permission(struct inode *inode, int mask)
 		on the client (above and beyond ACL on servers) for
 		servers which do not support setting and viewing mode bits,
 		so allowing client to check permissions is useful */
-		return generic_permission(inode, mask, NULL);
+		return generic_permission(inode, mask, cifs_check_acl);
 }
 
 static struct kmem_cache *cifs_inode_cachep;
-- 
1.6.3.3


[-- Attachment #2: Type: application/pgp-signature, Size: 206 bytes --]

Re: [RFC PATCH] CIFS posix acl permission checking

[Michael Adam]

[-- Attachment #1.1.1: Type: text/plain, Size: 11118 bytes --]

Hi,

yes, the coincidence is really funny!

I have been working on a similar patch, which I attach just for
reference. It is just in a proof-of-concept stage with some
printk calls for my personal debugging, and the code is not yet
conditional.

The main difference is that I used the CIFSSMBGetPosixACL call
instead of handcrafting the reading of the xtended attribute.

I have been chatting extensively chatting with Jeff about the
implementation of multi session/user mounts, which is definitely
the way to go but much more coding work.

I also noticed the problem of file ownership, which should
in principle be solvabel by useing the "setuids" mount option,
but this seemd to have no effect in my test setup with uml
linux running from a 2.6.32 git checkout.

Cheers - Michael

Jeff Layton wrote:
> On Thu, 4 Mar 2010 11:50:04 +0100
> Jon Severinsson <jon@severinsson.net> wrote:
> 
> > Hello
> > 
> > Early this weak I sent a patch implementing posix acl permission checking in 
> > the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
> > as I was unaware of the linux-cifs-client list. I later tried to submit it to 
> > linux-cifs-client as well, but my message seems to have been lost in the 
> > moderation queue, so I subscribed and am trying again.
> > 
> > I don't believe my patch is perfect, but I think it's a good start, and would 
> > like some comments from more experienced cifs developers to be able to get it 
> > into shape for inclusion in the kernel. 
> > 
> > I did get some comments from Matthew Wilcox at linux-fsdev, but unfortunately 
> > he never followed up on my response, so I'm including some unresolved 
> > questions I still have, as well as attaching the patch for further comments.
> > 
> > Best Regards
> > Jon Severinsson
> >
> 
> (cc'ing Michael Adam since he's been working on a similar patch)
> 
> I've looked over the patch and it's pretty good for a first attempt.
> There are a few minor problems with it, but it's a reasonable first
> pass.
> 
> The issues I have are with the approach -- you've stepped into a bit of
> a minefield with regards to CIFS' design. The issues that Simo brings up
> however are quite valid. Let me just state outright that permission
> checking on the client is a completely bogus concept. There are a
> couple of problems with this approach:
> 
> 1) Someone could change the permissions on the server between the time
> you check them and when you do your operation. Yes, I know that CIFS
> already does this for permission bits, but that's dumb too.
> 
> 2) Ownership matters. This patch will have no effect on how files get
> created. They still get created with the owner set to the user who's
> credentials were used, and you can't change them afterward (since users
> can't "chown" files to other users). Now, it is possible to mount a
> samba server with root's credentials. Then you can use the "setuids"
> mount option to make chown's work and you *might* get files created the
> way you intend. I really, really do not recommend this though -- it's a
> bad idea to allow any user to share root's server credentials.
> 
> I'm convinced, after working on it for more than 3 years that the *only*
> proper fix for the nightmare that is CIFS permissions is to make CIFS
> use proper credentials in the first place. CIFS is currently completely
> broken in this regard -- it's designed such that all accesses to the
> mount use the same set of credentials, and that's just plain wrong.
> 
> Fixing this entails establishing new SMB sessions on the fly whenever a
> user wants to do an on the wire operation. Obviously, we can't prompt
> for passwords for this, so we need to limit this to krb5 creds or come
> up with a way for people to stash credentials for the kernel to use for
> this purpose. I'm about 1/3 of the way into a first draft of a patchset
> to do this, but it won't be fixed anytime soon and may not be suitable
> for CIFS with SMB2 getting development focus now.
> 
> Now, all of that said...I don't have a specific issue with adding a
> patch to do this. I think this approach is completely ass-backwards and
> broken, but we do already attempt to enforce permission bits on the
> client currently. Adding checks for POSIX acl's don't make this (much)
> worse.
> 
> The one thing I don't think we want though is to turn this on by
> default since it means an extra round trip to the server every time
> you want to check permissions. This needs to be "switchable" somehow --
> possibly via mount option (maybe -o checkposixacls or something?)
> 
> > commit fa0b9415cda17b31966542101bc4ceb0c97c87cb
> > Author: Jon Severinsson <jon@severinsson.net>
> > Date:   Mon Mar 1 19:24:30 2010 +0100
> > 
> >     [CIFS] Adds support for permision checking vs. posix acl.
> >     
> >     CIFS already supports getting and setting acls through getfacl and setfacl, but
> >     prior to this patch, any acls was ignored when doing permission checking.
> > 
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 29f1da7..0605e11 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
> >  		on the client (above and beyond ACL on servers) for
> >  		servers which do not support setting and viewing mode bits,
> >  		so allowing client to check permissions is useful */
> > -		return generic_permission(inode, mask, NULL);
> > +		return generic_permission(inode, mask, inode->i_op->check_acl);
> >  }
> >  
> >  static struct kmem_cache *cifs_inode_cachep;
> > @@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > @@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > @@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> > index ac2b24c..6409a83 100644
> > --- a/fs/cifs/cifsfs.h
> > +++ b/fs/cifs/cifsfs.h
> > @@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
> >  			 int buflen);
> >  extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
> >  			const char *symname);
> > +
> > +/* Functions related to extended attributes */
> >  extern int	cifs_removexattr(struct dentry *, const char *);
> >  extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
> >  			size_t, int);
> >  extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
> >  extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
> > +extern int cifs_check_acl(struct inode *inode, int mask);
> > +
> >  extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
> >  
> >  #ifdef CONFIG_CIFS_EXPERIMENTAL
> > diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> > index a75afa3..a07633b 100644
> > --- a/fs/cifs/xattr.c
> > +++ b/fs/cifs/xattr.c
> > @@ -374,3 +374,71 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
> >  #endif
> >  	return rc;
> >  }
> > +
> > +int cifs_check_acl(struct inode *inode, int mask)
> > +{
> > +	int rc = -EOPNOTSUPP;
> > +#ifdef CONFIG_CIFS_XATTR
> > +#ifdef CONFIG_CIFS_POSIX
> > +	struct dentry *dentry;
> > +	size_t buf_size;
> > +	void *ea_value = NULL;
> > +	ssize_t ea_size;
> > +	struct posix_acl *acl = NULL;
> > +
> > +	/* CIFS gets acl from server by path, and thus needs a dentry rather than
> > +	   an inode. Note that the path of each dentry will point to the same inode
> > +	   on the backing fs at the server, so their acls will be the same, and it
> > +	   doesn't matter which one we pick, so just pick the fist. */
> 
> ^^^ comments should follow kernel coding styles. Be sure to run your
> patch through scripts/checkpatch.pl too.
> 
> > +	if (!list_empty(&inode->i_dentry))
> > +		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
> 
> 		^^^^ you should probably dget this dentry to take a
> 		reference to it and then put it when you're finished
> 		with it. It could vanish out of the cache before you
> 		have a chance to use it otherwise.
> 
> > +	else
> > +		return -EINVAL;
> > +
> > +	/* Try to fit the extended attribute corresponding to the posix acl in 4k
> > +	   memory. 4k was chosen because it always fits in a single page, and is
> > +	   the maximum on a default ext2/3/4 backing fs. */
> > +	buf_size = 4096;
> > +	ea_value = kmalloc(buf_size, GFP_KERNEL);
> > +	if (!ea_value) {
> > +		rc = -EAGAIN;
> > +		goto check_acl_exit;
> > +	}
> > +	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> > +
> > +	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
> > +	if (ea_size == -ERANGE) {
> > +		kfree(ea_value);
> > +		buf_size = 65536;
> > +		ea_value = kmalloc(buf_size, GFP_KERNEL);
> > +		if (!ea_value) {
> > +			rc = -EAGAIN;
> > +			goto check_acl_exit;
> > +		}
> > +		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> > +	}
> > +
> > +	/* If we didn't get any extended attribute, set the error code and exit */
> > +	if (ea_size <= 0) {
> > +		rc = -EAGAIN;
> > +		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
> > +			rc = ea_size;
> > +		goto check_acl_exit;
> > +	}
> > +
> > +	/* Set the appropriate return value. Adapted from ext4. */
> > +	acl = posix_acl_from_xattr(ea_value, ea_size);
> > +	if (IS_ERR(acl))
> > +		rc = PTR_ERR(acl);
> > +	else if (acl)
> > +		rc = posix_acl_permission(inode, acl, mask);
> > +	else
> > +		rc = -EAGAIN;
> > +
> > +check_acl_exit:
> > +	posix_acl_release(acl);
> > +	kfree(ea_value);
> > +#endif /* CONFIG_CIFS_POSIX */
> > +#endif /* CONFIG_CIFS_XATTR */
> > +	return rc;
> > +}
> 
> The rest of the patch looks reasonable but I agree with Simo that all
> of our time would be better spent working to make CIFS use proper
> credentials on the wire.
> 
> -- 
> Jeff Layton <jlayton@samba.org>
> --
> 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/

[-- Attachment #1.1.2: 0001-cifs-prototype-implementation-of-client-side-posix-a.patch --]
[-- Type: text/x-patch, Size: 3998 bytes --]

From 159de5f89c010dd2fe082d8b8109e5e3c8633462 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox@samba.org>
Date: Thu, 25 Feb 2010 17:06:03 +0100
Subject: [PATCH] cifs: prototype implementation of client-side posix acl checks.

---
 fs/cifs/cifsfs.c |  114 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 113 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 29f1da7..cb31ecd 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -47,6 +47,8 @@
 #include <linux/key-type.h>
 #include "dns_resolve.h"
 #include "cifs_spnego.h"
+#include <linux/posix_acl.h>
+#include <linux/posix_acl_xattr.h>
 #define CIFS_MAGIC_NUMBER 0xFF534D42	/* the first four bytes of SMB PDUs */
 
 #ifdef CONFIG_CIFS_QUOTA
@@ -269,6 +271,116 @@ cifs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	return 0;
 }
 
+static int cifs_check_acl(struct inode *inode, int mask)
+{
+	ssize_t rc = -EOPNOTSUPP;
+	struct cifs_sb_info *cifs_sb;
+	struct super_block *sb;
+	struct cifsTconInfo *pTcon;
+	int xid;
+	char *full_path;
+	void *ea_value = NULL;
+	size_t buf_size = 0;
+	struct dentry *dentry;
+	struct posix_acl *acl;
+
+printk(KERN_NOTICE "cifs_check_acl: entering\n");
+
+	sb = inode->i_sb;
+	if (sb == NULL) {
+		return -EIO;
+	}
+
+	cifs_sb = CIFS_SB(sb);
+	pTcon = cifs_sb->tcon;
+
+	xid = GetXid();
+
+	if (inode->i_dentry.next == NULL) {
+		/* no dentry found - deny access */
+		FreeXid(xid);
+		return -EACCES;
+	}
+
+	dentry = list_first_entry(&(inode->i_dentry), struct dentry, d_alias);
+	if (dentry == NULL) {
+		FreeXid(xid);
+		return -EACCES;
+	}
+
+printk(KERN_NOTICE "cifs_check_acl: got first dentry\n");
+
+	full_path = build_path_from_dentry(dentry);
+	if (full_path == NULL) {
+		rc = -ENOMEM;
+		FreeXid(xid);
+		return rc;
+	}
+
+printk(KERN_NOTICE "cifs_check_acl: got path '%s'\n", full_path);
+
+	if (!(sb->s_flags & MS_POSIXACL)) {
+		rc = -EOPNOTSUPP;
+		goto out;
+	}
+
+	ea_value = kmalloc(4096, GFP_KERNEL);
+	if (ea_value == NULL) {
+		rc = -ENOMEM;
+		goto out;
+	}
+	buf_size = (size_t)4096;
+
+printk(KERN_NOTICE "cifs_check_acl: allocated ea buffer (4096)\n");
+
+	rc = CIFSSMBGetPosixACL(xid, pTcon, full_path, ea_value,
+			buf_size, ACL_TYPE_ACCESS,
+			cifs_sb->local_nls,
+			cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
+	if (rc == -1) {
+printk(KERN_NOTICE "cifs_check_acl: error calling CIFSSMBGetPosixACL\n");
+		goto out;
+	}
+
+printk(KERN_NOTICE "cifs_check_ack: CIFSSMBGetPosixACL returned %d\n", rc);
+
+	acl = posix_acl_from_xattr(ea_value, rc);
+
+	if (acl == NULL) {
+printk(KERN_NOTICE "cifs_check_acl: error converting acl from ea buffer: NULL ACL\n");
+		rc = -EAGAIN;
+		goto out;
+	}
+
+	if (IS_ERR(acl)) {
+printk(KERN_NOTICE "cifs_check_acl: error converting acl from ea buffer: IS_ERR(acl): %d\n", -PTR_ERR(acl));
+		rc = PTR_ERR(acl);
+		goto out;
+	}
+
+	rc = posix_acl_valid(acl);
+	if (rc != 0) {
+printk(KERN_NOTICE "cifs_check_acl: acl is invalid\n");
+		goto out;
+	}
+
+	rc = posix_acl_permission(inode, acl, mask);
+	posix_acl_release(acl);
+
+printk(KERN_NOTICE "cifs_check_acl: posix_acl_permission check gave: %d\n", rc);
+
+out:
+	kfree(full_path);
+	FreeXid(xid);
+	if (ea_value != NULL) {
+		kfree(ea_value);
+	}
+
+printk(KERN_NOTICE "cifs_check_acl: done, returning %d\n", rc);
+
+	return rc;
+}
+
 static int cifs_permission(struct inode *inode, int mask)
 {
 	struct cifs_sb_info *cifs_sb;
@@ -284,7 +396,7 @@ static int cifs_permission(struct inode *inode, int mask)
 		on the client (above and beyond ACL on servers) for
 		servers which do not support setting and viewing mode bits,
 		so allowing client to check permissions is useful */
-		return generic_permission(inode, mask, NULL);
+		return generic_permission(inode, mask, cifs_check_acl);
 }
 
 static struct kmem_cache *cifs_inode_cachep;
-- 
1.6.3.3


[-- Attachment #1.2: Type: application/pgp-signature, Size: 206 bytes --]

[-- Attachment #2: Type: text/plain, Size: 172 bytes --]

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@lists.samba.org
https://lists.samba.org/mailman/listinfo/linux-cifs-client

Re: [linux-cifs-client] [RFC PATCH] CIFS posix acl permission checking

[Michael Adam]

[-- Attachment #1: Type: text/plain, Size: 10901 bytes --]

Jeff Layton wrote:
> On Thu, 4 Mar 2010 11:50:04 +0100
> Jon Severinsson <jon@severinsson.net> wrote:
> 
> > Hello
> > 
> > Early this weak I sent a patch implementing posix acl permission checking in 
> > the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
> > as I was unaware of the linux-cifs-client list. I later tried to submit it to 
> > linux-cifs-client as well, but my message seems to have been lost in the 
> > moderation queue, so I subscribed and am trying again.
> >
> > ...
> 
> (cc'ing Michael Adam since he's been working on a similar patch)
> 
> I've looked over the patch and it's pretty good for a first attempt.
> There are a few minor problems with it, but it's a reasonable first
> pass.
> 
> The issues I have are with the approach -- you've stepped into a bit of
> a minefield with regards to CIFS' design. The issues that Simo brings up
> however are quite valid. Let me just state outright that permission
> checking on the client is a completely bogus concept. There are a
> couple of problems with this approach:
> 
> 1) Someone could change the permissions on the server between the time
> you check them and when you do your operation. Yes, I know that CIFS
> already does this for permission bits, but that's dumb too.
> 
> 2) Ownership matters. This patch will have no effect on how files get
> created. They still get created with the owner set to the user who's
> credentials were used, and you can't change them afterward (since users
> can't "chown" files to other users). Now, it is possible to mount a
> samba server with root's credentials. Then you can use the "setuids"
> mount option to make chown's work and you *might* get files created the
> way you intend. I really, really do not recommend this though -- it's a
> bad idea to allow any user to share root's server credentials.

I completely agree that client side checking is bogus.
And as already mentioned in a different mail, the "setuids"
option seems to be broken currently (in my test cases at least).

> I'm convinced, after working on it for more than 3 years that the *only*
> proper fix for the nightmare that is CIFS permissions is to make CIFS
> use proper credentials in the first place. CIFS is currently completely
> broken in this regard -- it's designed such that all accesses to the
> mount use the same set of credentials, and that's just plain wrong.
> 
> Fixing this entails establishing new SMB sessions on the fly whenever a
> user wants to do an on the wire operation. Obviously, we can't prompt
> for passwords for this, so we need to limit this to krb5 creds or come
> up with a way for people to stash credentials for the kernel to use for
> this purpose.

So this method of "stashing" creds would usually involve some
kind of upcall to e.g. query the winbindd credential cache or
some static text file, right?

When discussing this with Volker today, he had a different idea:
One could implement a trans2 impersonate call in samba (as a new
call in the unix extensions) that could be used to transfer the
session established by the privileged user (root, say) to a
different user specified as an argument to the call -- without
the need to give credentials! Then this call could be used in
the multi user mount scenario: when uid 1000 accesse the cifs
mount then the root-dispatcher mount would create a new session
initially as root and issue an impersonate call to user 1000
directly afterwards.

Wouldn't that be something worth considering?

> I'm about 1/3 of the way into a first draft of a patchset
> to do this, but it won't be fixed anytime soon and may not be suitable
> for CIFS with SMB2 getting development focus now.

Sorry, what do you mean? Is SMB2 somehow contradictory to multi
session mounts?

Cheers - Michael

> Now, all of that said...I don't have a specific issue with adding a
> patch to do this. I think this approach is completely ass-backwards and
> broken, but we do already attempt to enforce permission bits on the
> client currently. Adding checks for POSIX acl's don't make this (much)
> worse.
> 
> The one thing I don't think we want though is to turn this on by
> default since it means an extra round trip to the server every time
> you want to check permissions. This needs to be "switchable" somehow --
> possibly via mount option (maybe -o checkposixacls or something?)
> 
> > commit fa0b9415cda17b31966542101bc4ceb0c97c87cb
> > Author: Jon Severinsson <jon@severinsson.net>
> > Date:   Mon Mar 1 19:24:30 2010 +0100
> > 
> >     [CIFS] Adds support for permision checking vs. posix acl.
> >     
> >     CIFS already supports getting and setting acls through getfacl and setfacl, but
> >     prior to this patch, any acls was ignored when doing permission checking.
> > 
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 29f1da7..0605e11 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
> >  		on the client (above and beyond ACL on servers) for
> >  		servers which do not support setting and viewing mode bits,
> >  		so allowing client to check permissions is useful */
> > -		return generic_permission(inode, mask, NULL);
> > +		return generic_permission(inode, mask, inode->i_op->check_acl);
> >  }
> >  
> >  static struct kmem_cache *cifs_inode_cachep;
> > @@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > @@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > @@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> > index ac2b24c..6409a83 100644
> > --- a/fs/cifs/cifsfs.h
> > +++ b/fs/cifs/cifsfs.h
> > @@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
> >  			 int buflen);
> >  extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
> >  			const char *symname);
> > +
> > +/* Functions related to extended attributes */
> >  extern int	cifs_removexattr(struct dentry *, const char *);
> >  extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
> >  			size_t, int);
> >  extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
> >  extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
> > +extern int cifs_check_acl(struct inode *inode, int mask);
> > +
> >  extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
> >  
> >  #ifdef CONFIG_CIFS_EXPERIMENTAL
> > diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> > index a75afa3..a07633b 100644
> > --- a/fs/cifs/xattr.c
> > +++ b/fs/cifs/xattr.c
> > @@ -374,3 +374,71 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
> >  #endif
> >  	return rc;
> >  }
> > +
> > +int cifs_check_acl(struct inode *inode, int mask)
> > +{
> > +	int rc = -EOPNOTSUPP;
> > +#ifdef CONFIG_CIFS_XATTR
> > +#ifdef CONFIG_CIFS_POSIX
> > +	struct dentry *dentry;
> > +	size_t buf_size;
> > +	void *ea_value = NULL;
> > +	ssize_t ea_size;
> > +	struct posix_acl *acl = NULL;
> > +
> > +	/* CIFS gets acl from server by path, and thus needs a dentry rather than
> > +	   an inode. Note that the path of each dentry will point to the same inode
> > +	   on the backing fs at the server, so their acls will be the same, and it
> > +	   doesn't matter which one we pick, so just pick the fist. */
> 
> ^^^ comments should follow kernel coding styles. Be sure to run your
> patch through scripts/checkpatch.pl too.
> 
> > +	if (!list_empty(&inode->i_dentry))
> > +		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
> 
> 		^^^^ you should probably dget this dentry to take a
> 		reference to it and then put it when you're finished
> 		with it. It could vanish out of the cache before you
> 		have a chance to use it otherwise.
> 
> > +	else
> > +		return -EINVAL;
> > +
> > +	/* Try to fit the extended attribute corresponding to the posix acl in 4k
> > +	   memory. 4k was chosen because it always fits in a single page, and is
> > +	   the maximum on a default ext2/3/4 backing fs. */
> > +	buf_size = 4096;
> > +	ea_value = kmalloc(buf_size, GFP_KERNEL);
> > +	if (!ea_value) {
> > +		rc = -EAGAIN;
> > +		goto check_acl_exit;
> > +	}
> > +	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> > +
> > +	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
> > +	if (ea_size == -ERANGE) {
> > +		kfree(ea_value);
> > +		buf_size = 65536;
> > +		ea_value = kmalloc(buf_size, GFP_KERNEL);
> > +		if (!ea_value) {
> > +			rc = -EAGAIN;
> > +			goto check_acl_exit;
> > +		}
> > +		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> > +	}
> > +
> > +	/* If we didn't get any extended attribute, set the error code and exit */
> > +	if (ea_size <= 0) {
> > +		rc = -EAGAIN;
> > +		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
> > +			rc = ea_size;
> > +		goto check_acl_exit;
> > +	}
> > +
> > +	/* Set the appropriate return value. Adapted from ext4. */
> > +	acl = posix_acl_from_xattr(ea_value, ea_size);
> > +	if (IS_ERR(acl))
> > +		rc = PTR_ERR(acl);
> > +	else if (acl)
> > +		rc = posix_acl_permission(inode, acl, mask);
> > +	else
> > +		rc = -EAGAIN;
> > +
> > +check_acl_exit:
> > +	posix_acl_release(acl);
> > +	kfree(ea_value);
> > +#endif /* CONFIG_CIFS_POSIX */
> > +#endif /* CONFIG_CIFS_XATTR */
> > +	return rc;
> > +}
> 
> The rest of the patch looks reasonable but I agree with Simo that all
> of our time would be better spent working to make CIFS use proper
> credentials on the wire.
> 
> -- 
> Jeff Layton <jlayton@samba.org>

-- 

i.A. Michael Adam

-- 
Michael Adam <ma@sernet.de>
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.SerNet.DE, mailto: Info @ SerNet.DE

[-- Attachment #2: Type: application/pgp-signature, Size: 206 bytes --]

Re: [RFC PATCH] CIFS posix acl permission checking

[Michael Adam]

[-- Attachment #1.1: Type: text/plain, Size: 10901 bytes --]

Jeff Layton wrote:
> On Thu, 4 Mar 2010 11:50:04 +0100
> Jon Severinsson <jon@severinsson.net> wrote:
> 
> > Hello
> > 
> > Early this weak I sent a patch implementing posix acl permission checking in 
> > the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
> > as I was unaware of the linux-cifs-client list. I later tried to submit it to 
> > linux-cifs-client as well, but my message seems to have been lost in the 
> > moderation queue, so I subscribed and am trying again.
> >
> > ...
> 
> (cc'ing Michael Adam since he's been working on a similar patch)
> 
> I've looked over the patch and it's pretty good for a first attempt.
> There are a few minor problems with it, but it's a reasonable first
> pass.
> 
> The issues I have are with the approach -- you've stepped into a bit of
> a minefield with regards to CIFS' design. The issues that Simo brings up
> however are quite valid. Let me just state outright that permission
> checking on the client is a completely bogus concept. There are a
> couple of problems with this approach:
> 
> 1) Someone could change the permissions on the server between the time
> you check them and when you do your operation. Yes, I know that CIFS
> already does this for permission bits, but that's dumb too.
> 
> 2) Ownership matters. This patch will have no effect on how files get
> created. They still get created with the owner set to the user who's
> credentials were used, and you can't change them afterward (since users
> can't "chown" files to other users). Now, it is possible to mount a
> samba server with root's credentials. Then you can use the "setuids"
> mount option to make chown's work and you *might* get files created the
> way you intend. I really, really do not recommend this though -- it's a
> bad idea to allow any user to share root's server credentials.

I completely agree that client side checking is bogus.
And as already mentioned in a different mail, the "setuids"
option seems to be broken currently (in my test cases at least).

> I'm convinced, after working on it for more than 3 years that the *only*
> proper fix for the nightmare that is CIFS permissions is to make CIFS
> use proper credentials in the first place. CIFS is currently completely
> broken in this regard -- it's designed such that all accesses to the
> mount use the same set of credentials, and that's just plain wrong.
> 
> Fixing this entails establishing new SMB sessions on the fly whenever a
> user wants to do an on the wire operation. Obviously, we can't prompt
> for passwords for this, so we need to limit this to krb5 creds or come
> up with a way for people to stash credentials for the kernel to use for
> this purpose.

So this method of "stashing" creds would usually involve some
kind of upcall to e.g. query the winbindd credential cache or
some static text file, right?

When discussing this with Volker today, he had a different idea:
One could implement a trans2 impersonate call in samba (as a new
call in the unix extensions) that could be used to transfer the
session established by the privileged user (root, say) to a
different user specified as an argument to the call -- without
the need to give credentials! Then this call could be used in
the multi user mount scenario: when uid 1000 accesse the cifs
mount then the root-dispatcher mount would create a new session
initially as root and issue an impersonate call to user 1000
directly afterwards.

Wouldn't that be something worth considering?

> I'm about 1/3 of the way into a first draft of a patchset
> to do this, but it won't be fixed anytime soon and may not be suitable
> for CIFS with SMB2 getting development focus now.

Sorry, what do you mean? Is SMB2 somehow contradictory to multi
session mounts?

Cheers - Michael

> Now, all of that said...I don't have a specific issue with adding a
> patch to do this. I think this approach is completely ass-backwards and
> broken, but we do already attempt to enforce permission bits on the
> client currently. Adding checks for POSIX acl's don't make this (much)
> worse.
> 
> The one thing I don't think we want though is to turn this on by
> default since it means an extra round trip to the server every time
> you want to check permissions. This needs to be "switchable" somehow --
> possibly via mount option (maybe -o checkposixacls or something?)
> 
> > commit fa0b9415cda17b31966542101bc4ceb0c97c87cb
> > Author: Jon Severinsson <jon@severinsson.net>
> > Date:   Mon Mar 1 19:24:30 2010 +0100
> > 
> >     [CIFS] Adds support for permision checking vs. posix acl.
> >     
> >     CIFS already supports getting and setting acls through getfacl and setfacl, but
> >     prior to this patch, any acls was ignored when doing permission checking.
> > 
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 29f1da7..0605e11 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
> >  		on the client (above and beyond ACL on servers) for
> >  		servers which do not support setting and viewing mode bits,
> >  		so allowing client to check permissions is useful */
> > -		return generic_permission(inode, mask, NULL);
> > +		return generic_permission(inode, mask, inode->i_op->check_acl);
> >  }
> >  
> >  static struct kmem_cache *cifs_inode_cachep;
> > @@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > @@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > @@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
> >  	.getxattr = cifs_getxattr,
> >  	.listxattr = cifs_listxattr,
> >  	.removexattr = cifs_removexattr,
> > +#ifdef CONFIG_CIFS_POSIX
> > +	.check_acl = cifs_check_acl,
> > +#endif
> >  #endif
> >  };
> >  
> > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> > index ac2b24c..6409a83 100644
> > --- a/fs/cifs/cifsfs.h
> > +++ b/fs/cifs/cifsfs.h
> > @@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
> >  			 int buflen);
> >  extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
> >  			const char *symname);
> > +
> > +/* Functions related to extended attributes */
> >  extern int	cifs_removexattr(struct dentry *, const char *);
> >  extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
> >  			size_t, int);
> >  extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
> >  extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
> > +extern int cifs_check_acl(struct inode *inode, int mask);
> > +
> >  extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
> >  
> >  #ifdef CONFIG_CIFS_EXPERIMENTAL
> > diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> > index a75afa3..a07633b 100644
> > --- a/fs/cifs/xattr.c
> > +++ b/fs/cifs/xattr.c
> > @@ -374,3 +374,71 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
> >  #endif
> >  	return rc;
> >  }
> > +
> > +int cifs_check_acl(struct inode *inode, int mask)
> > +{
> > +	int rc = -EOPNOTSUPP;
> > +#ifdef CONFIG_CIFS_XATTR
> > +#ifdef CONFIG_CIFS_POSIX
> > +	struct dentry *dentry;
> > +	size_t buf_size;
> > +	void *ea_value = NULL;
> > +	ssize_t ea_size;
> > +	struct posix_acl *acl = NULL;
> > +
> > +	/* CIFS gets acl from server by path, and thus needs a dentry rather than
> > +	   an inode. Note that the path of each dentry will point to the same inode
> > +	   on the backing fs at the server, so their acls will be the same, and it
> > +	   doesn't matter which one we pick, so just pick the fist. */
> 
> ^^^ comments should follow kernel coding styles. Be sure to run your
> patch through scripts/checkpatch.pl too.
> 
> > +	if (!list_empty(&inode->i_dentry))
> > +		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
> 
> 		^^^^ you should probably dget this dentry to take a
> 		reference to it and then put it when you're finished
> 		with it. It could vanish out of the cache before you
> 		have a chance to use it otherwise.
> 
> > +	else
> > +		return -EINVAL;
> > +
> > +	/* Try to fit the extended attribute corresponding to the posix acl in 4k
> > +	   memory. 4k was chosen because it always fits in a single page, and is
> > +	   the maximum on a default ext2/3/4 backing fs. */
> > +	buf_size = 4096;
> > +	ea_value = kmalloc(buf_size, GFP_KERNEL);
> > +	if (!ea_value) {
> > +		rc = -EAGAIN;
> > +		goto check_acl_exit;
> > +	}
> > +	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> > +
> > +	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
> > +	if (ea_size == -ERANGE) {
> > +		kfree(ea_value);
> > +		buf_size = 65536;
> > +		ea_value = kmalloc(buf_size, GFP_KERNEL);
> > +		if (!ea_value) {
> > +			rc = -EAGAIN;
> > +			goto check_acl_exit;
> > +		}
> > +		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> > +	}
> > +
> > +	/* If we didn't get any extended attribute, set the error code and exit */
> > +	if (ea_size <= 0) {
> > +		rc = -EAGAIN;
> > +		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
> > +			rc = ea_size;
> > +		goto check_acl_exit;
> > +	}
> > +
> > +	/* Set the appropriate return value. Adapted from ext4. */
> > +	acl = posix_acl_from_xattr(ea_value, ea_size);
> > +	if (IS_ERR(acl))
> > +		rc = PTR_ERR(acl);
> > +	else if (acl)
> > +		rc = posix_acl_permission(inode, acl, mask);
> > +	else
> > +		rc = -EAGAIN;
> > +
> > +check_acl_exit:
> > +	posix_acl_release(acl);
> > +	kfree(ea_value);
> > +#endif /* CONFIG_CIFS_POSIX */
> > +#endif /* CONFIG_CIFS_XATTR */
> > +	return rc;
> > +}
> 
> The rest of the patch looks reasonable but I agree with Simo that all
> of our time would be better spent working to make CIFS use proper
> credentials on the wire.
> 
> -- 
> Jeff Layton <jlayton@samba.org>

-- 

i.A. Michael Adam

-- 
Michael Adam <ma@sernet.de>
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.SerNet.DE, mailto: Info @ SerNet.DE

[-- Attachment #1.2: Type: application/pgp-signature, Size: 206 bytes --]

[-- Attachment #2: Type: text/plain, Size: 172 bytes --]

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@lists.samba.org
https://lists.samba.org/mailman/listinfo/linux-cifs-client

Re: [linux-cifs-client] [RFC PATCH] CIFS posix acl permission checking

[Jeff Layton]

On Thu, 11 Mar 2010 23:45:29 +0100
Michael Adam <obnox@samba.org> wrote:

> Jeff Layton wrote:
> > On Thu, 4 Mar 2010 11:50:04 +0100
> > Jon Severinsson <jon@severinsson.net> wrote:
> > 
> > > Hello
> > > 
> > > Early this weak I sent a patch implementing posix acl permission checking in 
> > > the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
> > > as I was unaware of the linux-cifs-client list. I later tried to submit it to 
> > > linux-cifs-client as well, but my message seems to have been lost in the 
> > > moderation queue, so I subscribed and am trying again.
> > >
> > > ...
> > 
> > (cc'ing Michael Adam since he's been working on a similar patch)
> > 
> > I've looked over the patch and it's pretty good for a first attempt.
> > There are a few minor problems with it, but it's a reasonable first
> > pass.
> > 
> > The issues I have are with the approach -- you've stepped into a bit of
> > a minefield with regards to CIFS' design. The issues that Simo brings up
> > however are quite valid. Let me just state outright that permission
> > checking on the client is a completely bogus concept. There are a
> > couple of problems with this approach:
> > 
> > 1) Someone could change the permissions on the server between the time
> > you check them and when you do your operation. Yes, I know that CIFS
> > already does this for permission bits, but that's dumb too.
> > 
> > 2) Ownership matters. This patch will have no effect on how files get
> > created. They still get created with the owner set to the user who's
> > credentials were used, and you can't change them afterward (since users
> > can't "chown" files to other users). Now, it is possible to mount a
> > samba server with root's credentials. Then you can use the "setuids"
> > mount option to make chown's work and you *might* get files created the
> > way you intend. I really, really do not recommend this though -- it's a
> > bad idea to allow any user to share root's server credentials.
> 
> I completely agree that client side checking is bogus.
> And as already mentioned in a different mail, the "setuids"
> option seems to be broken currently (in my test cases at least).
> 

Ok, some investigation as to the cause there might be helpful. That
said, the whole idea of mounting using root's creds seems a bit
dangerous to me. I can't point to a specific attack vector that employs
mounting with root creds, but it "smells" like bad practice.

> > I'm convinced, after working on it for more than 3 years that the *only*
> > proper fix for the nightmare that is CIFS permissions is to make CIFS
> > use proper credentials in the first place. CIFS is currently completely
> > broken in this regard -- it's designed such that all accesses to the
> > mount use the same set of credentials, and that's just plain wrong.
> > 
> > Fixing this entails establishing new SMB sessions on the fly whenever a
> > user wants to do an on the wire operation. Obviously, we can't prompt
> > for passwords for this, so we need to limit this to krb5 creds or come
> > up with a way for people to stash credentials for the kernel to use for
> > this purpose.
> 
> So this method of "stashing" creds would usually involve some
> kind of upcall to e.g. query the winbindd credential cache or
> some static text file, right?
> 

Actually, I was more envisioning using the kernel keyring to stash a
username, and password or NTLM hash. The kernel can then scoop those up
as needed without needing to upcall. You could do this per-user or
per-session too. Eventually we want to overhaul the krb5 code to do
less in userspace too.

See the keyctl_* manpages for details on that facility.

> When discussing this with Volker today, he had a different idea:
> One could implement a trans2 impersonate call in samba (as a new
> call in the unix extensions) that could be used to transfer the
> session established by the privileged user (root, say) to a
> different user specified as an argument to the call -- without
> the need to give credentials! Then this call could be used in
> the multi user mount scenario: when uid 1000 accesse the cifs
> mount then the root-dispatcher mount would create a new session
> initially as root and issue an impersonate call to user 1000
> directly afterwards.
> 
> Wouldn't that be something worth considering?
> 

I've only just read this, and haven't thought about it fully, but at
first glance that looks like the wrong way to go about fixing this. For
one thing, this is a samba only extension. For another, you're still
allowing "credential sharing" to some degree and that scheme encourages
people to mount using root's credentials.

The bottom line (IMO) is that any scheme that has multiple users
sharing credentials on a single SMB session is just a bad solution.
Credentials exist for a reason and really shouldn't be shared.

> > I'm about 1/3 of the way into a first draft of a patchset
> > to do this, but it won't be fixed anytime soon and may not be suitable
> > for CIFS with SMB2 getting development focus now.
> 
> Sorry, what do you mean? Is SMB2 somehow contradictory to multi
> session mounts?
> 

No, but the SMB2 code is a completely separate project however. What
there is of it today consists largely of code and functionality that is
cut-and-pasted from CIFS. Any fixes that gets implemented in CIFS
will need to be essentially duplicated for SMB2.

-- 
Jeff Layton <jlayton@samba.org>

Re: [RFC PATCH] CIFS posix acl permission checking

[Jeff Layton]

On Thu, 11 Mar 2010 23:45:29 +0100
Michael Adam <obnox@samba.org> wrote:

> Jeff Layton wrote:
> > On Thu, 4 Mar 2010 11:50:04 +0100
> > Jon Severinsson <jon@severinsson.net> wrote:
> > 
> > > Hello
> > > 
> > > Early this weak I sent a patch implementing posix acl permission checking in 
> > > the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
> > > as I was unaware of the linux-cifs-client list. I later tried to submit it to 
> > > linux-cifs-client as well, but my message seems to have been lost in the 
> > > moderation queue, so I subscribed and am trying again.
> > >
> > > ...
> > 
> > (cc'ing Michael Adam since he's been working on a similar patch)
> > 
> > I've looked over the patch and it's pretty good for a first attempt.
> > There are a few minor problems with it, but it's a reasonable first
> > pass.
> > 
> > The issues I have are with the approach -- you've stepped into a bit of
> > a minefield with regards to CIFS' design. The issues that Simo brings up
> > however are quite valid. Let me just state outright that permission
> > checking on the client is a completely bogus concept. There are a
> > couple of problems with this approach:
> > 
> > 1) Someone could change the permissions on the server between the time
> > you check them and when you do your operation. Yes, I know that CIFS
> > already does this for permission bits, but that's dumb too.
> > 
> > 2) Ownership matters. This patch will have no effect on how files get
> > created. They still get created with the owner set to the user who's
> > credentials were used, and you can't change them afterward (since users
> > can't "chown" files to other users). Now, it is possible to mount a
> > samba server with root's credentials. Then you can use the "setuids"
> > mount option to make chown's work and you *might* get files created the
> > way you intend. I really, really do not recommend this though -- it's a
> > bad idea to allow any user to share root's server credentials.
> 
> I completely agree that client side checking is bogus.
> And as already mentioned in a different mail, the "setuids"
> option seems to be broken currently (in my test cases at least).
> 

Ok, some investigation as to the cause there might be helpful. That
said, the whole idea of mounting using root's creds seems a bit
dangerous to me. I can't point to a specific attack vector that employs
mounting with root creds, but it "smells" like bad practice.

> > I'm convinced, after working on it for more than 3 years that the *only*
> > proper fix for the nightmare that is CIFS permissions is to make CIFS
> > use proper credentials in the first place. CIFS is currently completely
> > broken in this regard -- it's designed such that all accesses to the
> > mount use the same set of credentials, and that's just plain wrong.
> > 
> > Fixing this entails establishing new SMB sessions on the fly whenever a
> > user wants to do an on the wire operation. Obviously, we can't prompt
> > for passwords for this, so we need to limit this to krb5 creds or come
> > up with a way for people to stash credentials for the kernel to use for
> > this purpose.
> 
> So this method of "stashing" creds would usually involve some
> kind of upcall to e.g. query the winbindd credential cache or
> some static text file, right?
> 

Actually, I was more envisioning using the kernel keyring to stash a
username, and password or NTLM hash. The kernel can then scoop those up
as needed without needing to upcall. You could do this per-user or
per-session too. Eventually we want to overhaul the krb5 code to do
less in userspace too.

See the keyctl_* manpages for details on that facility.

> When discussing this with Volker today, he had a different idea:
> One could implement a trans2 impersonate call in samba (as a new
> call in the unix extensions) that could be used to transfer the
> session established by the privileged user (root, say) to a
> different user specified as an argument to the call -- without
> the need to give credentials! Then this call could be used in
> the multi user mount scenario: when uid 1000 accesse the cifs
> mount then the root-dispatcher mount would create a new session
> initially as root and issue an impersonate call to user 1000
> directly afterwards.
> 
> Wouldn't that be something worth considering?
> 

I've only just read this, and haven't thought about it fully, but at
first glance that looks like the wrong way to go about fixing this. For
one thing, this is a samba only extension. For another, you're still
allowing "credential sharing" to some degree and that scheme encourages
people to mount using root's credentials.

The bottom line (IMO) is that any scheme that has multiple users
sharing credentials on a single SMB session is just a bad solution.
Credentials exist for a reason and really shouldn't be shared.

> > I'm about 1/3 of the way into a first draft of a patchset
> > to do this, but it won't be fixed anytime soon and may not be suitable
> > for CIFS with SMB2 getting development focus now.
> 
> Sorry, what do you mean? Is SMB2 somehow contradictory to multi
> session mounts?
> 

No, but the SMB2 code is a completely separate project however. What
there is of it today consists largely of code and functionality that is
cut-and-pasted from CIFS. Any fixes that gets implemented in CIFS
will need to be essentially duplicated for SMB2.

-- 
Jeff Layton <jlayton@samba.org>

Re: [linux-cifs-client] [RFC PATCH] CIFS posix acl permission checking

[Jeremy Allison]

On Thu, Mar 11, 2010 at 11:45:29PM +0100, Michael Adam wrote:
> 
> When discussing this with Volker today, he had a different idea:
> One could implement a trans2 impersonate call in samba (as a new
> call in the unix extensions) that could be used to transfer the
> session established by the privileged user (root, say) to a
> different user specified as an argument to the call -- without
> the need to give credentials! Then this call could be used in
> the multi user mount scenario: when uid 1000 accesse the cifs
> mount then the root-dispatcher mount would create a new session
> initially as root and issue an impersonate call to user 1000
> directly afterwards.
> 
> Wouldn't that be something worth considering?

This world work, but protocol cleanliness-wise it's
*really* horrible :-).

Jeremy.

Re: [RFC PATCH] CIFS posix acl permission checking

[Jeremy Allison]

On Thu, Mar 11, 2010 at 11:45:29PM +0100, Michael Adam wrote:
> 
> When discussing this with Volker today, he had a different idea:
> One could implement a trans2 impersonate call in samba (as a new
> call in the unix extensions) that could be used to transfer the
> session established by the privileged user (root, say) to a
> different user specified as an argument to the call -- without
> the need to give credentials! Then this call could be used in
> the multi user mount scenario: when uid 1000 accesse the cifs
> mount then the root-dispatcher mount would create a new session
> initially as root and issue an impersonate call to user 1000
> directly afterwards.
> 
> Wouldn't that be something worth considering?

This world work, but protocol cleanliness-wise it's
*really* horrible :-).

Jeremy.

Re: [linux-cifs-client] [RFC PATCH] CIFS posix acl permission checking

[Michael Adam]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

Jeremy Allison wrote:
> On Thu, Mar 11, 2010 at 11:45:29PM +0100, Michael Adam wrote:
> > 
> > When discussing this with Volker today, he had a different idea:
> > One could implement a trans2 impersonate call in samba (as a new
> > call in the unix extensions) that could be used to transfer the
> > session established by the privileged user (root, say) to a
> > different user specified as an argument to the call -- without
> > the need to give credentials! Then this call could be used in
> > the multi user mount scenario: when uid 1000 accesse the cifs
> > mount then the root-dispatcher mount would create a new session
> > initially as root and issue an impersonate call to user 1000
> > directly afterwards.
> > 
> > Wouldn't that be something worth considering?
> 
> This world work, but protocol cleanliness-wise it's
> *really* horrible :-).

Agreed. :-)


[-- Attachment #2: Type: application/pgp-signature, Size: 206 bytes --]

Re: [RFC PATCH] CIFS posix acl permission checking

[Michael Adam]

[-- Attachment #1.1: Type: text/plain, Size: 890 bytes --]

Jeremy Allison wrote:
> On Thu, Mar 11, 2010 at 11:45:29PM +0100, Michael Adam wrote:
> > 
> > When discussing this with Volker today, he had a different idea:
> > One could implement a trans2 impersonate call in samba (as a new
> > call in the unix extensions) that could be used to transfer the
> > session established by the privileged user (root, say) to a
> > different user specified as an argument to the call -- without
> > the need to give credentials! Then this call could be used in
> > the multi user mount scenario: when uid 1000 accesse the cifs
> > mount then the root-dispatcher mount would create a new session
> > initially as root and issue an impersonate call to user 1000
> > directly afterwards.
> > 
> > Wouldn't that be something worth considering?
> 
> This world work, but protocol cleanliness-wise it's
> *really* horrible :-).

Agreed. :-)


[-- Attachment #1.2: Type: application/pgp-signature, Size: 206 bytes --]

[-- Attachment #2: Type: text/plain, Size: 172 bytes --]

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@lists.samba.org
https://lists.samba.org/mailman/listinfo/linux-cifs-client

Re: [RFC PATCH] CIFS posix acl permission checking

[Jon Severinsson]

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

On Monday 01 March 2010 18:03:49, Matthew Wilcox wrote:
> On Mon, Mar 01, 2010 at 04:15:10PM +0100, Jon Severinsson wrote:
>> Firstly, please note that I'm new to the kernel community, so if I'm
>> doing something wrong, just please politely point it out to me and I'll
>> try to fix it, or at the very least not do it again. Also please bear in
>> mind that English isn't my native language.
> 
> Okey dokey, here's some minor mistakes you made.

Thank you.

>  - Generally a good idea to cc the CIFS development list for CIFS patches.
>    I'm sure they'll notice it here, but just to be on the safe side, I've
>    added them to the cc.

Sure thing. I used linux-fsdevel as I didn't find anything more specific on 
vger.kernel.org, didn't think to look at lists.samba.org.

>  - You've included ifdefs around the check_acl entry in inode_operations,
>    *and* inside the definition of cifs_check_acl.  You only need to do one
>    or the other, and opinion is divided on which is better.

While I did recognize the redundancy, I decided to follow the same convention 
the other functions in xattr.c did, and include ifdefs at both locations.

I also considered the possible reasons for the existing functions to do both, 
and and came up with two reasons. The first simply being the paradigm of 
defensive programming, always check before doing a call, but never assume that 
the check has been done before being called. The second one is that of 
performance. The ifdefs has to be in cifs_check_acl to protect against other 
callers (while this patch doesn't introduce any, it doesn't prevent further 
patches from adding them), and not including the ifdefs in inode_operations 
would mean a completely useless function call on every permission check when a 
feature was turned off at compile time. The second one is a micro-optimization 
I 
don't really care fore, but defensive programming I do respect.

With this in mind, what do you recommend, double protection, breaking 
convention or changing the existing code?

>> --- a/fs/cifs/xattr.c
>> +++ b/fs/cifs/xattr.c
>> @@ -374,3 +374,74 @@ ssize_t cifs_listxattr(struct dentry *direntry, char
>> *data, size_t buf_size) #endif
>>  	return rc;
>>  }
>> +
>> +int cifs_check_acl(struct inode *inode, int mask)
>> +{
>> +	int rc = -EOPNOTSUPP;
>> +#ifdef CONFIG_CIFS_XATTR
>> +#ifdef CONFIG_CIFS_POSIX
>> +	struct dentry *dentry = NULL;
>> +	size_t buf_size;
>> +	void *ea_value = NULL;
>> +	ssize_t ea_size;
>> +	struct posix_acl *acl = NULL;
> 
> I don't think you need to initialise ea_value, do you?

While currently correct, I find it a good idea to immediately null any pointer 
that is freed in the exit section. Otherwise it is very easy to forget to do 
that the day someone adds a goto prior to the first assignment, and not nulling 
then can have unintended consequences in unrelated code. That being said, if 
you say that the kernel community frowns upon that kind of defensive 
programming I will definitely remove it.

With your suggested improvement below, there is no need to NULL dentry though, 
so I'll remove that.

>> +	/* CIFS gets acl from server by path, and thus needs a dentry rather 
>> than 
>> +	   an inode. Note that the path of each dentry will point to the
>> same inode 
>> +	   on the backing fs at the server, so their acls will be
>> the same, and it 
>> +	   doesn't matter which one we pick, so just pick the
>> fist. */ 
>> +	if (list_empty(&inode->i_dentry))
>> +		dentry = list_first_entry(&inode->i_dentry, struct dentry, 
>> d_alias);
>> +
>> +	/* If we didn't get an dentry for the inode, something went terribly
>> wrong. 
>> +	   All we can do at this point is to return an error. */
>> +	if (!dentry || IS_ERR(dentry))
>> +		return PTR_ERR(dentry);
> 
> I don't think dentry can possibly be IS_ERR here, and returning
> PTR_ERR(NULL) doesn't do what you think it does.  Also, you've got the
>  sense of list_empty backwards.  I think what you meant to do here was:
> 
> 	if (!list_empty(&inode->i_dentry))
> 		dentry = list_first_entry(&inode->i_dentry, struct dentry,
> 								d_alias);
> 	else
> 		return -EINVAL;

Sure, this makes more sense. Originally I used d_find_alias(inode), but later 
thought it was better to use the information in the inode struct directly. 
I never really understood how list.h lists was supposed to work, but stopped 
when cut-and-paste got it working for me. Probably a bad idea...

Attaching an updated patch, mostly for the convenience of anyone only 
following linux-cifs-client.

And thanks again for your input.

Best Regards
Jon Severinsson

[-- Attachment #2: cifs-acl-permission-check-v2.patch --]
[-- Type: text/x-patch, Size: 4882 bytes --]

commit fa0b9415cda17b31966542101bc4ceb0c97c87cb
Author: Jon Severinsson <jon@severinsson.net>
Date:   Mon Mar 1 19:24:30 2010 +0100

    [CIFS] Adds support for permision checking vs. posix acl.
    
    CIFS already supports getting and setting acls through getfacl and setfacl, but
    prior to this patch, any acls was ignored when doing permission checking.

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 29f1da7..0605e11 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
 		on the client (above and beyond ACL on servers) for
 		servers which do not support setting and viewing mode bits,
 		so allowing client to check permissions is useful */
-		return generic_permission(inode, mask, NULL);
+		return generic_permission(inode, mask, inode->i_op->check_acl);
 }
 
 static struct kmem_cache *cifs_inode_cachep;
@@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
@@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
@@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index ac2b24c..6409a83 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
 			 int buflen);
 extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
 			const char *symname);
+
+/* Functions related to extended attributes */
 extern int	cifs_removexattr(struct dentry *, const char *);
 extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
 			size_t, int);
 extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
 extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
+extern int cifs_check_acl(struct inode *inode, int mask);
+
 extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
 
 #ifdef CONFIG_CIFS_EXPERIMENTAL
diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
index a75afa3..a07633b 100644
--- a/fs/cifs/xattr.c
+++ b/fs/cifs/xattr.c
@@ -374,3 +374,71 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
 #endif
 	return rc;
 }
+
+int cifs_check_acl(struct inode *inode, int mask)
+{
+	int rc = -EOPNOTSUPP;
+#ifdef CONFIG_CIFS_XATTR
+#ifdef CONFIG_CIFS_POSIX
+	struct dentry *dentry;
+	size_t buf_size;
+	void *ea_value = NULL;
+	ssize_t ea_size;
+	struct posix_acl *acl = NULL;
+
+	/* CIFS gets acl from server by path, and thus needs a dentry rather than
+	   an inode. Note that the path of each dentry will point to the same inode
+	   on the backing fs at the server, so their acls will be the same, and it
+	   doesn't matter which one we pick, so just pick the fist. */
+	if (!list_empty(&inode->i_dentry))
+		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
+	else
+		return -EINVAL;
+
+	/* Try to fit the extended attribute corresponding to the posix acl in 4k
+	   memory. 4k was chosen because it always fits in a single page, and is
+	   the maximum on a default ext2/3/4 backing fs. */
+	buf_size = 4096;
+	ea_value = kmalloc(buf_size, GFP_KERNEL);
+	if (!ea_value) {
+		rc = -EAGAIN;
+		goto check_acl_exit;
+	}
+	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
+
+	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
+	if (ea_size == -ERANGE) {
+		kfree(ea_value);
+		buf_size = 65536;
+		ea_value = kmalloc(buf_size, GFP_KERNEL);
+		if (!ea_value) {
+			rc = -EAGAIN;
+			goto check_acl_exit;
+		}
+		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
+	}
+
+	/* If we didn't get any extended attribute, set the error code and exit */
+	if (ea_size <= 0) {
+		rc = -EAGAIN;
+		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
+			rc = ea_size;
+		goto check_acl_exit;
+	}
+
+	/* Set the appropriate return value. Adapted from ext4. */
+	acl = posix_acl_from_xattr(ea_value, ea_size);
+	if (IS_ERR(acl))
+		rc = PTR_ERR(acl);
+	else if (acl)
+		rc = posix_acl_permission(inode, acl, mask);
+	else
+		rc = -EAGAIN;
+
+check_acl_exit:
+	posix_acl_release(acl);
+	kfree(ea_value);
+#endif /* CONFIG_CIFS_POSIX */
+#endif /* CONFIG_CIFS_XATTR */
+	return rc;
+}

Re: [RFC PATCH] CIFS posix acl permission checking

[Matthew Wilcox]

On Mon, Mar 01, 2010 at 04:15:10PM +0100, Jon Severinsson wrote:
> Hello everyone
> 
> Firstly, please note that I'm new to the kernel community, so if I'm doing 
> something wrong, just please politely point it out to me and I'll try to fix 
> it, or at the very least not do it again. Also please bear in mind that 
> English isn't my native language.

Okey dokey, here's some minor mistakes you made.

 - Generally a good idea to cc the CIFS development list for CIFS patches.
   I'm sure they'll notice it here, but just to be on the safe side, I've
   added them to the cc.
 - You've included ifdefs around the check_acl entry in inode_operations,
   *and* inside the definition of cifs_check_acl.  You only need to do one
   or the other, and opinion is divided on which is better.
(further comments inline)

> Anyway, I recently realised that while the CIFS file system driver in Linux 
> does supports posix acl retrieval and modification using the getfacl and 
> setfacl command line tools, it does not use acl for client side permission 
> checking. On the server side Samba does consult the acl, but that doesn't 
> really help when cifs.ko never even asks the server, due to the users only 
> source of permission being from an acl.
> 
> I'm attaching a first attempt at implementing it. I have tested it, but only on 
> a single setup, so I can give no guarantees to its portability. Also please 
> note that this is my first kernel patch, so if I'm doing something wrong, such 
> as not following some coding standard, please enlighten me and I'll gladly fix 
> it.
> 
> I only subscribed to the linux-fsdevel list, so please include it in any 
> response you might send.
> 
> Best Regards
> Jon Severinsson

> commit fab1872fcbb8a5cdb85d7e7a88ecf0cad99d1c80
> Author: Jon Severinsson <jon@severinsson.net>
> Date:   Mon Mar 1 15:49:40 2010 +0100
> 
>     [CIFS] Adds support for permision checking vs. posix acl.
>     
>     CIFS already supports getting and setting acls through getfacl and setfacl, but
>     prior to this patch, any acls was ignored when doing permission checking.
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 29f1da7..0605e11 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
>  		on the client (above and beyond ACL on servers) for
>  		servers which do not support setting and viewing mode bits,
>  		so allowing client to check permissions is useful */
> -		return generic_permission(inode, mask, NULL);
> +		return generic_permission(inode, mask, inode->i_op->check_acl);
>  }
>  
>  static struct kmem_cache *cifs_inode_cachep;
> @@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
>  	.getxattr = cifs_getxattr,
>  	.listxattr = cifs_listxattr,
>  	.removexattr = cifs_removexattr,
> +#ifdef CONFIG_CIFS_POSIX
> +	.check_acl = cifs_check_acl,
> +#endif
>  #endif
>  };
>  
> @@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
>  	.getxattr = cifs_getxattr,
>  	.listxattr = cifs_listxattr,
>  	.removexattr = cifs_removexattr,
> +#ifdef CONFIG_CIFS_POSIX
> +	.check_acl = cifs_check_acl,
> +#endif
>  #endif
>  };
>  
> @@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
>  	.getxattr = cifs_getxattr,
>  	.listxattr = cifs_listxattr,
>  	.removexattr = cifs_removexattr,
> +#ifdef CONFIG_CIFS_POSIX
> +	.check_acl = cifs_check_acl,
> +#endif
>  #endif
>  };
>  
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index ac2b24c..6409a83 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
>  			 int buflen);
>  extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
>  			const char *symname);
> +
> +/* Functions related to extended attributes */
>  extern int	cifs_removexattr(struct dentry *, const char *);
>  extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
>  			size_t, int);
>  extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
>  extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
> +extern int cifs_check_acl(struct inode *inode, int mask);
> +
>  extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
>  
>  #ifdef CONFIG_CIFS_EXPERIMENTAL
> diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> index a75afa3..a851ba1 100644
> --- a/fs/cifs/xattr.c
> +++ b/fs/cifs/xattr.c
> @@ -374,3 +374,74 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
>  #endif
>  	return rc;
>  }
> +
> +int cifs_check_acl(struct inode *inode, int mask)
> +{
> +	int rc = -EOPNOTSUPP;
> +#ifdef CONFIG_CIFS_XATTR
> +#ifdef CONFIG_CIFS_POSIX
> +	struct dentry *dentry = NULL;
> +	size_t buf_size;
> +	void *ea_value = NULL;

I don't think you need to initialise ea_value, do you?

> +	ssize_t ea_size;
> +	struct posix_acl *acl = NULL;
> +
> +	/* CIFS gets acl from server by path, and thus needs a dentry rather than
> +	   an inode. Note that the path of each dentry will point to the same inode
> +	   on the backing fs at the server, so their acls will be the same, and it
> +	   doesn't matter which one we pick, so just pick the fist. */
> +	if (list_empty(&inode->i_dentry))
> +		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
> +
> +	/* If we didn't get an dentry for the inode, something went terribly wrong.
> +	   All we can do at this point is to return an error. */
> +	if (!dentry || IS_ERR(dentry)) 
> +		return PTR_ERR(dentry);

I don't think dentry can possibly be IS_ERR here, and returning
PTR_ERR(NULL) doesn't do what you think it does.  Also, you've got the sense
of list_empty backwards.  I think what you meant to do here was:

	if (!list_empty(&inode->i_dentry))
		dentry = list_first_entry(&inode->i_dentry, struct dentry,
								d_alias);
	else
		return -EINVAL;

> +	/* Try to fit the extended attribute corresponding to the posix acl in 4k
> +	   memory. 4k was chosen because it always fits in a single page, and is
> +	   the maximum on a default ext2/3/4 backing fs. */
> +	buf_size = 4096;
> +	ea_value = kmalloc(buf_size, GFP_KERNEL);
> +	if (!ea_value) {
> +		rc = -EAGAIN;
> +		goto check_acl_exit;
> +	}
> +	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> +
> +	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
> +	if (ea_size == -ERANGE) {
> +		kfree(ea_value);
> +		buf_size = 65536;
> +		ea_value = kmalloc(buf_size, GFP_KERNEL);
> +		if (!ea_value) {
> +			rc = -EAGAIN;
> +			goto check_acl_exit;
> +		}
> +		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
> +	}
> +
> +	/* If we didn't get any extended attribute, set the error code and exit */
> +	if (ea_size <= 0) {
> +		rc = -EAGAIN;
> +		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
> +			rc = ea_size;
> +		goto check_acl_exit;
> +	}
> +
> +	/* Set the appropriate return value. Adapted from ext4. */
> +	acl = posix_acl_from_xattr(ea_value, ea_size);
> +	if (IS_ERR(acl))
> +		rc = PTR_ERR(acl);
> +	else if (acl)
> +		rc = posix_acl_permission(inode, acl, mask);
> +	else
> +		rc = -EAGAIN;
> +
> +check_acl_exit:
> +	posix_acl_release(acl);
> +	kfree(ea_value);
> +#endif /* CONFIG_CIFS_POSIX */
> +#endif /* CONFIG_CIFS_XATTR */
> +	return rc;
> +}




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

[RFC PATCH] CIFS posix acl permission checking

[Jon Severinsson]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

Hello everyone

Firstly, please note that I'm new to the kernel community, so if I'm doing 
something wrong, just please politely point it out to me and I'll try to fix 
it, or at the very least not do it again. Also please bear in mind that 
English isn't my native language.

Anyway, I recently realised that while the CIFS file system driver in Linux 
does supports posix acl retrieval and modification using the getfacl and 
setfacl command line tools, it does not use acl for client side permission 
checking. On the server side Samba does consult the acl, but that doesn't 
really help when cifs.ko never even asks the server, due to the users only 
source of permission being from an acl.

I'm attaching a first attempt at implementing it. I have tested it, but only on 
a single setup, so I can give no guarantees to its portability. Also please 
note that this is my first kernel patch, so if I'm doing something wrong, such 
as not following some coding standard, please enlighten me and I'll gladly fix 
it.

I only subscribed to the linux-fsdevel list, so please include it in any 
response you might send.

Best Regards
Jon Severinsson

[-- Attachment #2: cifs-acl-permission-check-v1.patch --]
[-- Type: text/x-patch, Size: 5213 bytes --]

commit fab1872fcbb8a5cdb85d7e7a88ecf0cad99d1c80
Author: Jon Severinsson <jon@severinsson.net>
Date:   Mon Mar 1 15:49:40 2010 +0100

    [CIFS] Adds support for permision checking vs. posix acl.
    
    CIFS already supports getting and setting acls through getfacl and setfacl, but
    prior to this patch, any acls was ignored when doing permission checking.

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 29f1da7..0605e11 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
 		on the client (above and beyond ACL on servers) for
 		servers which do not support setting and viewing mode bits,
 		so allowing client to check permissions is useful */
-		return generic_permission(inode, mask, NULL);
+		return generic_permission(inode, mask, inode->i_op->check_acl);
 }
 
 static struct kmem_cache *cifs_inode_cachep;
@@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
@@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
@@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index ac2b24c..6409a83 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
 			 int buflen);
 extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
 			const char *symname);
+
+/* Functions related to extended attributes */
 extern int	cifs_removexattr(struct dentry *, const char *);
 extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
 			size_t, int);
 extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
 extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
+extern int cifs_check_acl(struct inode *inode, int mask);
+
 extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
 
 #ifdef CONFIG_CIFS_EXPERIMENTAL
diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
index a75afa3..a851ba1 100644
--- a/fs/cifs/xattr.c
+++ b/fs/cifs/xattr.c
@@ -374,3 +374,74 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
 #endif
 	return rc;
 }
+
+int cifs_check_acl(struct inode *inode, int mask)
+{
+	int rc = -EOPNOTSUPP;
+#ifdef CONFIG_CIFS_XATTR
+#ifdef CONFIG_CIFS_POSIX
+	struct dentry *dentry = NULL;
+	size_t buf_size;
+	void *ea_value = NULL;
+	ssize_t ea_size;
+	struct posix_acl *acl = NULL;
+
+	/* CIFS gets acl from server by path, and thus needs a dentry rather than
+	   an inode. Note that the path of each dentry will point to the same inode
+	   on the backing fs at the server, so their acls will be the same, and it
+	   doesn't matter which one we pick, so just pick the fist. */
+	if (list_empty(&inode->i_dentry))
+		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
+
+	/* If we didn't get an dentry for the inode, something went terribly wrong.
+	   All we can do at this point is to return an error. */
+	if (!dentry || IS_ERR(dentry)) 
+		return PTR_ERR(dentry);
+
+	/* Try to fit the extended attribute corresponding to the posix acl in 4k
+	   memory. 4k was chosen because it always fits in a single page, and is
+	   the maximum on a default ext2/3/4 backing fs. */
+	buf_size = 4096;
+	ea_value = kmalloc(buf_size, GFP_KERNEL);
+	if (!ea_value) {
+		rc = -EAGAIN;
+		goto check_acl_exit;
+	}
+	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
+
+	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
+	if (ea_size == -ERANGE) {
+		kfree(ea_value);
+		buf_size = 65536;
+		ea_value = kmalloc(buf_size, GFP_KERNEL);
+		if (!ea_value) {
+			rc = -EAGAIN;
+			goto check_acl_exit;
+		}
+		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
+	}
+
+	/* If we didn't get any extended attribute, set the error code and exit */
+	if (ea_size <= 0) {
+		rc = -EAGAIN;
+		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
+			rc = ea_size;
+		goto check_acl_exit;
+	}
+
+	/* Set the appropriate return value. Adapted from ext4. */
+	acl = posix_acl_from_xattr(ea_value, ea_size);
+	if (IS_ERR(acl))
+		rc = PTR_ERR(acl);
+	else if (acl)
+		rc = posix_acl_permission(inode, acl, mask);
+	else
+		rc = -EAGAIN;
+
+check_acl_exit:
+	posix_acl_release(acl);
+	kfree(ea_value);
+#endif /* CONFIG_CIFS_POSIX */
+#endif /* CONFIG_CIFS_XATTR */
+	return rc;
+}

[RFC PATCH] CIFS posix acl permission checking

[Jon Severinsson]

[-- Attachment #1.1: Type: text/plain, Size: 1174 bytes --]

Hello everyone

Firstly, please note that I'm new to the kernel community, so if I'm doing 
something wrong, just please politely point it out to me and I'll try to fix 
it, or at the very least not do it again. Also please bear in mind that 
English isn't my native language.

Anyway, I recently realised that while the CIFS file system driver in Linux 
does supports posix acl retrieval and modification using the getfacl and 
setfacl command line tools, it does not use acl for client side permission 
checking. On the server side Samba does consult the acl, but that doesn't 
really help when cifs.ko never even asks the server, due to the users only 
source of permission being from an acl.

I'm attaching a first attempt at implementing it. I have tested it, but only on 
a single setup, so I can give no guarantees to its portability. Also please 
note that this is my first kernel patch, so if I'm doing something wrong, such 
as not following some coding standard, please enlighten me and I'll gladly fix 
it.

I only subscribed to the linux-fsdevel list, so please include it in any 
response you might send.

Best Regards
Jon Severinsson

[-- Attachment #1.2: cifs-acl-permission-check-v1.patch --]
[-- Type: text/x-patch, Size: 5213 bytes --]

commit fab1872fcbb8a5cdb85d7e7a88ecf0cad99d1c80
Author: Jon Severinsson <jon@severinsson.net>
Date:   Mon Mar 1 15:49:40 2010 +0100

    [CIFS] Adds support for permision checking vs. posix acl.
    
    CIFS already supports getting and setting acls through getfacl and setfacl, but
    prior to this patch, any acls was ignored when doing permission checking.

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 29f1da7..0605e11 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -284,7 +284,7 @@ static int cifs_permission(struct inode *inode, int mask)
 		on the client (above and beyond ACL on servers) for
 		servers which do not support setting and viewing mode bits,
 		so allowing client to check permissions is useful */
-		return generic_permission(inode, mask, NULL);
+		return generic_permission(inode, mask, inode->i_op->check_acl);
 }
 
 static struct kmem_cache *cifs_inode_cachep;
@@ -702,6 +702,9 @@ const struct inode_operations cifs_dir_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
@@ -716,6 +719,9 @@ const struct inode_operations cifs_file_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
@@ -732,6 +738,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
 	.getxattr = cifs_getxattr,
 	.listxattr = cifs_listxattr,
 	.removexattr = cifs_removexattr,
+#ifdef CONFIG_CIFS_POSIX
+	.check_acl = cifs_check_acl,
+#endif
 #endif
 };
 
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index ac2b24c..6409a83 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -102,11 +102,15 @@ extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
 			 int buflen);
 extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
 			const char *symname);
+
+/* Functions related to extended attributes */
 extern int	cifs_removexattr(struct dentry *, const char *);
 extern int 	cifs_setxattr(struct dentry *, const char *, const void *,
 			size_t, int);
 extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
 extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
+extern int cifs_check_acl(struct inode *inode, int mask);
+
 extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
 
 #ifdef CONFIG_CIFS_EXPERIMENTAL
diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
index a75afa3..a851ba1 100644
--- a/fs/cifs/xattr.c
+++ b/fs/cifs/xattr.c
@@ -374,3 +374,74 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
 #endif
 	return rc;
 }
+
+int cifs_check_acl(struct inode *inode, int mask)
+{
+	int rc = -EOPNOTSUPP;
+#ifdef CONFIG_CIFS_XATTR
+#ifdef CONFIG_CIFS_POSIX
+	struct dentry *dentry = NULL;
+	size_t buf_size;
+	void *ea_value = NULL;
+	ssize_t ea_size;
+	struct posix_acl *acl = NULL;
+
+	/* CIFS gets acl from server by path, and thus needs a dentry rather than
+	   an inode. Note that the path of each dentry will point to the same inode
+	   on the backing fs at the server, so their acls will be the same, and it
+	   doesn't matter which one we pick, so just pick the fist. */
+	if (list_empty(&inode->i_dentry))
+		dentry = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
+
+	/* If we didn't get an dentry for the inode, something went terribly wrong.
+	   All we can do at this point is to return an error. */
+	if (!dentry || IS_ERR(dentry)) 
+		return PTR_ERR(dentry);
+
+	/* Try to fit the extended attribute corresponding to the posix acl in 4k
+	   memory. 4k was chosen because it always fits in a single page, and is
+	   the maximum on a default ext2/3/4 backing fs. */
+	buf_size = 4096;
+	ea_value = kmalloc(buf_size, GFP_KERNEL);
+	if (!ea_value) {
+		rc = -EAGAIN;
+		goto check_acl_exit;
+	}
+	ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
+
+	/* If 4k wasn't enough, try 64k, the maximum on any current backing fs. */
+	if (ea_size == -ERANGE) {
+		kfree(ea_value);
+		buf_size = 65536;
+		ea_value = kmalloc(buf_size, GFP_KERNEL);
+		if (!ea_value) {
+			rc = -EAGAIN;
+			goto check_acl_exit;
+		}
+		ea_size = cifs_getxattr(dentry, POSIX_ACL_XATTR_ACCESS, ea_value, buf_size);
+	}
+
+	/* If we didn't get any extended attribute, set the error code and exit */
+	if (ea_size <= 0) {
+		rc = -EAGAIN;
+		if (ea_size == -EOPNOTSUPP || ea_size == -EIO || ea_size == -ENOTDIR || ea_size == -ENOENT || ea_size == -EFAULT || ea_size == -EACCES)
+			rc = ea_size;
+		goto check_acl_exit;
+	}
+
+	/* Set the appropriate return value. Adapted from ext4. */
+	acl = posix_acl_from_xattr(ea_value, ea_size);
+	if (IS_ERR(acl))
+		rc = PTR_ERR(acl);
+	else if (acl)
+		rc = posix_acl_permission(inode, acl, mask);
+	else
+		rc = -EAGAIN;
+
+check_acl_exit:
+	posix_acl_release(acl);
+	kfree(ea_value);
+#endif /* CONFIG_CIFS_POSIX */
+#endif /* CONFIG_CIFS_XATTR */
+	return rc;
+}

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