Re: [PATCH] btrfs: qgroup: fix slab-out-of-bounds in btrfs_qgroup_inherit

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 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@gmx.com
> <mailto:quwenruo.btrfs@gmx.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@syzkaller.appspotmail.com
>             <mailto:syzbot+a0d1f7e26910be4dc171@syzkaller.appspotmail.com>
>             Fixes: 3f5e2d3b3877 ("Btrfs: fix missing check in the
>             btrfs_qgroup_inherit()")
>             Signed-off-by: Jeongjun Park <aha310510@gmail.com
>             <mailto:aha310510@gmail.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;
>             --
>
>