btrfs: add a sanity check for csum root before fill the data csum

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

Posted by Edward Adam Davis 1 month ago

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 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) &&
+	    bg->flags & BTRFS_BLOCK_GROUP_DATA) {
 		int sector_nr;
 		unsigned long csum_bitmap = 0;
 
-- 
2.43.0

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

Posted by David Sterba 1 month ago

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>

Added to for-next, thanks.

> ---
>  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
>

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

Posted by Qu Wenruo 1 month ago


在 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@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f
>> Signed-off-by: Edward Adam Davis <eadavis@qq.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
>>
>

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

Posted by David Sterba 3 weeks, 6 days ago

On Sat, Oct 26, 2024 at 07:45:18AM +1030, Qu Wenruo wrote:
> 
> 
> 在 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@syzkaller.appspotmail.com
> >> Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f
> >> Signed-off-by: Edward Adam Davis <eadavis@qq.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.

Ok, I've dropped this patch from for-next. Please add "btrfs: add a
sanity check for btrfs root".

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

Posted by Qu Wenruo 1 month ago


在 2024/10/23 21:34, Edward Adam Davis 写道:
> 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

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) &&
> +	    bg->flags & BTRFS_BLOCK_GROUP_DATA) {
>   		int sector_nr;
>   		unsigned long csum_bitmap = 0;
>