btrfs: qgroup: fix slab-out-of-bounds in btrfs_qgroup_inherit

Commit Message

Jeongjun Park June 24, 2024, 3:07 a.m. UTC

If a value exists in inherit->num_ref_copies or inherit->num_excl_copies,
an out-of-bounds vulnerability occurs.

Therefore, you need to add code to check the presence or absence of 
that value.

Regards.
Jeongjun Park.

Reported-by: [e-mail]
Fixes: 3f5e2d3b3877 ("Btrfs: fix missing check in the btrfs_qgroup_inherit()")
Signed-off-by: Jeongjun Park <[e-mail]>
---
 fs/btrfs/qgroup.c | 4 ++++
 1 file changed, 4 insertions(+)

--

Comments

Qu Wenruo June 24, 2024, 5:10 a.m. UTC | #1

在 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/[e-mail]/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.

Thanks,
Qu

> Therefore, you need to add code to check the presence or absence of
> that value.
>
> Regards.
> Jeongjun Park.
>
> Reported-by: [e-mail]
> Fixes: 3f5e2d3b3877 ("Btrfs: fix missing check in the btrfs_qgroup_inherit()")
> Signed-off-by: Jeongjun Park <[e-mail]>
> ---
>   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;
> --
>

Qu Wenruo June 24, 2024, 5:40 a.m. UTC | #2

在 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/[e-mail]/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: [e-mail]
>> Fixes: 3f5e2d3b3877 ("Btrfs: fix missing check in the
>> btrfs_qgroup_inherit()")
>> Signed-off-by: Jeongjun Park <[e-mail]>
>> ---
>>   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;
>> --
>>
>

Qu Wenruo June 24, 2024, 6:41 a.m. UTC | #3

在 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/[e-mail]/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 <[e-mail]
> <mailto:[e-mail]>>님이 작성:
>
>
>
>     在 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/[e-mail]/T/ <https://lore.kernel.org/lkml/[e-mail]/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:
>             [e-mail]
>             <mailto:[e-mail]>
>             Fixes: 3f5e2d3b3877 ("Btrfs: fix missing check in the
>             btrfs_qgroup_inherit()")
>             Signed-off-by: Jeongjun Park <[e-mail]
>             <mailto:[e-mail]>>
>             ---
>                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;
>             --
>
>

Jeongjun Park June 28, 2024, 1:21 a.m. UTC | #4

> 2024. 6. 24. 오후 3:41, Qu Wenruo <[e-mail]> 작성:
> 
> 
>> 在 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/[e-mail]/T/#u
> 
> I believe this would be enough to prevent the bug from happening, with
> all the existing checks in-place.
> 
> Thanks,
> Qu

Perfact Thanks.

Jeongjun Park

Patch

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;