Date:   Wed, 28 Mar 2018 09:59:14 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>,
        Theodore Ts'o <tytso@....edu>, Wen Xu <wen.xu@...ech.edu>,
        Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH] ext4: limit external inode xattrs to XATTR_SIZE_MAX

On Mar 27, 2018, at 3:43 PM, Eric Biggers <ebiggers3@...il.com> wrote:
> 
> Hi Andreas,
> 
> On Tue, Mar 27, 2018 at 02:13:20PM -0600, Andreas Dilger wrote:
>> On Mar 26, 2018, at 9:38 PM, Eric Biggers <ebiggers3@...il.com> wrote:
>>> 
>>> From: Eric Biggers <ebiggers@...gle.com>
>>> 
>>> This is a replacement for the broken patch "ext4: add better range
>>> checking for e_value_size in xattrs" currently in ext4/dev.
>>> 
>>> -----8<-----
>>> 
>>> ext4 isn't validating the sizes of xattrs that are stored in external
>>> inodes.  This is problematic because ->e_value_size is a u32, but
>>> ext4_xattr_get() returns an int.  A very large size is misinterpreted as
>>> an error code, which ext4_get_acl() translates into a bogus ERR_PTR()
>>> for which IS_ERR() returns false, causing a crash.
>>> 
>>> Fix this by validating that all xattrs are <= XATTR_SIZE_MAX bytes.
>>> (It's not strictly needed for non-EA-inode xattrs, but it doesn't hurt.)
>> 
>> Hmm, I'm not so keen on this check.  XATTR_SIZE_MAX is a current, rather
>> arbitrary, limit in the kernel, but there is no reason the on-disk format
>> can't allow a larger xattr to be stored.  Large xattrs would potentially
>> be useful for in-ext4 storage of per-block checksums on an xattr for each
>> file, or other uses beyond what the xattr userspace interface allows.
>> 
>> We used to define another (also arbitrary) limit of 1MB for xattr inodes
>> in the Lustre code, but it was not actually needed and was dropped.
>> 
>> IMHO, it would make more sense to validate e_value_size against i_size
>> (if we want to pay the expense of reading the external xattr at lookup
>> time, otherwise only do it when the external xattr inode is actually read),
>> and return -EFSCORRUPTED if they don't match, or if the xattr is over
>> 2^31 bytes in size.  Otherwise, the filesystem will be mounted read-only,
>> but this may be a legitimate xattr from a newer kernel and a needless DOS.
>> 
>> At most this should return -ERANGE if the xattr is larger than the passed
>> buffer (which will currently be 64KB, but might be larger in the future),
>> but it can't be done from within ext4_xattr_check_entries(), since the caller
>> will call __ext4_error_inode() if any error is returned.
>> 
> 
> To be clear, we already return -ERANGE if the xattr is larger than the passed
> buffer.  The question is what to do if the user requests the size of an xattr.
> In the code, this is the case where the buffer is NULL.
> 
> We could allow returning any size up to INT_MAX.  But large sizes are
> problematic because some callers will actually allocate that amount of memory.
> That's what ext4_get_acl() does, and it uses kmalloc().  Among the issues with
> that is that if the size is over KMALLOC_MAX_SIZE, then the WARN_ON() in
> kmalloc_slab() will be hit, which fuzzers will report as a kernel bug.  So if we
> are going to allow very large sizes to be reported, we need to go through any
> places in the kernel that read variable-size xattrs and either limit the size
> for that type of xattr (e.g. saying that ACLs can only be up to 64K, or 1 MB, or
> whatever), or switch to kvmalloc() with GFP_NOWARN so that large allocations are
> handled more gracefully.

Given that kmalloc() of a 64KB ACL could already fail due to fragmentation of
free memory, it probably makes sense that this be changed to kvmalloc/kvfree()
in any case?

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists