[Acked] [PATCH 1/1] eCryptfs: Clear ECRYPTFS_NEW_FILE flag during truncate

[Colin Ian King]

On 16/03/12 11:00, Andy Whitcroft wrote:
> On Fri, Mar 16, 2012 at 09:16:51AM +0000, Colin King wrote:
>> From: Colin Ian King<colin.king at canonical.com>
>>
>> BugLink: http://bugs.launchpad.net/bugs/745836
>>
>> The ECRYPTFS_NEW_FILE crypt_stat flag is set upon creation of a new
>> eCryptfs file. When the flag is set, eCryptfs reads directly from the
>> lower filesystem when bringing a page up to date. This means that no
>> offset translation (for the eCryptfs file metadata in the lower file)
>> and no decryption is performed. The flag is cleared just before the
>> first write is completed (at the beginning of ecryptfs_write_begin()).
>>
>> It was discovered that if a new file was created and then extended with
>> truncate, the ECRYPTFS_NEW_FILE flag was not cleared. If pages
>> corresponding to this file are ever reclaimed, any subsequent reads
>> would result in userspace seeing eCryptfs file metadata and encrypted
>> file contents instead of the expected decrypted file contents.
>>
>> Data corruption is possible if the file is written to before the
>> eCryptfs directory is unmounted. The data written will be copied into
>> pages which have been read directly from the lower file rather than
>> zeroed pages, as would be expected after extending the file with
>> truncate.
>>
>> This flag, and the functionality that used it, was removed in upstream
>> kernels in 2.6.39 with the following commits:
>>
>> bd4f0fe8bb7c73c738e1e11bc90d6e2cf9c6e20e
>> fed8859b3ab94274c986cbdf7d27130e0545f02c
>
> Confirmed these two are in Natty as cherry-picks.
>
>> Signed-off-by: Tyler Hicks<tyhicks at canonical.com>
>> Signed-off-by: Colin Ian King<colin.king at canonical.com>
>> ---
>>   fs/ecryptfs/inode.c |    3 +++
>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
>> index 645da17..3c1dbc0 100644
>> --- a/fs/ecryptfs/inode.c
>> +++ b/fs/ecryptfs/inode.c
>> @@ -777,6 +777,9 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
>>   		goto out;
>>   	}
>>   	crypt_stat =&ecryptfs_inode_to_private(dentry->d_inode)->crypt_stat;
>> +	if (crypt_stat->flags&  ECRYPTFS_NEW_FILE)
>> +		crypt_stat->flags&= ~(ECRYPTFS_NEW_FILE);
>> +
>>   	/* Set up a fake ecryptfs file, this is used to interface with
>>   	 * the file in the underlying filesystem so that the
>>   	 * truncation has an effect there as well. */
>
> Seems to do what it says.
>
> Acked-by: Andy Whitcroft<apw at canonical.com>
>
> BTW as we are wrapping up Maverick, is this serious enough to warrent
> including in any final upload.  Data corruption sounds like a critical
> issue.

Yep, I will configure up a test machine and check out the patch on this 
sometime today.

Colin
>
> -apw
>