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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
> }
>
>
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 <[email protected]> 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 <[email protected]>
> ---
> 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
>
From 1570888797362860728@xxx Thu Jun 22 07:24:59 +0000 2017
X-GM-THRID: 1570888797362860728
X-Gmail-Labels: Inbox,Category Forums