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

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

[Junjie Cao]

ext4_read_inline_data() reads e_value_offs from the inode buffer_head on
each call, but the decision to enter the xattr value path depends on
i_inline_size cached in EXT4_I(inode) at iget time. If the buffer
contents change after the initial validation, e_value_offs can point
beyond the inode body while i_inline_size still directs the code into
the xattr value path, causing an out-of-bounds read in the memcpy.

Add a bounds check before the memcpy, consistent with
ext4_xattr_ibody_get(). Also guard folio_mark_uptodate() in
ext4_read_inline_folio() since ext4_read_inline_data() can now return
-EFSCORRUPTED.

Fixes: 67cf5b09a46f ("ext4: add the basic function for inline data support")
Cc: stable@vger.kernel.org
Reported-by: syzbot+26c4a8cab92d0cda3e3b@syzkaller.appspotmail.com
Tested-by: syzbot+26c4a8cab92d0cda3e3b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=26c4a8cab92d0cda3e3b
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 fs/ext4/inline.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 408677fa8196..18c678df0a6e 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -211,6 +211,14 @@ static int ext4_read_inline_data(struct inode *inode, void *buffer,
 	len = min_t(unsigned int, len,
 		    (unsigned int)le32_to_cpu(entry->e_value_size));
 
+	if (unlikely((void *)IFIRST(header) + le16_to_cpu(entry->e_value_offs) +
+		     len > (void *)ITAIL(inode, raw_inode))) {
+		EXT4_ERROR_INODE(inode,
+			"inline data value out of bounds (offs %u len %u)",
+			le16_to_cpu(entry->e_value_offs), len);
+		return -EFSCORRUPTED;
+	}
+
 	memcpy(buffer,
 	       (void *)IFIRST(header) + le16_to_cpu(entry->e_value_offs), len);
 	cp_len += len;
@@ -535,7 +543,8 @@ static int ext4_read_inline_folio(struct inode *inode, struct folio *folio)
 	ret = ext4_read_inline_data(inode, kaddr, len, &iloc);
 	kaddr = folio_zero_tail(folio, len, kaddr + len);
 	kunmap_local(kaddr);
-	folio_mark_uptodate(folio);
+	if (ret >= 0)
+		folio_mark_uptodate(folio);
 	brelse(iloc.bh);
 
 out:
-- 
2.43.0

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

[Jan Kara]

On Tue 21-04-26 17:31:38, Junjie Cao wrote:
> ext4_read_inline_data() reads e_value_offs from the inode buffer_head on
> each call, but the decision to enter the xattr value path depends on
> i_inline_size cached in EXT4_I(inode) at iget time. If the buffer
> contents change after the initial validation, e_value_offs can point
> beyond the inode body while i_inline_size still directs the code into
> the xattr value path, causing an out-of-bounds read in the memcpy.
> 
> Add a bounds check before the memcpy, consistent with
> ext4_xattr_ibody_get(). Also guard folio_mark_uptodate() in
> ext4_read_inline_folio() since ext4_read_inline_data() can now return
> -EFSCORRUPTED.
> 
> Fixes: 67cf5b09a46f ("ext4: add the basic function for inline data support")
> Cc: stable@vger.kernel.org
> Reported-by: syzbot+26c4a8cab92d0cda3e3b@syzkaller.appspotmail.com
> Tested-by: syzbot+26c4a8cab92d0cda3e3b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=26c4a8cab92d0cda3e3b
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>

If the buffer contents changes after the initial validation, there is some
problem somewhere and this isn't going to fix it (likely the fs is
corrupted and that isn't properly detected). Please fix the real problem,
not just paper over it.

								Honza

> ---
>  fs/ext4/inline.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 408677fa8196..18c678df0a6e 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -211,6 +211,14 @@ static int ext4_read_inline_data(struct inode *inode, void *buffer,
>  	len = min_t(unsigned int, len,
>  		    (unsigned int)le32_to_cpu(entry->e_value_size));
>  
> +	if (unlikely((void *)IFIRST(header) + le16_to_cpu(entry->e_value_offs) +
> +		     len > (void *)ITAIL(inode, raw_inode))) {
> +		EXT4_ERROR_INODE(inode,
> +			"inline data value out of bounds (offs %u len %u)",
> +			le16_to_cpu(entry->e_value_offs), len);
> +		return -EFSCORRUPTED;
> +	}
> +
>  	memcpy(buffer,
>  	       (void *)IFIRST(header) + le16_to_cpu(entry->e_value_offs), len);
>  	cp_len += len;
> @@ -535,7 +543,8 @@ static int ext4_read_inline_folio(struct inode *inode, struct folio *folio)
>  	ret = ext4_read_inline_data(inode, kaddr, len, &iloc);
>  	kaddr = folio_zero_tail(folio, len, kaddr + len);
>  	kunmap_local(kaddr);
> -	folio_mark_uptodate(folio);
> +	if (ret >= 0)
> +		folio_mark_uptodate(folio);
>  	brelse(iloc.bh);
>  
>  out:
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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@suse.com>
SUSE Labs, CR

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

[Junjie Cao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 1021 bytes --]

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

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.

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?

Many thanks，
Junjie