On 2025/2/7 20:34, Jan Kara wrote:
On Fri 07-02-25 17:31:49, yebin wrote:
On 2025/2/7 12:16, Darrick J. Wong wrote:
On Fri, Feb 07, 2025 at 11:27:43AM +0800, Ye Bin wrote:
From: Ye Bin <yebin10@xxxxxxxxxx>
There's issue as follows:
BUG: KASAN: use-after-free in ext4_xattr_inode_dec_ref_all+0x6ff/0x790
Read of size 4 at addr ffff88807b003000 by task syz-executor.0/15172
CPU: 3 PID: 15172 Comm: syz-executor.0
Call Trace:
__dump_stack lib/dump_stack.c:82 [inline]
dump_stack+0xbe/0xfd lib/dump_stack.c:123
print_address_description.constprop.0+0x1e/0x280 mm/kasan/report.c:400
__kasan_report.cold+0x6c/0x84 mm/kasan/report.c:560
kasan_report+0x3a/0x50 mm/kasan/report.c:585
ext4_xattr_inode_dec_ref_all+0x6ff/0x790 fs/ext4/xattr.c:1137
ext4_xattr_delete_inode+0x4c7/0xda0 fs/ext4/xattr.c:2896
ext4_evict_inode+0xb3b/0x1670 fs/ext4/inode.c:323
evict+0x39f/0x880 fs/inode.c:622
iput_final fs/inode.c:1746 [inline]
iput fs/inode.c:1772 [inline]
iput+0x525/0x6c0 fs/inode.c:1758
ext4_orphan_cleanup fs/ext4/super.c:3298 [inline]
ext4_fill_super+0x8c57/0xba40 fs/ext4/super.c:5300
mount_bdev+0x355/0x410 fs/super.c:1446
legacy_get_tree+0xfe/0x220 fs/fs_context.c:611
vfs_get_tree+0x8d/0x2f0 fs/super.c:1576
do_new_mount fs/namespace.c:2983 [inline]
path_mount+0x119a/0x1ad0 fs/namespace.c:3316
do_mount+0xfc/0x110 fs/namespace.c:3329
__do_sys_mount fs/namespace.c:3540 [inline]
__se_sys_mount+0x219/0x2e0 fs/namespace.c:3514
do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x67/0xd1
Memory state around the buggy address:
ffff88807b002f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff88807b002f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff88807b003000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
^
ffff88807b003080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88807b003100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
Above issue happens as ext4_xattr_delete_inode() isn't check xattr
is valid if xattr is in inode.
To solve above issue call xattr_check_inode() check if xattr if valid
in inode.
Fixes: e50e5129f384 ("ext4: xattr-in-inode support")
Signed-off-by: Ye Bin <yebin10@xxxxxxxxxx>
---
fs/ext4/xattr.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 0e4494863d15..cb724477f8da 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2922,7 +2922,6 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode,
int extra_credits)
{
struct buffer_head *bh = NULL;
- struct ext4_xattr_ibody_header *header;
struct ext4_iloc iloc = { .bh = NULL };
struct ext4_xattr_entry *entry;
struct inode *ea_inode;
@@ -2937,6 +2936,9 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode,
if (ext4_has_feature_ea_inode(inode->i_sb) &&
ext4_test_inode_state(inode, EXT4_STATE_XATTR)) {
+ struct ext4_xattr_ibody_header *header;
+ struct ext4_inode *raw_inode;
+ void *end;
error = ext4_get_inode_loc(inode, &iloc);
if (error) {
@@ -2952,14 +2954,20 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode,
goto cleanup;
}
- header = IHDR(inode, ext4_raw_inode(&iloc));
- if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC))
+ raw_inode = ext4_raw_inode(&iloc);
+ header = IHDR(inode, raw_inode);
+ end = ITAIL(inode, raw_inode);
+ if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)) {
This needs to make sure that header + sizeof(h_magic) >= end before
checking the magic number in header::h_magic, right?
--D
Thank you for your reply.
There ' s no need to check "header + sizeof(h_magic) >= end" because it has
been checked
when the EXT4_STATE_XATTR flag bit is set:
__ext4_iget
ret = ext4_iget_extra_inode(inode, raw_inode, ei);
if (EXT4_INODE_HAS_XATTR_SPACE(inode) && *magic ==
cpu_to_le32(EXT4_XATTR_MAGIC))
ext4_set_inode_state(inode, EXT4_STATE_XATTR);
It seems that the judgment of "header->h_magic ==
cpu_to_le32(EXT4_XATTR_MAGIC)"
should be redundant here.
Yes, if we have EXT4_STATE_XATTR set, xattr_check_inode() should be safe to
call (ext4_iget_extra_inode() makes sure inode space is sane) and should
return success. I'm actually wondering whether it wouldn't be better for
ext4_iget_extra_inode() to check xattr validity with xattr_check_inode()
along with setting EXT4_STATE_XATTR. So we'd be checking xattr validity
when loading from disk similarly as for xattr blocks which is a well
defined place. When doing the checking on use (as we do now) it is always
easy to miss some place...
Honza
It's actually better. I'll revise it and post it again.
+ error = xattr_check_inode(inode, header, end);
+ if (error)
+ goto cleanup;
ext4_xattr_inode_dec_ref_all(handle, inode, iloc.bh,
IFIRST(header),
false /* block_csum */,
ea_inode_array,
extra_credits,
false /* skip_quota */);
+ }
}
if (EXT4_I(inode)->i_file_acl) {
--
2.34.1
