Hi Ted, On Thu, Mar 29, 2018 at 03:41:25PM -0400, Theodore Y. Ts'o wrote: > I tried a variation of this patch (checking against INT_MAX) instead > of XATTR_MAX_SIZE, but it's not sufficient to address the BUG() in > > https://bugzilla.kernel.org/show_bug.cgi?id=199185 > > The reason for that is that initially the xattr block is OK: > > [ 15.236890] xattr_check_block: 2047 0 > > but afterwards, due to other file system corruptions (remember, this > is a carefully corrupted file system by malicious attacker), the xattr > block gets corrupted in memory, and then *subsequent* checks of the > block are skipped because the "buffer verified" bit is set: > > [ 15.239204] EXT4-fs error (device vdc): mb_free_blocks:1456: group 0, inode 16: block 1985:freeing already freed block (bit 1984); block bitmap corrupt. > [ 15.243899] EXT4-fs error (device vdc): ext4_mb_generate_buddy:744: group 0, block bitmap and bg descriptor inconsistent: 960 vs 961 free clusters > [ 15.248943] xattr_check_block skip: 2047 > [ 15.251442] xattr_check_block skip: 2047 > [ 15.252642] ext4_xattr_block_get ERANGE: 2047 > > Hence, if we don't add a specific check in ext4_xattr_block_get(), we > still return an overflowed size as an error check, and then kernel > will still BUG. I think this analysis is wrong. The check in ext4_xattr_check_entries() is sufficient for the given POC provided that the check is done for all xattrs, like my original patch did. Your modified version of my patch doesn't do the check when entry->e_value_inum != 0, but that's exactly the case that was missing the size check and is triggered by the POC. Also note that this bug was introduced with the EA inode feature; again, see my original patch, which gave the Fixes: line and noted that the size validation was missing only for xattrs stored in external inodes. > > I think it's fair to add an INT_MAX check to > ext4_xattr_check_entries(), but we're still going to need to have > checks in ext4_xattr_block_get() and ext4_xattr_ibody_get(). > This isn't applicable given my comment above, but even if it was, this doesn't make sense. If the xattr code itself can corrupt the xattr list, then that is its own, different bug. Else, we are talking about someone else modifying the buffer head concurrently, which seems to be well outside the threat model assumed by ext4 for its metadata (not just xattrs but also directory blocks, inodes, etc.). For ext4 to actually parse its metadata securely under the assumption that the copy in the block device's address_space can be concurrently modified, it would need to be copied to a temporary buffer and re-validated on every access. Eric
