On Thu, Mar 29, 2018 at 08:43:02PM -0700, Eric Biggers wrote: > > 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. You're right, I misparsed your patch. It also wasn't how the code pre the Fixes: commit how the POC worked, since it wasn't as explicit. > > > > 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. Well, it's not necessary xattr code that could do this. For example,ht you can have a bitmap block pointing at an xattr block, or a extent tree leaf block and an xattr block mapped onto the same physical block. The first case can be detected with block_validity (which is now enabled by default, but which can be turned off). The second is one that we can't necessarily detect. It might be because of a bug, or a hardware issue (e.g., a media error), or due to a maliciously introduced 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. See above. The problem is if a block is concurrently modified by two different pieces of ext4 code, due to a maliciously crafted image. Some of this could be detected by annotating the buffer head if we notice that a block is used by two different use cases. That would be a non-trivial change, but that would eliminate most of these issues. It might be possible to eliminate all of them, but it gets tricky to make sure we handle all of the cases --- for example, in the the data=journalled mode where data blocks also get modified through buffer_heads. This wasn't what was going on here, but one of the other POC's it was the case that we ran into the problem where a bitmap allocation block was twiddling the superblock. So when I misinterpreted what had been going on above, I had assumed that this was another case where a metadata block was getting modified in two different ways as two different types of metadata. Cheers, - Ted
