Message-ID: <20180327214336.GA257332@gmail.com>
Date:   Tue, 27 Mar 2018 14:43:36 -0700
From:   Eric Biggers <ebiggers3@...il.com>
To:     Andreas Dilger <adilger@...ger.ca>
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

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.

Eric

Powered by blists - more mailing lists