linux-ext4 - Re: [PATCH 2/2] ext4: check magic even the extent block bh is verified

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2627cb7a-b552-4cf3-fabe-3600535329ff@huawei.com>
Date:   Fri, 8 Oct 2021 09:38:32 +0800
From:   yangerkun <yangerkun@...wei.com>
To:     Jan Kara <jack@...e.cz>
CC:     <tytso@....edu>, <linux-ext4@...r.kernel.org>, <yukuai3@...wei.com>
Subject: Re: [PATCH 2/2] ext4: check magic even the extent block bh is
 verified



在 2021/10/1 17:18, Jan Kara 写道:
> On Sat 04-09-21 12:49:46, yangerkun wrote:
>> Our stress testing with IO error can trigger follow OOB with a very low
>> probability.
>>
>> [59898.282466] BUG: KASAN: slab-out-of-bounds in ext4_find_extent+0x2e4/0x480
>> ...
>> [59898.287162] Call Trace:
>> [59898.287575]  dump_stack+0x8b/0xb9
>> [59898.288070]  print_address_description+0x73/0x280
>> [59898.289903]  ext4_find_extent+0x2e4/0x480
>> [59898.290553]  ext4_ext_map_blocks+0x125/0x1470
>> [59898.295481]  ext4_map_blocks+0x5ee/0x940
>> [59898.315984]  ext4_mpage_readpages+0x63c/0xdb0
>> [59898.320231]  read_pages+0xe6/0x370
>> [59898.321589]  __do_page_cache_readahead+0x233/0x2a0
>> [59898.321594]  ondemand_readahead+0x157/0x450
>> [59898.321598]  generic_file_read_iter+0xcb2/0x1550
>> [59898.328828]  __vfs_read+0x233/0x360
>> [59898.328840]  vfs_read+0xa5/0x190
>> [59898.330126]  ksys_read+0xa5/0x150
>> [59898.331405]  do_syscall_64+0x6d/0x1f0
>> [59898.331418]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> Digging deep and we found it's actually a xattr block which can happened
>> with follow steps:
>>
>> 1. extent update for file1 and will remove a leaf extent block(block A)
>> 2. we need update the idx extent block too
>> 3. block A has been allocated as a xattr block and will set verified
>> 3. io error happened for this idx block and will the buffer has been
>>     released late
>> 4. extent find for file1 will read the idx block and see block A again
>> 5. since the buffer of block A is already verified, we will use it
>>     directly, which can lead the upper OOB
>>
>> Same as __ext4_xattr_check_block, we can check magic even the buffer is
>> verified to fix the problem.
>>
>> Signed-off-by: yangerkun <yangerkun@...wei.com>
> 
> Honestly, I'm not sure if this is worth it. What you suggest will work if
> the magic is overwritten but if we reallocate the block for something else
> but the magic happens to stay intact, we have a problem. The filesystem is
> corrupted at that point with metadata blocks being multiply claimed and
> that's very difficult to deal with. Maybe we should start ignoring
> buffer_verified() bit once the fs is known to have errors and recheck the
> buffer contents on each access? Sure it will be slow but I have little
> sympathy towards people running filesystems with errors... What do people
> think?

What you means was that something like a extent block for inode A has
been reallocate as a extent block for inode B? Ignoring buffer_verified
seems useless for this case since extent check will pass. Maybe we
should first try to prevent the OOB...


> 
> 								Honza
> 
>> ---
>>   fs/ext4/extents.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 8559e288472f..d2e2ae90bc4a 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -506,6 +506,14 @@ __read_extent_tree_block(const char *function, unsigned int line,
>>   			goto errout;
>>   	}
>>   	if (buffer_verified(bh)) {
>> +		if (unlikely(ext_block_hdr(bh)->eh_magic != EXT4_EXT_MAGIC)) {
>> +			err = -EFSCORRUPTED;
>> +			ext4_error_inode(inode, function, line, 0,
>> +				"invalid magic for verified extent block %llu",
>> +				(unsigned long long)bh->b_blocknr);
>> +			goto errout;
>> +		}
>> +
>>   		if (!(flags & EXT4_EX_FORCE_CACHE))
>>   			return bh;
>>   	} else {
>> -- 
>> 2.31.1
>>