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. Cheers, Andreas > https://bugzilla.kernel.org/show_bug.cgi?id=199185 > > Reported-by: Wen Xu <wen.xu@xxxxxxxxxx> > Fixes: e50e5129f384 ("ext4: xattr-in-inode support") > Cc: <stable@xxxxxxxxxxxxxxx> # v4.13+ > Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx> > --- > fs/ext4/xattr.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > index 63656dbafdc45..8c9ade64aea2a 100644 > --- a/fs/ext4/xattr.c > +++ b/fs/ext4/xattr.c > @@ -195,10 +195,13 @@ ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end, > > /* Check the values */ > while (!IS_LAST_ENTRY(entry)) { > - if (entry->e_value_size != 0 && > - entry->e_value_inum == 0) { > + u32 size = le32_to_cpu(entry->e_value_size); > + > + if (size > XATTR_SIZE_MAX) > + return -EFSCORRUPTED; > + > + if (size != 0 && entry->e_value_inum == 0) { > u16 offs = le16_to_cpu(entry->e_value_offs); > - u32 size = le32_to_cpu(entry->e_value_size); > void *value; > > /* > -- > 2.16.2 > Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
