linux-kernel - Re: [PATCH] btrfs: add a sanity check for csum root before fill the data csum

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: <09b446c0-0c47-4822-b14f-5df1e7e4f4de@gmx.com>
Date: Sat, 26 Oct 2024 07:45:18 +1030
From: Qu Wenruo <quwenruo.btrfs@....com>
To: dsterba@...e.cz, Edward Adam Davis <eadavis@...com>
Cc: syzbot+5d2b33d7835870519b5f@...kaller.appspotmail.com, clm@...com,
 dsterba@...e.com, josef@...icpanda.com, linux-btrfs@...r.kernel.org,
 linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] btrfs: add a sanity check for csum root before fill the
 data csum



在 2024/10/26 05:14, David Sterba 写道:
> On Wed, Oct 23, 2024 at 07:04:40PM +0800, Edward Adam Davis wrote:
>> Syzbot reported a null-ptr-deref in btrfs_lookup_csums_bitmap.
>> The btrfs info contains IGNOREDATACSUMS, which prevents the csum root from
>> being loaded.
>> Before filling in the csum data, check the flag BTRFS_FS_STATE_NO_DATA_CSUMS
>> to confirm that the csum root has been loaded.
>>
>> Reported-and-tested-by: syzbot+5d2b33d7835870519b5f@...kaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f
>> Signed-off-by: Edward Adam Davis <eadavis@...com>
>
> Added to for-next, thanks.

Wait for a second, I believe LiZhi Xu's solution is better.

And sorry I didn't notice that until his patch is submitted.

The problem for this fix is, although it fixes the crash, it also gives
a false feel of safety that scrub is finding nothing wrong.

But the truth is, there is no csum root, and everything can go wrong.

Thus I'd prefer LiZhi's solution which error out and terminate the scrub
immediately.

Thanks,
Qu
>
>> ---
>>   fs/btrfs/scrub.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>> index 3a3427428074..1ba4d8ba902b 100644
>> --- a/fs/btrfs/scrub.c
>> +++ b/fs/btrfs/scrub.c
>> @@ -1602,7 +1602,8 @@ static int scrub_find_fill_first_stripe(struct btrfs_block_group *bg,
>>   	}
>>
>>   	/* Now fill the data csum. */
>> -	if (bg->flags & BTRFS_BLOCK_GROUP_DATA) {
>> +	if (!test_bit(BTRFS_FS_STATE_NO_DATA_CSUMS, &fs_info->fs_state) &&
>
> I've updatd the coment as this is double negation that could be
> confusing on a quick read.
>
>> +	    bg->flags & BTRFS_BLOCK_GROUP_DATA) {
>>   		int sector_nr;
>>   		unsigned long csum_bitmap = 0;
>>
>> --
>> 2.43.0
>>
>