On Sun Dec 06, 2009 at 05:31:58PM -0500, Erez Zadok (ezk@xxxxxxxxxxxxx) was quoted:
> Tyler/Dustin,
>
> We came across several related bugs while trying to use the -o
> ecryptfs_xattr option while mounting the ecryptfs (in 2.6.31.latest and
> 2.6.32) which saves its metadata information in extended attributes of the
> lower file system.
Doesn't surprise me. This option doesn't seem to be implemented very
well and has been the cause of several headaches. I rarely run across
anyone actually using it except for the occasional user just trying it
out.
> These bugs discovered and fixed by Saumitra Bhanage
> while using ecryptfs for a graduate class project. We've reviewed the code
> and we're reasonably certain of our fixes below; it'll probably make sense
> for someone else to give this patch a careful review.
I appreciate the feedback!
>
> The first problem was running out of memory, and was reproduced as follows:
>
> # mount -t ecryptfs -o ecryptfs_xattr /root/ecryptfs_base/ /mnt/test_mount/
> ...
> Attempting to mount with the following options:
> ecryptfs_xattr_metadata
> ecryptfs_key_bytes=16
> ecryptfs_cipher=aes
> ecryptfs_sig=633937dbcf1fef34
> Mounted eCryptfs
> # touch /mnt/test_mount/file1
> touch: cannot touch `/mnt/test_mount/file1': Cannot allocate memory
>
> Problem: ecryptfs_set_default_sizes() hard-codes the header size to 0 if the
> xattr option is enabled.
This is the correct behavior. There is an important distinction between
header size (size reserved for eCryptfs metadata at the beginning of the
file) and metadata size (size reserved for eCryptfs metadata, regardless
of where it is stored). crypt_stat->num_header_bytes_at_front is
specifically for header size, so it should be 0 when the
ecryptfs_xattr_metadata option is used.
> That size is used to decide how much memory to
> allocate to hold the header. When you set the num_header_bytes_at_front to
> zero, later on ecryptfs_write_metadata will call get_order(0), and
> get_order(0) returns the value 20 (if you follow the math there). Next, it
> calls ecryptfs_get_zeroed_pages(GFP_KERNEL, 20), which tries (and fails with
> ENOMEM) to allocate 2^20 pages.
This is the incorrect behavior. We should be conditionalizing the
assignment of virt_len in ecryptfs_write_metadata() around the
ECRYPTFS_METADATA_IN_XATTR bit being set in crypt_stat->flag.
>
> Solution: set the header size to ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE. This
> is the fix to crypto.c:ecryptfs_set_default_sizes.
Hmm... is ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE (8192) what we should be
using when writing the headers? It isn't clear as
ecryptfs_read_xattr_region() only reads in ECRYPTFS_DEFAULT_EXTENT_SIZE
(4096) from the lower xattr. I tend to think you're right in using
ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE and maybe
ecryptfs_read_xattr_region() should be fixed.
>
> Fixing the above revealed a new problem. Now, if you fix the header size to
> ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE, then now the code in
> ecryptfs_lower_offset_for_extent was wrong: it used to assume that the
> header size was the default 8k. Now, if we use xattrs, then the
> lower_offset computed should exclude the current header size. That is the
> patch to crypto.c:ecryptfs_lower_offset_for_extent.
>
> Finally, we've discovered one more place where the header size was
> incorrectly computed. When truncating a file, ecryptfs will try to compute
> the actual lower truncation point by adding the header size. The code
> always assumed that there was a physical in-file header of size
> num_header_bytes_at_front. But when using the xattr option, this needs to
> be set to zero. This is the patch to fix
> inode.c:upper_size_to_lower_size().
I don't think these last 2 problems will pop up if we leave
num_header_bytes_at_front as 0 when using ecryptfs_xatter_metadata and
making the change I mentioned in ecryptfs_write_metadata().
>
> Cheers,
> Erez.
>
> ------------------------------------------------------------------------------
>
> ecryptfs: properly compute header sizes/offsets when storing headers in xattrs
>
> Ecryptfs can store its meta-data in a header which can be either an 8k chunk
> at the front of the actual ciphertext file, or stored in extended
> attributes. When using this ecryptfs_xattr option, the header sizes and
> file offsets are computed incorrectly, resulting in ENOMEM errors, wasted
> header spaces, or truncating a file at the wrong offset.
>
> Signed-off-by: Saumitra Bhanage <sbhanage@xxxxxxxxxxxxx>
> Signed-off-by: Erez Zadok <ezk@xxxxxxxxxxxxx>
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index fbb6e5e..9c26ce7 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -381,8 +381,12 @@ out:
> static void ecryptfs_lower_offset_for_extent(loff_t *offset, loff_t extent_num,
> struct ecryptfs_crypt_stat *crypt_stat)
> {
> - (*offset) = (crypt_stat->num_header_bytes_at_front
> - + (crypt_stat->extent_size * extent_num));
> + /* if using xattr, exclude no. of header bytes from start offset */
> + if (crypt_stat->flags & ECRYPTFS_METADATA_IN_XATTR)
> + (*offset) = (crypt_stat->extent_size * extent_num);
> + else
> + (*offset) = (crypt_stat->num_header_bytes_at_front
> + + (crypt_stat->extent_size * extent_num));
> }
>
> /**
> @@ -833,8 +837,10 @@ void ecryptfs_set_default_sizes(struct ecryptfs_crypt_stat *crypt_stat)
> crypt_stat->extent_size = ECRYPTFS_DEFAULT_EXTENT_SIZE;
> set_extent_mask_and_shift(crypt_stat);
> crypt_stat->iv_bytes = ECRYPTFS_DEFAULT_IV_BYTES;
> + /* metadata is kept in xattr, maintain its size in crypt_stat */
> if (crypt_stat->flags & ECRYPTFS_METADATA_IN_XATTR)
> - crypt_stat->num_header_bytes_at_front = 0;
> + crypt_stat->num_header_bytes_at_front =
> + ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE;
> else {
> if (PAGE_CACHE_SIZE <= ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE)
> crypt_stat->num_header_bytes_at_front =
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 2dc0bdb..91a5dba 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -767,7 +767,11 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
> {
> loff_t lower_size;
>
> - lower_size = crypt_stat->num_header_bytes_at_front;
> + /* if xattr, then lower file size starts at 0 (no in-file header) */
> + if (crypt_stat->flags & ECRYPTFS_METADATA_IN_XATTR)
> + lower_size = 0;
> + else
> + lower_size = crypt_stat->num_header_bytes_at_front;
> if (upper_size != 0) {
> loff_t num_extents;
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
