| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <67A6B8CC.8000809@huaweicloud.com>
Date: Sat, 8 Feb 2025 09:52:12 +0800
From: yebin <yebin@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: "Darrick J. Wong" <djwong@...nel.org>, tytso@....edu,
adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org
Subject: Re: [RESEND PATCH 2/2] ext4: fix out-of-bound read in
ext4_xattr_inode_dec_ref_all()
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@...wei.com>
>>>>
>>>> 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@...wei.com>
>>>> ---
>>>> 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
>>>>
>>>>
>>>
>>
Powered by blists - more mailing lists