linux-kernel - Re:[PATCH 2/3] eCryptfs: Check inode changes in setattr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <120121155758aecba8095c7be7e9983b250416d86f0c@nudt.edu.cn>
Date:	Sat, 21 Jan 2012 15:57:58 +0800
From:	"Li Wang" <liwang@...t.edu.cn>
To:	"Linus Torvalds" <torvalds@...ux-foundation.org>,
	john.johansen@...onical.com, dustin.kirkland@...zang.com,
	"Cong Wang" <xiyou.wangcong@...il.com>, ecryptfs@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	"Tyler Hicks" <tyhicks@...onical.com>
Subject: Re:[PATCH 2/3] eCryptfs: Check inode changes in setattr

Hi Tyler,
  Please consider the following two things,
1. While invoking inode_newsize_ok/inode_change_ok, it just make sure the new file size seen from
eCryptfs will not exceed the whatever kinds of file size limit, what about the new size does not
exceed the limit, plus ecryptfs_lower_header_size will. Therefore the safest way is to check the
new size seen from lower file system, which is ecryptfs_lower_header_size bigger.  
2. The senmatics of sb->s_maxbytes, is the maximum file size allowed by the file system 
repsented by sb. For eCryptfs, it should be lower_sb->s_maxbytes - ecryptfs_lower_header_size, 
rather than equal to lower_sb->s_maxbytes. However, the ecryptfs_lower_header_size is different
file by file, not a file system wide constant. It is, kind of nasty and we cannot trust it. 
Combined with the reason 1, we prefer to execute an extra new size check on lower inode
after inode_change_ok on ecryptfs inode. For ecryptfs_truncate, directly perform new size check
on lower inode. 
  Please check the patch below.

Cheers,
Li Wang

Signed-off-by: Li Wang <liwang@...t.edu.cn>
               Yunchuan Wen <wenyunchuan@...inos.com.cn>

---

diff -prNu a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
--- a/fs/ecryptfs/inode.c	2012-01-05 07:55:44.000000000 +0800
+++ b/fs/ecryptfs/inode.c	2012-01-21 15:55:21.000000000 +0800
@@ -841,18 +841,6 @@ static int truncate_upper(struct dentry 
 		size_t num_zeros = (PAGE_CACHE_SIZE
 				    - (ia->ia_size & ~PAGE_CACHE_MASK));

-
-		/*
-		 * XXX(truncate) this should really happen at the begginning
-		 * of ->setattr.  But the code is too messy to that as part
-		 * of a larger patch.  ecryptfs is also totally missing out
-		 * on the inode_change_ok check at the beginning of
-		 * ->setattr while would include this.
-		 */
-		rc = inode_newsize_ok(inode, ia->ia_size);
-		if (rc)
-			goto out;
-
 		if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
 			truncate_setsize(inode, ia->ia_size);
 			lower_ia->ia_size = ia->ia_size;
@@ -916,8 +904,14 @@ int ecryptfs_truncate(struct dentry *den
 {
 	struct iattr ia = { .ia_valid = ATTR_SIZE, .ia_size = new_length };
 	struct iattr lower_ia = { .ia_valid = 0 };
+	struct ecryptfs_crypt_stat *crypt_stat;
 	int rc;
-
+	
+	crypt_stat = &ecryptfs_inode_to_private(dentry->d_inode)->crypt_stat;
+	rc = inode_newsize_ok(ecryptfs_inode_to_lower(dentry->d_inode), new_length + ecryptfs_lower_header_size(crypt_stat));
+	if (rc)
+		return rc;
+	
 	rc = truncate_upper(dentry, &ia, &lower_ia);
 	if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
 		struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
@@ -997,6 +991,15 @@ static int ecryptfs_setattr(struct dentr
 		}
 	}
 	mutex_unlock(&crypt_stat->cs_mutex);
+	
+	rc = inode_change_ok(inode, ia);
+	if (rc)
+		goto out;
+	if (ia->ia_valid & ATTR_SIZE)
+		rc = inode_newsize_ok(lower_inode, ia->ia_size + ecryptfs_lower_header_size(crypt_stat));
+	if (rc)
+		goto out;
+	
 	if (S_ISREG(inode->i_mode)) {
 		rc = filemap_write_and_wait(inode->i_mapping);
 		if (rc)

---------- Origin message ----------
>From："Tyler Hicks" <tyhicks@...onical.com>
>To：ecryptfs@...r.kernel.org, linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
>Subject：[PATCH 2/3] eCryptfs: Check inode changes in setattr
>Date：2012-01-21 06:35:06

Most filesystems call inode_change_ok() very early in ->setattr(), but
eCryptfs didn't call it at all. It allowed the lower filesystem to make
the call in its ->setattr() function. Then, eCryptfs would copy the
appropriate inode attributes from the lower inode to the eCryptfs inode.

This patch changes that and actually calls inode_change_ok() on the
eCryptfs inode, fairly early in ecryptfs_setattr(). Ideally, the call
would happen earlier in ecryptfs_setattr(), but there is some possible
inode initialization that must happen first.

Since the call was already being made on the lower inode, the change in
functionality should be minimal, except for the case of a file extending
truncate call. In that case, inode_newsize_ok() was never being
called on the eCryptfs inode. Rather than inode_newsize_ok() catching
errors early on, eCryptfs would encrypt zeroed pages and write them to
the lower filesystem until the lower filesystem's write path caught the
error in generic_write_checks().

In summary this change prevents eCryptfs truncate operations (and the
resulting page encryptions), which would exceed the lower filesystem