Re: [PATCH] eCryptfs: fix permission denied with ecryptfs_xattr mount option when create readonly file

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



I noticed that this patch was ignored/forgotten. Sorry about that!

On 2018-08-21 16:17:40, robbieko wrote:
> From: Robbie Ko <robbieko@xxxxxxxxxxxx>
> 
> When the ecryptfs_xattr mount option is turned on, the ecryptfs
> metadata will be written to xattr via vfs_setxattr, which will
> check the WRITE permissions.
> 
> However, this will cause denial of permission when creating a
> file withoug write permission.
> 
> So fix this by calling __vfs_setxattr directly to skip permission
> check.
> 
> Signed-off-by: Robbie Ko <robbieko@xxxxxxxxxxxx>

This looks good to me. We're already calling __vfs_setxattr() when
updating the inode size when the metadata is stored in xattrs. I think
it makes sense to also do this when we're initializing the metadata.

I'm going to make one change, mentioned below, before I merge this
patch.

> ---
>  fs/ecryptfs/crypto.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index 4dd842f..ce4892b 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -37,6 +37,7 @@
>  #include <linux/slab.h>
>  #include <asm/unaligned.h>
>  #include <linux/kernel.h>
> +#include <linux/xattr.h>
>  #include "ecryptfs_kernel.h"
>  
>  #define DECRYPT		0
> @@ -1129,9 +1130,19 @@ static int ecryptfs_write_headers_virt(char *page_virt, size_t max,
>  				 char *page_virt, size_t size)
>  {
>  	int rc;
> +	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry);
> +	struct inode *lower_inode = d_inode(lower_dentry);
>  
> -	rc = ecryptfs_setxattr(ecryptfs_dentry, ecryptfs_inode,
> -			       ECRYPTFS_XATTR_NAME, page_virt, size, 0);
> +	if (!(lower_inode->i_opflags & IOP_XATTR)) {
> +		rc = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	inode_lock(lower_inode);
> +	rc = __vfs_setxattr(lower_dentry, lower_inode, ECRYPTFS_XATTR_NAME,
> +			    page_virt, size, 0);

On success, we need to copy up the inode attributes from the lower
inode. ecryptfs_setxattr() was doing that. The reason it is needed is
because the lower filesystem is likely to update, for example, the ctime
on xattr modification. I'll make this change before I push the patch to
my next branch.

Thanks for this patch!

Tyler

> +	inode_unlock(lower_inode);
> +out:
>  	return rc;
>  }
>  
> -- 
> 1.9.1
> 

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Crypto]     [Device Mapper Crypto]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux