Re: [PATCH] ext4: prevent out-of-bounds read in ext4_read_inline_data()

[Jan Kara]

On Fri 24-04-26 01:05:26, Junjie Cao wrote:
> Thanks for the review, Jan.
> 
> You're right that v1 failed to identify why the buffer changes.  I dug
> into the syzbot reproducer ??? the corruption path is:
> 
>   1. Mount a crafted ext4 image on a loop device
>   2. Bind-mount the loop device, open + mmap it MAP_SHARED|PROT_WRITE
>   3. Write through the mapping ??? this overwrites the inline xattr
>      entry directly in the bdev page cache

Ah, interesting. I had a look at the syzbot bug and all the reproductions
are actually only on quite old Android kernel (5.15). That kernel doesn't
have infrastructure to block writes to mounted devices - in newer kernels
syzbot sets

CONFIG_BLK_DEV_WRITE_MOUNTED=n

which blocks reproducers like this one.

> The inode buffer_head stays uptodate throughout, so no re-validation
> ever triggers ??? xattr_check_inode() at iget time is thorough but only
> runs once, leaving subsequent in-place corruption of the page cache
> undetected.

Yes, writing to buffer cache through the mapping is equivalent to poking
to your memory. You can do a lot of damage if you are not careful.
Filesystems have no sensible way to protect against such things and in
general it is root-restricted corruption vector so not really interesting
from security perspective.

> However, ext4_xattr_ibody_get() already guards against this with a
> bounds check before its memcpy (xattr.c:674).  ext4_read_inline_data()
> lacks the same check because it indexes via the cached i_inline_off,
> bypassing xattr_find_entry() entirely.  I think aligning the two paths
> is worthwhile, and it would also clear this syzbot report.
> 
> Would a v2 with this framing be acceptable to you?

No, I don't think there a problem to fix. The additional checks will
complicate the code, will be racy anyway, and will cost some performance.
There is no good reason to have them to protect from sysadmin doing stupid
stuff...

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR