linux-ext4 - Re: [PATCH] ext4: limit external inode xattrs to XATTR_SIZE

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180330215501.GF9300@thunk.org>
Date:   Fri, 30 Mar 2018 17:55:01 -0400
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     Andreas Dilger <adilger@...ger.ca>,
        linux-ext4 <linux-ext4@...r.kernel.org>,
        Wen Xu <wen.xu@...ech.edu>, Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH] ext4: limit external inode xattrs to XATTR_SIZE_MAX

On Fri, Mar 30, 2018 at 12:32:12PM -0700, Eric Biggers wrote:
> But, if we're actually operating under the assumption that someone can modify
> the xattr block's buffer_head concurrently, we'd actually need to copy it to a
> temporary buffer and validate it there, rather than reading directly from the
> cache.  Likewise for the other ext4 metadata such as directory blocks.  Checking
> one specific field in one specific place is not nearly enough.  And if we did
> read directly from the cache we'd have to use volatile memory accesses.

We don't need to copy *all* of the the block and validate it *all*.
We just need to validate the bits which might cause a buffer overrun.
For example:

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index b0a9ba8e576a..7b7e40a05419 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -547,8 +547,14 @@ ext4_xattr_block_get(struct inode *inode, int name_index, const char *name,
 			if (error)
 				goto cleanup;
 		} else {
-			memcpy(buffer, bh->b_data +
-			       le16_to_cpu(entry->e_value_offs), size);
+			__u16 offset = le16_to_cpu(entry->e_value_offs);
+			unsigned long blocksize = inode->i_sb->s_s_blocksize;
+
+			error = -ERANGE;
+			if (unlikely((offset >= blocksize) ||
+				     ((offset + size) >= blocksize)))
+				goto cleanup;
+			memcpy(buffer, bh->b_data + offset, size);
 		}
 	}
 	error = size;

See?  This is much more efficient, doesn't require that we do a memory
allocation for the temp buffer (which might fail), doesn't require any
locking (which would be the other way to try to solve the problem), etc.

					- Ted

P.S.  We should change all of the return -ERANGE to -EFSCORRUPTED and
arrange to have ext4_error() getting called.  Another cleanup patch.
The fs/ext4/xattr.c file is ripe for a lot of cleanup /
robustification patches...