[Cluster-devel] [PATCH] gfs2: Fix uaf for qda in gfs2_quota_sync
From: eadaivs
Hello Andreas,
On Mon, 30 Jan 2023 15:32:42 +0100 wrote:
> I can see that there is a problem in the gfs2 quota code, but this
> unfortunately doesn't look like an acceptable fix. A better approach
> would be to use proper reference counting for gfs2_quota_data objects.
> In this case, gfs2_quota_sync() is still holding a reference, so the
> underlying object shouldn't be freed.
>
> Fixing this properly will require more than a handful of lines.
Before clearing quota, wait for the qd synchronization operation to end.
Add reference count in gfs2_quota_data and perform qd synchronization
to control whether qd is in use. If so, temporarily clear quota.
Link: https://lore.kernel.org/all/2b5e2405f14e8...@google.com
Reported-and-tested-by: syzbot+3f6a670108ce43356...@syzkaller.appspotmail.com
Signed-off-by: eadaivs
---
fs/gfs2/incore.h | 1 +
fs/gfs2/quota.c | 16
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index c26765080f28..61549bd95b32 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -463,6 +463,7 @@ struct gfs2_quota_data {
u64 qd_sync_gen;
unsigned long qd_last_warn;
struct rcu_head qd_rcu;
+ unsigned int ref_cnt;
};
enum {
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 1ed17226d9ed..dca4be078ce8 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -478,6 +478,7 @@ static int qd_fish(struct gfs2_sbd *sdp, struct
gfs2_quota_data **qdp)
qd_put(qd);
return error;
}
+ WRITE_ONCE(qd->ref_cnt, READ_ONCE(qd->ref_cnt) + 1);
}
*qdp = qd;
@@ -493,6 +494,7 @@ static void qd_unlock(struct gfs2_quota_data *qd)
bh_put(qd);
slot_put(qd);
qd_put(qd);
+ WRITE_ONCE(qd->ref_cnt, READ_ONCE(qd->ref_cnt) - 1);
}
static int qdsb_get(struct gfs2_sbd *sdp, struct kqid qid,
@@ -1422,6 +1424,7 @@ int gfs2_quota_init(struct gfs2_sbd *sdp)
qd->qd_change = qc_change;
qd->qd_slot = slot;
qd->qd_slot_count = 1;
+ WRITE_ONCE(qd->ref_cnt, 0);
spin_lock(_lock);
BUG_ON(test_and_set_bit(slot, sdp->sd_quota_bitmap));
@@ -1454,11 +1457,13 @@ int gfs2_quota_init(struct gfs2_sbd *sdp)
void gfs2_quota_cleanup(struct gfs2_sbd *sdp)
{
struct list_head *head = >sd_quota_list;
- struct gfs2_quota_data *qd;
+ struct gfs2_quota_data *qd, *safe;
+retry:
spin_lock(_lock);
- while (!list_empty(head)) {
- qd = list_last_entry(head, struct gfs2_quota_data, qd_list);
+ list_for_each_entry_safe(qd, safe, head, qd_list) {
+ if (READ_ONCE(qd->ref_cnt))
+ continue;
list_del(>qd_list);
@@ -1482,7 +1487,10 @@ void gfs2_quota_cleanup(struct gfs2_sbd *sdp)
}
spin_unlock(_lock);
- gfs2_assert_warn(sdp, !atomic_read(>sd_quota_count));
+ if (atomic_read(>sd_quota_count)) {
+ schedule_timeout_uninterruptible(HZ);
+ goto retry;
+ }
kvfree(sdp->sd_quota_bitmap);
sdp->sd_quota_bitmap = NULL;
--
2.41.0
[Cluster-devel] [PATCH] gfs2: Fix uaf for qda in gfs2_quota_sync
From: Edward Adam Davis [ 81.372851][ T5532] CPU: 1 PID: 5532 Comm: syz-executor.0 Not tainted 6.2.0-rc1-syzkaller-dirty #0 [ 81.382080][ T5532] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/12/2023 [ 81.392343][ T5532] Call Trace: [ 81.395654][ T5532] [ 81.398603][ T5532] dump_stack_lvl+0x1b1/0x290 [ 81.418421][ T5532] gfs2_assert_warn_i+0x19a/0x2e0 [ 81.423480][ T5532] gfs2_quota_cleanup+0x4c6/0x6b0 [ 81.428611][ T5532] gfs2_make_fs_ro+0x517/0x610 [ 81.457802][ T5532] gfs2_withdraw+0x609/0x1540 [ 81.481452][ T5532] gfs2_inode_refresh+0xb2d/0xf60 [ 81.506658][ T5532] gfs2_instantiate+0x15e/0x220 [ 81.511504][ T5532] gfs2_glock_wait+0x1d9/0x2a0 [ 81.516352][ T5532] do_sync+0x485/0xc80 [ 81.554943][ T5532] gfs2_quota_sync+0x3da/0x8b0 [ 81.559738][ T5532] gfs2_sync_fs+0x49/0xb0 [ 81.564063][ T5532] sync_filesystem+0xe8/0x220 [ 81.568740][ T5532] generic_shutdown_super+0x6b/0x310 [ 81.574112][ T5532] kill_block_super+0x79/0xd0 [ 81.578779][ T5532] deactivate_locked_super+0xa7/0xf0 [ 81.584064][ T5532] cleanup_mnt+0x494/0x520 [ 81.593753][ T5532] task_work_run+0x243/0x300 [ 81.608837][ T5532] exit_to_user_mode_loop+0x124/0x150 [ 81.614232][ T5532] exit_to_user_mode_prepare+0xb2/0x140 [ 81.619820][ T5532] syscall_exit_to_user_mode+0x26/0x60 [ 81.625287][ T5532] do_syscall_64+0x49/0xb0 [ 81.629710][ T5532] entry_SYSCALL_64_after_hwframe+0x63/0xcd [ 81.636292][ T5532] RIP: 0033:0x7efdd688d517 [ 81.640728][ T5532] Code: ff ff ff f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 31 f6 e9 09 00 00 00 66 0f 1f 84 00 00 00 00 00 b8 a6 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48 [ 81.660550][ T5532] RSP: 002b:7fff34520ce8 EFLAGS: 0246 ORIG_RAX: 00a6 [ 81.669413][ T5532] RAX: RBX: RCX: 7efdd688d517 [ 81.677403][ T5532] RDX: 7fff34520db9 RSI: 000a RDI: 7fff34520db0 [ 81.685388][ T5532] RBP: 7fff34520db0 R08: R09: 7fff34520b80 [ 81.695973][ T5532] R10: 55ca38b3 R11: 0246 R12: 7efdd68e6b24 [ 81.704152][ T5532] R13: 7fff34521e70 R14: 55ca3810 R15: 7fff34521eb0 [ 81.712868][ T5532] The function "gfs2_quota_cleanup()" may be called in the function "do_sync()", This will cause the qda obtained in the function "qd_check_sync" to be released, resulting in the occurrence of uaf. In order to avoid this uaf, we can increase the judgment of "sdp->sd_quota_bitmap" released in the function "gfs2_quota_cleanup" to confirm that "sdp->sd_quota_list" has been released. Link: https://lore.kernel.org/all/2b5e2405f14e8...@google.com Reported-and-tested-by: syzbot+3f6a670108ce43356...@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis --- fs/gfs2/quota.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c index 1ed1722..4cf66bd 100644 --- a/fs/gfs2/quota.c +++ b/fs/gfs2/quota.c @@ -1321,6 +1321,9 @@ int gfs2_quota_sync(struct super_block *sb, int type) qda[x]->qd_sync_gen = sdp->sd_quota_sync_gen; + if (!sdp->sd_quota_bitmap) + break; + for (x = 0; x < num_qd; x++) qd_unlock(qda[x]); } -- 2.39.0
