[PATCH] ext4: remove some redundant corruption checks

[PATCH] ext4: remove some redundant corruption checks

[Chengguang Xu]

Remove some redundant corruption checks in
ext4_xattr_block_get() and ext4_xattr_ibody_get()
because ext4_xattr_check_entries() has done those
checks.

Signed-off-by: Chengguang Xu <cgxu519@zoho.com.cn>
---
 fs/ext4/xattr.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 491f9ee4040e..b74346f103a6 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -542,8 +542,6 @@ ext4_xattr_block_get(struct inode *inode, int name_index, const char *name,
 		goto cleanup;
 	size = le32_to_cpu(entry->e_value_size);
 	error = -ERANGE;
-	if (unlikely(size > EXT4_XATTR_SIZE_MAX))
-		goto cleanup;
 	if (buffer) {
 		if (size > buffer_size)
 			goto cleanup;
@@ -556,8 +554,6 @@ ext4_xattr_block_get(struct inode *inode, int name_index, const char *name,
 			u16 offset = le16_to_cpu(entry->e_value_offs);
 			void *p = bh->b_data + offset;
 
-			if (unlikely(p + size > end))
-				goto cleanup;
 			memcpy(buffer, p, size);
 		}
 	}
@@ -597,8 +593,6 @@ ext4_xattr_ibody_get(struct inode *inode, int name_index, const char *name,
 		goto cleanup;
 	size = le32_to_cpu(entry->e_value_size);
 	error = -ERANGE;
-	if (unlikely(size > EXT4_XATTR_SIZE_MAX))
-		goto cleanup;
 	if (buffer) {
 		if (size > buffer_size)
 			goto cleanup;
@@ -611,8 +605,6 @@ ext4_xattr_ibody_get(struct inode *inode, int name_index, const char *name,
 			u16 offset = le16_to_cpu(entry->e_value_offs);
 			void *p = (void *)IFIRST(header) + offset;
 
-			if (unlikely(p + size > end))
-				goto cleanup;
 			memcpy(buffer, p, size);
 		}
 	}
-- 
2.17.2

Re: [PATCH] ext4: remove some redundant corruption checks

[Theodore Ts'o]

On Thu, May 23, 2019 at 08:48:43PM +0800, Chengguang Xu wrote:
> Remove some redundant corruption checks in
> ext4_xattr_block_get() and ext4_xattr_ibody_get()
> because ext4_xattr_check_entries() has done those
> checks.
> 
> Signed-off-by: Chengguang Xu <cgxu519@zoho.com.cn>

I deliberately left in the redundant checks.

That's because we still have issues where a buffer can be verified for
some other block type (say, an allocation bitmap), but if that block
is also referenced as an xattr block, we could cause a kernel crash,
or worse, we might be end up corrupting kernel memory in some way that
would lead to a privilege escalation test.

I would actually prefer that we have enough tests ant the Time Of Use
that we wouldn't need to check the whole xattr block the first time we
read it into memory.  The way we are doing it today, we are very much
vulnerable to TOC/TOU bugs/vulnerabilities.

In the long run we should making the xattr code so careful and robust
that we wouldn't need to use the ext4_xattr_check_entries() except as
a backup safety.  (Today, we depend on it far too much, and there have
been multiple example of deliberately/malicious crafted file systems
which can bypass the checks in ext4_xattr_check_entries() check.  Just
scan fs/ext4/xattr.c for various CVE fixes....)  So if anything we
should be adding more checks to ext4_xattr_block_get(), et. al.

					- Ted