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.
Regards.
Jeongjun Park.
2024년 6월 24일 월요일, Qu Wenruo <quwenruo.btrfs@xxxxxxx
<mailto:quwenruo.btrfs@xxxxxxx>>님이 작성:
在 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@xxxxxxxxxx/T/ <https://lore.kernel.org/lkml/000000000000bc19ba061a67ca77@xxxxxxxxxx/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@xxxxxxxxxxxxxxxxxxxxxxxxx
<mailto:syzbot+a0d1f7e26910be4dc171@xxxxxxxxxxxxxxxxxxxxxxxxx>
Fixes: 3f5e2d3b3877 ("Btrfs: fix missing check in the
btrfs_qgroup_inherit()")
Signed-off-by: Jeongjun Park <aha310510@xxxxxxxxx
<mailto:aha310510@xxxxxxxxx>>
---
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;
--