[PATCH] ecryptfs: Fix stat command displays wrong file size in xattr region

[PATCH] ecryptfs: Fix stat command displays wrong file size in xattr region

[Jason Xing]

When doing ecryptfs_read_and_validate_xattr_region(), eCryptfs
reads only 16 bytes from xattr region. However, the lower filesystem
like ext4 always compares 16 with the size of ecryptfs xattr region
which is 81 bytes, and then it will return ERANGE (-34) and do not
read that region.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 fs/ecryptfs/crypto.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index e5e29f8..895140f 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -1389,19 +1389,36 @@ int ecryptfs_read_xattr_region(char *page_virt, struct inode *ecryptfs_inode)
 int ecryptfs_read_and_validate_xattr_region(struct dentry *dentry,
 					    struct inode *inode)
 {
-	u8 file_size[ECRYPTFS_SIZE_AND_MARKER_BYTES];
-	u8 *marker = file_size + ECRYPTFS_FILE_SIZE_BYTES;
+	char *page_virt;
 	int rc;
 
+	page_virt = kmem_cache_alloc(ecryptfs_header_cache, GFP_USER);
+	if (!page_virt) {
+		rc = -ENOMEM;
+		printk(KERN_ERR "%s: Unable to allocate page_virt\n",
+		       __func__);
+		goto out;
+	}
 	rc = ecryptfs_getxattr_lower(ecryptfs_dentry_to_lower(dentry),
 				     ecryptfs_inode_to_lower(inode),
-				     ECRYPTFS_XATTR_NAME, file_size,
-				     ECRYPTFS_SIZE_AND_MARKER_BYTES);
-	if (rc < ECRYPTFS_SIZE_AND_MARKER_BYTES)
-		return rc >= 0 ? -EINVAL : rc;
-	rc = ecryptfs_validate_marker(marker);
+				     ECRYPTFS_XATTR_NAME, page_virt,
+				     ECRYPTFS_DEFAULT_EXTENT_SIZE);
+	if (rc < 0) {
+		if (unlikely(ecryptfs_verbosity > 0))
+			printk(KERN_INFO "Error attempting to read the [%s] "
+			       "xattr from the lower file; return value = "
+			       "[%d]\n", ECRYPTFS_XATTR_NAME, rc);
+		rc = -EINVAL;
+		goto out;
+	}
+	rc = ecryptfs_validate_marker(page_virt + ECRYPTFS_FILE_SIZE_BYTES);
 	if (!rc)
-		ecryptfs_i_size_init(file_size, inode);
+		ecryptfs_i_size_init(page_virt, inode);
+out:
+	if (page_virt) {
+		memset(page_virt, 0, PAGE_SIZE);
+		kmem_cache_free(ecryptfs_header_cache, page_virt);
+	}
 	return rc;
 }
 
-- 
2.7.4

Re: [PATCH] ecryptfs: Fix stat command displays wrong file size in xattr region

[Jason Xing]

Could anyone take a look at this patch which fixes the xattr-read
issue? Thanks anyway.

Jason

On Thu, Jun 22, 2017 at 3:21 PM, Jason Xing <kerneljasonxing@gmail.com> wrote:
> When doing ecryptfs_read_and_validate_xattr_region(), eCryptfs
> reads only 16 bytes from xattr region. However, the lower filesystem
> like ext4 always compares 16 with the size of ecryptfs xattr region
> which is 81 bytes, and then it will return ERANGE (-34) and do not
> read that region.
>
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>  fs/ecryptfs/crypto.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index e5e29f8..895140f 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -1389,19 +1389,36 @@ int ecryptfs_read_xattr_region(char *page_virt, struct inode *ecryptfs_inode)
>  int ecryptfs_read_and_validate_xattr_region(struct dentry *dentry,
>                                             struct inode *inode)
>  {
> -       u8 file_size[ECRYPTFS_SIZE_AND_MARKER_BYTES];
> -       u8 *marker = file_size + ECRYPTFS_FILE_SIZE_BYTES;
> +       char *page_virt;
>         int rc;
>
> +       page_virt = kmem_cache_alloc(ecryptfs_header_cache, GFP_USER);
> +       if (!page_virt) {
> +               rc = -ENOMEM;
> +               printk(KERN_ERR "%s: Unable to allocate page_virt\n",
> +                      __func__);
> +               goto out;
> +       }
>         rc = ecryptfs_getxattr_lower(ecryptfs_dentry_to_lower(dentry),
>                                      ecryptfs_inode_to_lower(inode),
> -                                    ECRYPTFS_XATTR_NAME, file_size,
> -                                    ECRYPTFS_SIZE_AND_MARKER_BYTES);
> -       if (rc < ECRYPTFS_SIZE_AND_MARKER_BYTES)
> -               return rc >= 0 ? -EINVAL : rc;
> -       rc = ecryptfs_validate_marker(marker);
> +                                    ECRYPTFS_XATTR_NAME, page_virt,
> +                                    ECRYPTFS_DEFAULT_EXTENT_SIZE);
> +       if (rc < 0) {
> +               if (unlikely(ecryptfs_verbosity > 0))
> +                       printk(KERN_INFO "Error attempting to read the [%s] "
> +                              "xattr from the lower file; return value = "
> +                              "[%d]\n", ECRYPTFS_XATTR_NAME, rc);
> +               rc = -EINVAL;
> +               goto out;
> +       }
> +       rc = ecryptfs_validate_marker(page_virt + ECRYPTFS_FILE_SIZE_BYTES);
>         if (!rc)
> -               ecryptfs_i_size_init(file_size, inode);
> +               ecryptfs_i_size_init(page_virt, inode);
> +out:
> +       if (page_virt) {
> +               memset(page_virt, 0, PAGE_SIZE);
> +               kmem_cache_free(ecryptfs_header_cache, page_virt);
> +       }
>         return rc;
>  }
>
> --
> 2.7.4
>

