linux-kernel - Re: [patch 1/1] ecryptfs: call __vfs_setxattr_noperm() in ecryptfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101005062353.GA24737@boomer>
Date:	Tue, 5 Oct 2010 01:23:53 -0500
From:	Tyler Hicks <tyhicks@...ux.vnet.ibm.com>
To:	roberto.sassu@...ito.it
Cc:	kirkland@...onical.com, jmorris@...ei.org,
	akpm@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [patch 1/1] ecryptfs: call __vfs_setxattr_noperm() in
 ecryptfs_setxattr()

On Fri Oct 01, 2010 at 02:14:00PM -0700, akpm@...ux-foundation.org <akpm@...ux-foundation.org> wrote:
> From: Roberto Sassu <roberto.sassu@...ito.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@...ito.it>
> Cc: Tyler Hicks <tyhicks@...ux.vnet.ibm.com>
> Cc: Dustin Kirkland <kirkland@...onical.com>
> Cc: James Morris <jmorris@...ei.org>
> Signed-off-by: Andrew Morton <akpm@...ux-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
> _
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/