[Cluster-devel] [PATCH] gfs2: Fix uaf for qda in gfs2_quota_sync

From: eadaivs <[email protected]>

Hello Andreas,

On Mon, 30 Jan 2023 15:32:42 +0100  <[email protected]> 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/[email protected]
Reported-and-tested-by: [email protected]
Signed-off-by: eadaivs <[email protected]>
---
 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(&qd_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 = &sdp->sd_quota_list;
-	struct gfs2_quota_data *qd;
+	struct gfs2_quota_data *qd, *safe;
 
+retry:
 	spin_lock(&qd_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->qd_list);
 
@@ -1482,7 +1487,10 @@ void gfs2_quota_cleanup(struct gfs2_sbd *sdp)
 	}
 	spin_unlock(&qd_lock);
 
-	gfs2_assert_warn(sdp, !atomic_read(&sdp->sd_quota_count));
+	if (atomic_read(&sdp->sd_quota_count)) {
+		schedule_timeout_uninterruptible(HZ);
+		goto retry;
+	}
 
 	kvfree(sdp->sd_quota_bitmap);
 	sdp->sd_quota_bitmap = NULL;
-- 
2.41.0