Re: [PATCH] ecryptfs: Fix stat command displays wrong file size in xattr region

[Tyler Hicks]

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

Hi Jason! My apologies for not getting to this review earlier. I haven't
had much time to spend on eCryptfs lately.

Thanks for this bug fix! This patch looks mostly good but I have a
suggestion and question below.

On 06/22/2017 02:21 AM, Jason Xing wrote:
> When doing ecryptfs_read_and_validate_xattr_region(), eCryptfs
> reads only 16 bytes from xattr region. However, the lower filesystem
> like ext4 always compares 16 with the size of ecryptfs xattr region
> which is 81 bytes, and then it will return ERANGE (-34) and do not
> read that region.
> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>  fs/ecryptfs/crypto.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index e5e29f8..895140f 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -1389,19 +1389,36 @@ int ecryptfs_read_xattr_region(char *page_virt, struct inode *ecryptfs_inode)
>  int ecryptfs_read_and_validate_xattr_region(struct dentry *dentry,
>  					    struct inode *inode)
>  {
> -	u8 file_size[ECRYPTFS_SIZE_AND_MARKER_BYTES];
> -	u8 *marker = file_size + ECRYPTFS_FILE_SIZE_BYTES;
> +	char *page_virt;
>  	int rc;
>  
> +	page_virt = kmem_cache_alloc(ecryptfs_header_cache, GFP_USER);
> +	if (!page_virt) {
> +		rc = -ENOMEM;
> +		printk(KERN_ERR "%s: Unable to allocate page_virt\n",
> +		       __func__);
> +		goto out;
> +	}
>  	rc = ecryptfs_getxattr_lower(ecryptfs_dentry_to_lower(dentry),
>  				     ecryptfs_inode_to_lower(inode),
> -				     ECRYPTFS_XATTR_NAME, file_size,
> -				     ECRYPTFS_SIZE_AND_MARKER_BYTES);
> -	if (rc < ECRYPTFS_SIZE_AND_MARKER_BYTES)
> -		return rc >= 0 ? -EINVAL : rc;
> -	rc = ecryptfs_validate_marker(marker);
> +				     ECRYPTFS_XATTR_NAME, page_virt,
> +				     ECRYPTFS_DEFAULT_EXTENT_SIZE);

I worry that the ecryptfs_header_cache size and
ECRYPTFS_DEFAULT_EXTENT_SIZE aren't tied together in any way. What do
you think about using ecryptfs_header_cache->size instead of
ECRYPTFS_DEFAULT_EXTENT_SIZE here?

I think you probably copied this call to ecryptfs_getxattr_lower() from
elsewhere in fs/ecryptfs/* so I wouldn't expect you to have use
ecryptfs_header_cache->size but maybe we should correct it before making
the same mistake twice.

> +	if (rc < 0) {
> +		if (unlikely(ecryptfs_verbosity > 0))
> +			printk(KERN_INFO "Error attempting to read the [%s] "
> +			       "xattr from the lower file; return value = "
> +			       "[%d]\n", ECRYPTFS_XATTR_NAME, rc);
> +		rc = -EINVAL;

Why not pass rc up from ecryptfs_getxattr_lower() instead of masking its
error with -EINVAL?

> +		goto out;
> +	}

Lets make sure that rc is greater than or equal to
ECRYPTFS_FILE_SIZE_AND_MARKER_BYTES before proceeding.

> +	rc = ecryptfs_validate_marker(page_virt + ECRYPTFS_FILE_SIZE_BYTES);
>  	if (!rc)
> -		ecryptfs_i_size_init(file_size, inode);
> +		ecryptfs_i_size_init(page_virt, inode);
> +out:
> +	if (page_virt) {
> +		memset(page_virt, 0, PAGE_SIZE);

I don't feel like a memset() is needed here since the header metadata is
not considered to be secret in itself. All secrets are encrypted.

Tyler

> +		kmem_cache_free(ecryptfs_header_cache, page_virt);
> +	}
>  	return rc;
>  }
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]