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-next>] [day] [month] [year] [list]
Message-ID: <40fbcb1c-e35f-4310-a2d9-9932570cb245@gmx.com>
Date: Mon, 24 Jun 2024 16:11:21 +0930
From: Qu Wenruo <quwenruo.btrfs@....com>
To: Jeongjun Park <aha310510@...il.com>
Cc: "clm@...com" <clm@...com>, "dsterba@...e.com" <dsterba@...e.com>,
 "josef@...icpanda.com" <josef@...icpanda.com>,
 "syzbot+a0d1f7e26910be4dc171@...kaller.appspotmail.com"
 <syzbot+a0d1f7e26910be4dc171@...kaller.appspotmail.com>,
 "fdmanana@...e.com" <fdmanana@...e.com>,
 "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "syzkaller-bugs@...glegroups.com" <syzkaller-bugs@...glegroups.com>
Subject: Re: [PATCH] btrfs: qgroup: fix slab-out-of-bounds in
 btrfs_qgroup_inherit



在 2024/6/24 15:59, Jeongjun Park 写道:
> While debugging, I also found that a problem
> occurred in btrfs_qgroup_check_inherit().
> While debugging, I also found that a problem
> occurred in btrfs_qgroup_check_inherit().
>
> I think out-of-bounds can be prevented
> more effectively if the inspection logic
> containing btrfs_qgroup_enabled() is
> moved a little lower.
>
> If possible, we will send you the v2 patch.
> I think out-of-bounds can be prevented
> more effectively if the inspection logic
> containing btrfs_qgroup_enabled() is
> moved a little lower.
>
> I will send you the v2 patch later after work.

Mind to check my v2 patch?

https://lore.kernel.org/linux-btrfs/47d3dd33f637b70f230fa31f98dbf9ff066b58bb.1719207446.git.wqu@suse.com/T/#u

I believe this would be enough to prevent the bug from happening, with
all the existing checks in-place.

Thanks,
Qu

>
> Regards.
> Jeongjun Park.
>
> 2024년 6월 24일 월요일, Qu Wenruo <quwenruo.btrfs@....com
> <mailto:quwenruo.btrfs@....com>>님이 작성:
>
>
>
>     在 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/ <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
>             <mailto:syzbot+a0d1f7e26910be4dc171@...kaller.appspotmail.com>
>             Fixes: 3f5e2d3b3877 ("Btrfs: fix missing check in the
>             btrfs_qgroup_inherit()")
>             Signed-off-by: Jeongjun Park <aha310510@...il.com
>             <mailto: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;
>             --
>
>