[PATCH 1/3] eCryptfs: Write bug for non-ecryptfs file

[PATCH 1/3] eCryptfs: Write bug for non-ecryptfs file

[Li Wang]

The following code segment in ecryptfs_write_begin(mmap.c) is problematic,

1  if (!PageUptodate(page)) {
2	if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
3		...
4	} else if(crypt_stat->flags & ECRYPTFS_VIEW_AS_ENCRYPTED) {
5		...
6	} else {
7		if (prev_page_end_size >= i_size_read(page->mapping->host)) {
8			zero_user(page, 0, PAGE_CACHE_SIZE);
9		}
10		...
11	}
12	...
13      /* Writing to a new page, and creating a small hole from start
14       * of page? Zero it out. */
15	if ((i_size_read(mapping->host)==prev_page_end_size) && (pos!=0))
16		zero_user(page, 0, PAGE_CACHE_SIZE);

1 The check on 'pos!=0' for the IF statement in line 15 implies that while
  pos==0, the page needs not be zeroed. Unfortunately, that is not true,
  suppose an empty non-ecryptfs file has been created beforehand, currently the
  writter want to write at pos==0 with a length of m bytes, m<PAGE_CACHE_SIZE,
  now the page will not be zeroed, and set to Uptodate! Then the data within
  (m,PAGE_CACHE_SIZE) read by any reader will be undefined.
2 Even remove 'pos!=0', there is further more a case which does not be taken
  into account,
  if a file satisfies (i_size_read(mapping->host) > prev_page_end_size), and is
  non-ecryptfs file, then the page is not zeroed, but the data within
  (i_size_read(mapping->host), (prev_page_end_size+1)<<PAGE_CACHE_SHIFT) need
  be zeroed as well.
3 For ecryptfs file, if satisfies (prev_page_end_size == i_size_read(page->
  mapping->host) && pos!=0, this page will be double zeroed.

This patch solves the above problem.

Signed-off-by: Li Wang <liwang@nudt.edu.cn>
Signed-off-by: Yunchuan Wen <wenyunchuan@kylinos.com.cn>
---
 fs/ecryptfs/mmap.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index 10ec695..27c0da8 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -308,6 +308,8 @@ static int ecryptfs_write_begin(struct file *file,
 			&ecryptfs_inode_to_private(mapping->host)->crypt_stat;
 
 		if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
+			if ((i_size_read(mapping->host) >> PAGE_CACHE_SHIFT) <= index)
+				zero_user(page, 0, PAGE_CACHE_SIZE);
 			rc = ecryptfs_read_lower_page_segment(
 				page, index, 0, PAGE_CACHE_SIZE, mapping->host);
 			if (rc) {
@@ -379,11 +381,6 @@ static int ecryptfs_write_begin(struct file *file,
 			}
 		}
 	}
-	/* Writing to a new page, and creating a small hole from start
-	 * of page?  Zero it out. */
-	if ((i_size_read(mapping->host) == prev_page_end_size)
-	    && (pos != 0))
-		zero_user(page, 0, PAGE_CACHE_SIZE);
 out:
 	if (unlikely(rc)) {
 		unlock_page(page);
-- 
1.7.6.5

[PATCH 2/3] eCryptfs: Remove redundant IF statement

[Li Wang]

ecryptfs_write_begin(mmap.c)
1  ...
2  if (index != 0) {
3	if (prev_page_end_size > i_size_read(page->mapping->host)) {
4		...
5	}
6	...
7  }
The IF statement in line 2 is redundant since the IF statement in line
3 deduces that.

Signed-off-by: Li Wang <liwang@nudt.edu.cn>
---
 fs/ecryptfs/mmap.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index 27c0da8..59a8e57 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -368,8 +368,7 @@ static int ecryptfs_write_begin(struct file *file,
 	}
 	/* If creating a page or more of holes, zero them out via truncate.
 	 * Note, this will increase i_size. */
-	if (index != 0) {
-		if (prev_page_end_size > i_size_read(page->mapping->host)) {
+	if (prev_page_end_size > i_size_read(page->mapping->host)) {
 			rc = ecryptfs_truncate(file->f_path.dentry,
 					       prev_page_end_size);
 			if (rc) {
@@ -379,7 +378,6 @@ static int ecryptfs_write_begin(struct file *file,
 				       prev_page_end_size, rc);
 				goto out;
 			}
-		}
 	}
 out:
 	if (unlikely(rc)) {
-- 
1.7.6.5

[PATCH 3/3] eCryptfs: Remove unreached codes

[Li Wang]

The code segment handles the ECRYPTFS_VIEW_AS_ENCRYPTED option in
ecryptfs_write_begin(mmap.c) is unreached, since this option is
enabled only when eCryptfs is mounted with RDONLY.

Signed-off-by: Li Wang <liwang@nudt.edu.cn>
---
 fs/ecryptfs/mmap.c |   29 +----------------------------
 1 files changed, 1 insertions(+), 28 deletions(-)

diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index 59a8e57..a4802a6 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -320,35 +320,8 @@ static int ecryptfs_write_begin(struct file *file,
 				goto out;
 			} else
 				SetPageUptodate(page);
-		} else if (crypt_stat->flags & ECRYPTFS_VIEW_AS_ENCRYPTED) {
-			if (crypt_stat->flags & ECRYPTFS_METADATA_IN_XATTR) {
-				rc = ecryptfs_copy_up_encrypted_with_header(
-					page, crypt_stat);
-				if (rc) {
-					printk(KERN_ERR "%s: Error attempting "
-					       "to copy the encrypted content "
-					       "from the lower file whilst "
-					       "inserting the metadata from "
-					       "the xattr into the header; rc "
-					       "= [%d]\n", __func__, rc);
-					ClearPageUptodate(page);
-					goto out;
-				}
-				SetPageUptodate(page);
-			} else {
-				rc = ecryptfs_read_lower_page_segment(
-					page, index, 0, PAGE_CACHE_SIZE,
-					mapping->host);
-				if (rc) {
-					printk(KERN_ERR "%s: Error reading "
-					       "page; rc = [%d]\n",
-					       __func__, rc);
-					ClearPageUptodate(page);
-					goto out;
-				}
-				SetPageUptodate(page);
-			}
 		} else {
+			BUG_ON(crypt_stat->flags & ECRYPTFS_VIEW_AS_ENCRYPTED);
 			if (prev_page_end_size
 			    >= i_size_read(page->mapping->host)) {
 				zero_user(page, 0, PAGE_CACHE_SIZE);
-- 
1.7.6.5