[PATCH] ecryptfs: calling __vfs_setxattr_noperm() in ecryptfs_setxattr()

[PATCH] ecryptfs: calling __vfs_setxattr_noperm() in ecryptfs_setxattr()

[Roberto Sassu]

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

Ecryptfs is a stackable filesystem which relies on lower filesystems the ability 
of setting/getting extended attributes.
If there is a security module enabled on the system, it updates the 'security' field
of inodes according to the owned extended attribute set with the function vfs_setxattr().
When this function is performed on a ecryptfs filesystem the 'security' field is not 
updated for the lower filesystem since the call security_inode_post_setxattr() is 
missing for the lower inode.
This patch makes the function __vfs_setxattr_noperm() available for modules and 
replaces the call to the setxattr() method of the lower inode with the exported function.
The patch applies on the latest checked commit (a850ea30374eb) of the mainline kernel.


Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 fs/ecryptfs/inode.c |    5 +++--
 fs/xattr.c          |    1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 3fbc942..e17f65d 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -32,6 +32,7 @@
 #include <linux/crypto.h>
 #include <linux/fs_stack.h>
 #include <linux/slab.h>
+#include <linux/xattr.h>
 #include <asm/unaligned.h>
 #include "ecryptfs_kernel.h"
 
@@ -1109,8 +1110,8 @@ ecryptfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 		goto out;
 	}
 	mutex_lock(&lower_dentry->d_inode->i_mutex);
-	rc = lower_dentry->d_inode->i_op->setxattr(lower_dentry, name, value,
-						   size, flags);
+	rc = __vfs_setxattr_noperm(lower_dentry, name, value,
+				   size, flags);
 	mutex_unlock(&lower_dentry->d_inode->i_mutex);
 out:
 	return rc;
diff --git a/fs/xattr.c b/fs/xattr.c
index 01bb813..40f42af 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -106,6 +106,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
 
 	return error;
 }
+EXPORT_SYMBOL_GPL(__vfs_setxattr_noperm);
 
 
 int
-- 
1.7.2.3

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4707 bytes --]

Re: [patch 1/1] ecryptfs: call __vfs_setxattr_noperm() in ecryptfs_setxattr()

[Tyler Hicks]

On Fri Oct 01, 2010 at 02:14:00PM -0700, akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:
> From: Roberto Sassu <roberto.sassu@polito.it>

Andrew - thanks for not letting this one slip through.

> 
> Ecryptfs is a stackable filesystem which relies on lower filesystems the
> ability of setting/getting extended attributes.
> 
> If there is a security module enabled on the system it updates the
> 'security' field of inodes according to the owned extended attribute set
> with the function vfs_setxattr().  When this function is performed on a
> ecryptfs filesystem the 'security' field is not updated for the lower
> filesystem since the call security_inode_post_setxattr() is missing for
> the lower inode.
> 
> This patch makes the function __vfs_setxattr_noperm() available for
> modules and replaces the call to the setxattr() method of the lower inode
> with the exported function.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> Cc: Tyler Hicks <tyhicks@linux.vnet.ibm.com>
> Cc: Dustin Kirkland <kirkland@canonical.com>
> Cc: James Morris <jmorris@namei.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  fs/ecryptfs/inode.c |    5 +++--
>  fs/xattr.c          |    1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff -puN fs/ecryptfs/inode.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr fs/ecryptfs/inode.c
> --- a/fs/ecryptfs/inode.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr
> +++ a/fs/ecryptfs/inode.c
> @@ -32,6 +32,7 @@
>  #include <linux/crypto.h>
>  #include <linux/fs_stack.h>
>  #include <linux/slab.h>
> +#include <linux/xattr.h>
>  #include <asm/unaligned.h>
>  #include "ecryptfs_kernel.h"
> 
> @@ -1109,8 +1110,8 @@ ecryptfs_setxattr(struct dentry *dentry,
>  		goto out;
>  	}
>  	mutex_lock(&lower_dentry->d_inode->i_mutex);
> -	rc = lower_dentry->d_inode->i_op->setxattr(lower_dentry, name, value,
> -						   size, flags);
> +	rc = __vfs_setxattr_noperm(lower_dentry, name, value,
> +				   size, flags);

Hi Roberto - Thanks for the fix. However, it seems to me like we should
call vfs_setxattr(). We can't guarantee consistency among the eCryptfs
inode and the lower inode, so the extra call to xattr_permission()
isn't a waste.

