linux-kernel - Re: [PATCH] btrfs: Fix NULL pointer access in btrfs_check_leaked

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] [day] [month] [year] [list]

Message-ID: <ec8854e3-dd79-4233-8f14-fbd55252e210@cqsoftware.com.cn>
Date: Thu, 16 Oct 2025 10:11:53 +0800
From: Dewei Meng <mengdewei@...oftware.com.cn>
To: Qu Wenruo <quwenruo.btrfs@....com>, clm@...com, dsterba@...e.com
Cc: linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
 josef@...icpanda.com
Subject: Re: [PATCH] btrfs: Fix NULL pointer access in
 btrfs_check_leaked_roots()


在 2025/10/15 18:07, Qu Wenruo 写道:
>
>
> 在 2025/10/15 20:32, Dewei Meng 写道:
>>
>> 在 2025/10/15 16:24, Qu Wenruo 写道:
>>>
>>>
>>> 在 2025/10/15 17:54, Dewei Meng 写道:
>>>> If fs_info->super_copy or fs_info->super_for_commit is NULL in
>>>> btrfs_get_tree_subvol(),
>>>
>>> Please reorganize this sentence. It would be way more easier to read 
>>> by just saying something like "If memory allocation failed for 
>>> fs_info->super_copy or fs_info->super_for_commit in 
>>> btrfs_get_tree_subvol()".
>> I agree, I will fix these words to make them easier to read.
>>>
>>>> the btrfs_check_leaked_roots() will get the
>>>> btrfs_root list entry using the fs_info->allocated_roots->next
>>>> which is NULL.
>>>>
>>>> syzkaller reported the following information:
>>>>    ------------[ cut here ]------------
>>>>    BUG: unable to handle page fault for address: fffffffffffffbb0
>>>>    #PF: supervisor read access in kernel mode
>>>>    #PF: error_code(0x0000) - not-present page
>>>>    PGD 64c9067 P4D 64c9067 PUD 64cb067 PMD 0
>>>>    Oops: Oops: 0000 [#1] SMP KASAN PTI
>>>>    CPU: 0 UID: 0 PID: 1402 Comm: syz.1.35 Not tainted 6.15.8 #4 
>>>> PREEMPT(lazy)
>>>>    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), (...)
>>>>    RIP: 0010:arch_atomic_read arch/x86/include/asm/atomic.h:23 
>>>> [inline]
>>>>    RIP: 0010:raw_atomic_read include/linux/atomic/atomic-arch- 
>>>> fallback.h:457 [inline]
>>>>    RIP: 0010:atomic_read include/linux/atomic/atomic- 
>>>> instrumented.h:33 [inline]
>>>>    RIP: 0010:refcount_read include/linux/refcount.h:170 [inline]
>>>>    RIP: 0010:btrfs_check_leaked_roots+0x18f/0x2c0 fs/btrfs/disk- 
>>>> io.c:1230
>>>>    [...]
>>>>    Call Trace:
>>>>     <TASK>
>>>>     btrfs_free_fs_info+0x310/0x410 fs/btrfs/disk-io.c:1280
>>>>     btrfs_get_tree_subvol+0x592/0x6b0 fs/btrfs/super.c:2029
>>>>     btrfs_get_tree+0x63/0x80 fs/btrfs/super.c:2097
>>>>     vfs_get_tree+0x98/0x320 fs/super.c:1759
>>>>     do_new_mount+0x357/0x660 fs/namespace.c:3899
>>>>     path_mount+0x716/0x19c0 fs/namespace.c:4226
>>>>     do_mount fs/namespace.c:4239 [inline]
>>>>     __do_sys_mount fs/namespace.c:4450 [inline]
>>>>     __se_sys_mount fs/namespace.c:4427 [inline]
>>>>     __x64_sys_mount+0x28c/0x310 fs/namespace.c:4427
>>>>     do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>>>>     do_syscall_64+0x92/0x180 arch/x86/entry/syscall_64.c:94
>>>>     entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>>>    RIP: 0033:0x7f032eaffa8d
>>>>    [...]
>>>>
>>>> This should check if the fs_info->allocated_roots->next is NULL before
>>>> accessing it.
>>>>
>>>> Fixes: 3bb17a25bcb0 ("btrfs: add get_tree callback for new mount API")
>>>> Signed-off-by: Dewei Meng <mengdewei@...oftware.com.cn>
>>>> ---
>>>>   fs/btrfs/disk-io.c | 3 +++
>>>>   1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>> index 0aa7e5d1b05f..76db7f98187a 100644
>>>> --- a/fs/btrfs/disk-io.c
>>>> +++ b/fs/btrfs/disk-io.c
>>>> @@ -1213,6 +1213,9 @@ void btrfs_check_leaked_roots(const struct 
>>>> btrfs_fs_info *fs_info)
>>>>   #ifdef CONFIG_BTRFS_DEBUG
>>>>       struct btrfs_root *root;
>>>>   +    if (!fs_info->allocated_roots.next)
>>>> +        return;
>>>> +
>>>
>>> The check looks too adhoc to me.
>>>
>>> It would be much easier to just call kvfree() in the error handling 
>>> of super_copy/super_for_commit allocation, we do not and should not 
>>> call btrfs_free_fs_info() before calling btrfs_init_fs_info().
>>
>> It is a good solution to fix this bug, or can we put the 
>> 'btrfs_init_fs_info(fs_info)' before super_copy/super_for_commit 
>> allocation?
>
> That also sounds fine to me.

I have tested the kfree method, and test case runs fine. I plan to take 
your suggestion and re-do the patch.

Thanks.

>
>>
>> Thanks,
>>
>> Dewei Meng
>>
>>>
>>> Thanks,
>>> Qu
>>>>       while (!list_empty(&fs_info->allocated_roots)) {
>>>>           char buf[BTRFS_ROOT_NAME_BUF_LEN];
>>>
>>>
>>>
>
>
>