Re: [PATCH] eCryptfs: Fix kernel bug for writing mmaped non-eCryptfsfile

[Tyler Hicks]

On 2012-03-13 19:39:49, Li Wang wrote:
> eCryptfs did not handle the writing for mmaped non-eCryptfs file.
> Instead, it put BUG_ON(!(crypt_stat->flags & ECRYPTFS_ENCRYPTED))
> on ecryptfs_writepage call path. This patch enables eCryptfs to
> deal with such case, to fully support non-eCryptfs operations as it
> claims.

Hi Li - Thanks for the patch!

Before I review/merge it, I'm curious if you use the
ecryptfs_plaintext_passthrough feature? IMO, it is a terrible design and
I've found that hardly anyone uses it. Maybe *no one*, after knowing
that mmap was broken for plaintext files...

Having to pre-create the file in the lower filesystem in order for it to
be a "plaintext" file inside the eCryptfs mount is a terrible idea. The
admin/user/application has no idea if the file is plaintext by looking
at it inside the eCryptfs mount and he/she has to examine the file in
the lower filesystem to find out. If that file is ever unlinked and then
created again, it turns into an encrypted file. There is too much
uncertainty around this feature for it to exist in a cryptographic
filesystem.

I'd really like to continue focusing on getting eCryptfs as stable as
possible and then dump the CONFIG_EXPERIMENTAL dependency at some point.
As a prerequisite to that, I'm thinking that
ecryptfs_plaintext_passthrough support should be removed, at least in
its current form. Any thoughts?

Tyler

> 
> ---
> 
> To make the bug present
> 
> cd cipher // enter eCryptfs cipher text folder
> echo "123" > foo // make non-eCryptfs file
> cd ..
> mount -t ecryptfs cipher plain -o ecryptfs_passthrough // allow for non-eCryptfs files to be read and written from within an eCryptfs mount
> cd plain
> run the following program
> 
> int main()
> {
> 	int fd = open("foo", O_RDWR);
> 	char * addr;
> 	addr = (char *)mmap(NULL, 256, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> 	add[0] = '3';
> 	munmap(addr, 256);
> 	close(fd);
> 	return 0;
> }
> 
> Signed-off-by: Li Wang <liwang@xxxxxxxxxxx>
> Signed-off-by: Yunchuan Wen <wenyunchuan@xxxxxxxxxxxxxx>
> ---
>  fs/ecryptfs/mmap.c |   26 ++++++++++++++++++++++++++
>  1 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
> index 10ec695..a4be0e9 100644
> --- a/fs/ecryptfs/mmap.c
> +++ b/fs/ecryptfs/mmap.c
> @@ -65,6 +65,32 @@ struct page *ecryptfs_get_locked_page(struct inode *inode, loff_t index)
>  static int ecryptfs_writepage(struct page *page, struct writeback_control *wbc)
>  {
>  	int rc;
> +	struct inode *inode;
> +	struct ecryptfs_crypt_stat *crypt_stat;
> +
> +	inode = page->mapping->host;
> +	crypt_stat = &ecryptfs_inode_to_private(inode)->crypt_stat;
> +	if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
> +		size_t size;
> +		loff_t file_size = i_size_read(inode);
> +		pgoff_t end_page_index = file_size >> PAGE_CACHE_SHIFT;
> +
> +		if (end_page_index < page->index)
> +			size = 0;
> +		else if (end_page_index == page->index)
> +			size = file_size & ~PAGE_CACHE_MASK;
> +		else
> +			size = PAGE_CACHE_SIZE;
> +
> +		rc = ecryptfs_write_lower_page_segment(inode, page, 0, size);
> +		if (unlikely(rc)) {
> +			ecryptfs_printk(KERN_WARNING, "Error write "
> +					"page (upper index [0x%.16lx])\n", page->index);
> +			ClearPageUptodate(page);
> +		} else
> +			SetPageUptodate(page);
> +		goto out;
> +	}
>  
>  	/*
>  	 * Refuse to write the page out if we are called from reclaim context
> -- 
> 1.7.6.5
>

Attachment: signature.asc
Description: Digital signature