在 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@xxxxxxxxxx>
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.
Cheers,
- Ted
.