>  	mutex_unlock(&lower_dentry->d_inode->i_mutex);
>  out:
>  	return rc;
> diff -puN fs/xattr.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr fs/xattr.c
> --- a/fs/xattr.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr
> +++ a/fs/xattr.c
> @@ -106,6 +106,7 @@ int __vfs_setxattr_noperm(struct dentry 
> 
>  	return error;
>  }
> +EXPORT_SYMBOL_GPL(__vfs_setxattr_noperm);
> 
> 
>  int
> _

Re: [patch 1/1] ecryptfs: call __vfs_setxattr_noperm() in ecryptfs_setxattr()

[Roberto Sassu]

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

On Tuesday, October 05, 2010 08:23:53 am Tyler Hicks wrote:
> On Fri Oct 01, 2010 at 02:14:00PM -0700, akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:
> > From: Roberto Sassu <roberto.sassu@polito.it>
> 
> Andrew - thanks for not letting this one slip through.
> 
> > 
> > Ecryptfs is a stackable filesystem which relies on lower filesystems the
> > ability of setting/getting extended attributes.
> > 
> > If there is a security module enabled on the system it updates the
> > 'security' field of inodes according to the owned extended attribute set
> > with the function vfs_setxattr().  When this function is performed on a
> > ecryptfs filesystem the 'security' field is not updated for the lower
> > filesystem since the call security_inode_post_setxattr() is missing for
> > the lower inode.
> > 
> > This patch makes the function __vfs_setxattr_noperm() available for
> > modules and replaces the call to the setxattr() method of the lower inode
> > with the exported function.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> > Cc: Tyler Hicks <tyhicks@linux.vnet.ibm.com>
> > Cc: Dustin Kirkland <kirkland@canonical.com>
> > Cc: James Morris <jmorris@namei.org>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > 
> >  fs/ecryptfs/inode.c |    5 +++--
> >  fs/xattr.c          |    1 +
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff -puN fs/ecryptfs/inode.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr fs/ecryptfs/inode.c
> > --- a/fs/ecryptfs/inode.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr
> > +++ a/fs/ecryptfs/inode.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/crypto.h>
> >  #include <linux/fs_stack.h>
> >  #include <linux/slab.h>
> > +#include <linux/xattr.h>
> >  #include <asm/unaligned.h>
> >  #include "ecryptfs_kernel.h"
> > 
> > @@ -1109,8 +1110,8 @@ ecryptfs_setxattr(struct dentry *dentry,
> >  		goto out;
> >  	}
> >  	mutex_lock(&lower_dentry->d_inode->i_mutex);
> > -	rc = lower_dentry->d_inode->i_op->setxattr(lower_dentry, name, value,
> > -						   size, flags);
> > +	rc = __vfs_setxattr_noperm(lower_dentry, name, value,
> > +				   size, flags);
> 
> Hi Roberto - Thanks for the fix. However, it seems to me like we should
> call vfs_setxattr(). We can't guarantee consistency among the eCryptfs
> inode and the lower inode, so the extra call to xattr_permission()
> isn't a waste.
> 

Hi Tyler

i look in deep at the code and i made the following considerations:
first, i think calling vfs_setxattr() instead of __vfs_setxattr is not necessary when ecryptfs is 
mounted on the top of a directory in the lower filesystem: in this case the only way
to modify the extended attributes of lower inodes is to call ecryptfs_setxattr().
Second, the use of vfs_setxattr() causes functions xattr_permission() and 
security_inode_setxattr() to be called twice. While the former is not needed the second time
because it calls ecryptfs_permission() which in turn invokes inode_permission() 
on the lower inode, i noted the second is required at least for SELinux in this situation:
suppose that ecryptfs and the lower filesystem have different security contexts for the 
filesystem object; in this case if the security_inode_setxattr() is missing for the lower inode,
the filesystem 'associate' permission is not checked (SELinux verifies if an inode with a 
given label can be stored on a filesystem bound to a specific security context).
I will post a second version of the patch which has the function __vfs_setxattr() replaced with
vfs_setxattr().


> >  	mutex_unlock(&lower_dentry->d_inode->i_mutex);
> >  out:
> >  	return rc;
> > diff -puN fs/xattr.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr fs/xattr.c
> > --- a/fs/xattr.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr
> > +++ a/fs/xattr.c
> > @@ -106,6 +106,7 @@ int __vfs_setxattr_noperm(struct dentry 
> > 
> >  	return error;
> >  }
> > +EXPORT_SYMBOL_GPL(__vfs_setxattr_noperm);
> > 
> > 
> >  int
> > _
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4707 bytes --]