On Mar 27, 2018, at 3:43 PM, Eric Biggers <ebiggers3@xxxxxxxxx> 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@xxxxxxxxx> wrote: >>> >>> From: Eric Biggers <ebiggers@xxxxxxxxxx> >>> >>> 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
Attachment:
signature.asc
Description: Message signed with OpenPGP
