Tyler,
Thank you for quick review of the patch and suggestions. As per our
observation num_header_bytes_at_front should hold size of header bytes
irrespective of where header is stored. This will allow other functions
to use same header size without causing some inconsistency.
Many functions like ecryptfs_lookup_and_interpose_lower(),
ecryptfs_write_metadata(), ecryptfs_readpage(), ecryptfs_write_begin()
and ecryptfs_write_inode_size_to_metadata() perform a check if metadata
is stored in xattrs. But ecryptfs_lower_offset_for_extent() and
upper_size_to_lower_size() calculate file offsets from
num_header_bytes_at_front without a check. When xattrs are used,
crypt_stat->num_header_bytes_at_front should be excluded from file
offset calculations because it is not part of real file data. But they
are in fact present in the xattrs, and we can get header size from
crypt_stat->num_header_bytes_at_front if required.
So our proposal to solve this inconsistency is to have a generic
initialization of crypt_stat->num_header_bytes_at_front in
ecryptfs_set_default_sizes() to its required header size and to add
checks in ecryptfs_lower_offset_for_extent() as well as
upper_size_to_lower_size() where it is missing.
As per your suggestion, ecryptfs_read_xattr_region() needed updates for
header size to read which should be same as
ecryptfs_set_default_sizes() that is added in the patch.
If you can comment on this approach vs. one with custom initializing
virt_len in ecryptfs_write_metadata() and leaving
crypt_stat->num_header_bytes_at_front to zero, then we can correct
the patch.
--
Regards,
Saumitra Bhanage.
--------------------------------------------------------------------------
eCryptfs: properly compute header sizes/offsets when storing headers in xattrs
Ecryptfs can store its meta-data in a header which can be either a 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 -uNrp a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
--- a/fs/ecryptfs/crypto.c 2009-12-02 22:51:21.000000000 -0500
+++ b/fs/ecryptfs/crypto.c 2009-12-08 18:31:05.000000000 -0500
@@ -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,15 +837,12 @@ void ecryptfs_set_default_sizes(struct e
crypt_stat->extent_size = ECRYPTFS_DEFAULT_EXTENT_SIZE;
set_extent_mask_and_shift(crypt_stat);
crypt_stat->iv_bytes = ECRYPTFS_DEFAULT_IV_BYTES;
- if (crypt_stat->flags & ECRYPTFS_METADATA_IN_XATTR)
- crypt_stat->num_header_bytes_at_front = 0;
- else {
- if (PAGE_CACHE_SIZE <= ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE)
- crypt_stat->num_header_bytes_at_front =
- ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE;
- else
- crypt_stat->num_header_bytes_at_front = PAGE_CACHE_SIZE;
- }
+
+ if (PAGE_CACHE_SIZE <= ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE)
+ crypt_stat->num_header_bytes_at_front =
+ ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE;
+ else
+ crypt_stat->num_header_bytes_at_front = PAGE_CACHE_SIZE;
}
/**
@@ -1532,11 +1533,16 @@ int ecryptfs_read_xattr_region(char *pag
{
struct dentry *lower_dentry =
ecryptfs_inode_to_private(ecryptfs_inode)->lower_file->f_dentry;
- ssize_t size;
+ ssize_t size, header_size;
int rc = 0;
+ if (PAGE_CACHE_SIZE <= ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE)
+ header_size = ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE;
+ else
+ header_size = PAGE_CACHE_SIZE;
+
size = ecryptfs_getxattr_lower(lower_dentry, ECRYPTFS_XATTR_NAME,
- page_virt, ECRYPTFS_DEFAULT_EXTENT_SIZE);
+ page_virt, header_size);
if (size < 0) {
if (unlikely(ecryptfs_verbosity > 0))
printk(KERN_INFO "Error attempting to read the [%s] "
diff -uNrp a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
--- a/fs/ecryptfs/inode.c 2009-12-02 22:51:21.000000000 -0500
+++ b/fs/ecryptfs/inode.c 2009-12-08 18:23:38.000000000 -0500
@@ -759,7 +759,11 @@ upper_size_to_lower_size(struct ecryptfs
{
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;
On Mon, Dec 7, 2009 at 5:12 PM, Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxx> wrote:
> 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
