On Fri, Mar 30, 2018 at 01:02:45PM -0400, Theodore Y. Ts'o wrote: > 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. > For xattrs not stored in external inodes we validate that their size fits in the xattr block, or inline in the inode. So their sizes cannot get anywhere close to INT_MAX (though there were a couple bugs I fixed previously, see d7614cc16146). > > > > > > 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. > That's what I meant by someone else modifying the buffer_head concurrently. It can also happen if someone writes directly to the block device node. > > 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. > But, if we're actually operating under the assumption that someone can modify the xattr block's buffer_head concurrently, we'd actually need to copy it to a temporary buffer and validate it there, rather than reading directly from the cache. Likewise for the other ext4 metadata such as directory blocks. Checking one specific field in one specific place is not nearly enough. And if we did read directly from the cache we'd have to use volatile memory accesses. Eric
