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

[yangerkun <yangerkun@huawei.com>]

在 2021/10/14 15:21, yangerkun 写道:
> 
> 在 2021/10/1 22:09, Theodore Ts'o 写道:
>> On Fri, Oct 01, 2021 at 11:18:33AM +0200, Jan Kara wrote:
>>>>
>>>> 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@huawei.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?
>>
>> At some point, if we transition away from using buffer_heads for the
>> jbd2 layer, and use our own ext4_metadata_buf structure which
>> incorporates the journal_head and buffer_head fields, this will allow
>> us to control our own writeback, and allow us to have our own error
>> callbacks so we can do things like declare an inode to be bad and not
>> to be referenced again.  This would allow us to have a metadata type
>> field, so we could know that a buffer had been verified as an inode
>> table block, or bitmap block, or an xattr block.
>>
>> However, I think the bigger issue is that even if we had a metadata
>> type field in the buffer_head (or ext4_metadata_buf), we should be
>> using the metadata validation, and buffer_verified bit, as a backup.
>> It should not be the primary line of defense.
>>
>> So what I would suggest doing is preventing the out of bounds
>> reference in ext4_find_extent() in the first place.  I note we're not
>> sanity checking the values of EXT4_{FIRST,LAST}_{EXTENT,INDEX} used in
>> ext4_ext_binsearch() and ext4_ext_binsearch_idx(), and that's probably
>> how we triggered the out of bounds read in the first place.  The cost
>> of making sure that pointers returned by
>> EXT4_{FIRST,LAST}_{EXTENT,INDEX} don't exceed the bounds of the extent
>> tree node would be minimal, and it would be an additional cross check
>> which would protect us against the buffer getting corrupted while in
>> memory (bit flips, or wild pointer dereferences).
> 
> Sorry for the latter replay.
> 
> This can prevent a corrupt extent block buffer(maybe a xattr block for 
> another file) with verified trigger the OOB. But once corrupt data in 
> extent block buffer won't trigger OOB. We pass the check and will use a 
> xattr block's data as a extent block. This may trigger other 
> unpredictable result...
> 
> The patch I send check the magic to ensure the block is really a extent 
> block which prevent this case. But for the case a extent block been 
> reallocated as another file's extent block. This seems useless and will 
> lead to some problem too. But we may first stop the unpredictable result 
> like the OOB or other error.

Hi,

Does there some advise for this...

Thanks,
Kun.

> 
>>
>> Cheers,
>>
>>                         - Ted
>> .
>>
> .