linux-kernel - Re: [PATCH] btrfs: qgroup: fix slab-out-of-bounds in btrfs_qgroup

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: <c5646885-3368-4c49-9cbd-092d4b7b7551@gmx.com>
Date: Mon, 24 Jun 2024 15:10:41 +0930
From: Qu Wenruo <quwenruo.btrfs@....com>
To: Jeongjun Park <aha310510@...il.com>, clm@...com, dsterba@...e.com,
 josef@...icpanda.com
Cc: syzbot+a0d1f7e26910be4dc171@...kaller.appspotmail.com, fdmanana@...e.com,
 linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
 syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] btrfs: qgroup: fix slab-out-of-bounds in
 btrfs_qgroup_inherit



在 2024/6/24 14:40, Qu Wenruo 写道:
>
>
> 在 2024/6/24 12:37, Jeongjun Park 写道:
>> If a value exists in inherit->num_ref_copies or inherit->num_excl_copies,
>> an out-of-bounds vulnerability occurs.
>>
>
> Thanks for the fix.
>
> Although I'm still not 100% sure what's going wrong.
>
> The original report
> (https://lore.kernel.org/lkml/000000000000bc19ba061a67ca77@google.com/T/)
> is showing a backtrace when creating snapshot.
>
> In that case they should all go through __btrfs_ioctl_snap_create(), and
> since it has qgroup_inherit, it can only come from
> btrfs_ioctl_snap_create_v2().
>
> But in that function, we have just called btrfs_qgroup_check_inherit()
> function and it already has the check on num_ref_copies/num_excl_copies.
>
> So in that case it should not even happen.
>
> I think the root cause is why the existing btrfs_qgroup_check_inherit()
> doesn't catch the problem in the first place.

OK, the root cause is the qgroup enable/disable race and delayed
snapshot creation. So that we can have a btrfs_qgroup_inherit structure
passed in with qgroup disabled.

But at transaction commitment, the qgroup is enabled, so some unchecked
inherit structure is passed in.

In that case, the added check is not strong enough (lacks the structure
size and flags checks etc).

A better fix would be only let btrfs_qgroup_check_inherit() to skip the
source qgroup checks.

I'll send a fix using the findings above.

Thanks,
Qu

>
> Thanks,
> Qu
>
>> Therefore, you need to add code to check the presence or absence of
>> that value.
>>
>> Regards.
>> Jeongjun Park.
>>
>> Reported-by: syzbot+a0d1f7e26910be4dc171@...kaller.appspotmail.com
>> Fixes: 3f5e2d3b3877 ("Btrfs: fix missing check in the
>> btrfs_qgroup_inherit()")
>> Signed-off-by: Jeongjun Park <aha310510@...il.com>
>> ---
>>   fs/btrfs/qgroup.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/btrfs/qgroup.c b /fs/btrfs/qgroup.c
>> index fc2a7ea26354..23beac746637 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -3270,6 +3270,10 @@ int btrfs_qgroup_inherit(struct
>> btrfs_trans_handle *trans, u64 srcid,
>>       }
>>
>>       if (inherit) {
>> +        if (inherit->num_ref_copies > 0 || inherit->num_excl_copies >
>> 0) {
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>>           i_qgroups = (u64 *)(inherit + 1);
>>           nums = inherit->num_qgroups + 2 * inherit->num_ref_copies +
>>                  2 * inherit->num_excl_copies;
>> --
>>
>