On Nov 26, 2016, at 11:39 PM, Eric Biggers <ebiggers@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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:
signature.asc
Description: Message signed with OpenPGP using GPGMail
