Re: [PATCH 3/3] ext4: correctly detect when an xattr value has an invalid size

[Andreas Dilger <adilger@dilger.ca>]

[-- Attachment #1: Type: text/plain, Size: 3263 bytes --]

On Nov 26, 2016, at 11:39 PM, Eric Biggers <ebiggers@google.com> wrote:
> 
> It was possible for an xattr value to have a very large size, which
> would then pass validation on 32-bit architectures due to a pointer
> wraparound.  Fix this by validating the size in a way which avoids
> pointer wraparound.

It isn't actually possible for a valid xattr value to be very large.
At most 65536 bytes even with large blocks, so it might be easier to
directly check that e_value_size is not too large rather than trying
to deal with values of 0xfffffffe bytes or similar?

Cheers, Andreas

> It was also possible that a value's size would fit in the available
> space but its padded size would not.  This would cause an out-of-bounds
> memory write in ext4_xattr_set_entry when replacing the xattr value.
> For example, if an xattr value of unpadded size 253 bytes went until the
> very end of the inode or block, then using setxattr(2) to replace this
> xattr's value with 256 bytes would cause a write to the 3 bytes past the
> end of the inode or buffer, and the new xattr value would be incorrectly
> truncated.  Fix this by requiring that the padded size fit in the
> available space rather than the unpadded size.
> 
> This patch shouldn't have any noticeable effect on
> non-corrupted/non-malicious filesystems.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> fs/ext4/xattr.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 59c9ec7..5a94fa52 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -185,6 +185,7 @@ ext4_xattr_check_names(struct ext4_xattr_entry *entry, void *end,
> {
> 	struct ext4_xattr_entry *e = entry;
> 
> +	/* Find the end of the names list */
> 	while (!IS_LAST_ENTRY(e)) {
> 		struct ext4_xattr_entry *next = EXT4_XATTR_NEXT(e);
> 		if ((void *)next >= end)
> @@ -192,15 +193,29 @@ ext4_xattr_check_names(struct ext4_xattr_entry *entry, void *end,
> 		e = next;
> 	}
> 
> +	/* Check the values */
> 	while (!IS_LAST_ENTRY(entry)) {
> 		if (entry->e_value_block != 0)
> 			return -EFSCORRUPTED;
> -		if (entry->e_value_size != 0 &&
> -		    (value_start + le16_to_cpu(entry->e_value_offs) <
> -		     (void *)e + sizeof(__u32) ||
> -		     value_start + le16_to_cpu(entry->e_value_offs) +
> -		    le32_to_cpu(entry->e_value_size) > end))
> -			return -EFSCORRUPTED;
> +		if (entry->e_value_size != 0) {
> +			u16 offs = le16_to_cpu(entry->e_value_offs);
> +			u32 size = le32_to_cpu(entry->e_value_size);
> +			void *value;
> +
> +			/*
> +			 * The value cannot overlap the names, and the value
> +			 * with padding cannot extend beyond 'end'.  Check both
> +			 * the padded and unpadded sizes, since the size may
> +			 * overflow to 0 when adding padding.
> +			 */
> +			if (offs > end - value_start)
> +				return -EFSCORRUPTED;
> +			value = value_start + offs;
> +			if (value < (void *)e + sizeof(u32) ||
> +			    size > end - value ||
> +			    EXT4_XATTR_SIZE(size) > end - value)
> +				return -EFSCORRUPTED;
> +		}
> 		entry = EXT4_XATTR_NEXT(entry);
> 	}
> 
> --
> 2.8.0.rc3.226.g39d4020
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]