linux-kernel - [PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20221129203400.1456100-1-longman@redhat.com>
Date:   Tue, 29 Nov 2022 15:34:00 -0500
From:   Waiman Long <longman@...hat.com>
To:     Jens Axboe <axboe@...nel.dk>, Tejun Heo <tj@...nel.org>
Cc:     cgroups@...r.kernel.org, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Ming Lei <ming.lei@...hat.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michal Koutný <mkoutny@...e.com>,
        Hillf Danton <hdanton@...a.com>,
        Chaitanya Kulkarni <chaitanyak@...dia.com>,
        Bart Van Assche <bvanassche@....org>,
        Josef Bacik <josef@...icpanda.com>,
        Waiman Long <longman@...hat.com>,
        Yi Zhang <yi.zhang@...hat.com>
Subject: [PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

Commit 59b57717fff8 ("blkcg: delay blkg destruction until after
writeback has finished") delayed call to blkcg_destroy_blkgs() to
cgwb_release_workfn(). However, it is done after a css_put() of blkcg
which may be the final put that causes the blkcg to be freed as RCU
read lock isn't held.

By adding a css_tryget() into blkcg_destroy_blkgs() and warning its
failure, the following stack trace was produced in a test system on
bootup.

[   34.254240] RIP: 0010:blkcg_destroy_blkgs+0x16a/0x1a0
      :
[   34.339943] Call Trace:
[   34.342395]  <TASK>
[   34.344510]  blkcg_unpin_online+0x38/0x60
[   34.348523]  cgwb_release_workfn+0x6a/0x200
[   34.352708]  process_one_work+0x1e5/0x3b0
[   34.356742]  ? rescuer_thread+0x390/0x390
[   34.360758]  worker_thread+0x50/0x3a0
[   34.364425]  ? rescuer_thread+0x390/0x390
[   34.368447]  kthread+0xd9/0x100
[   34.371592]  ? kthread_complete_and_exit+0x20/0x20
[   34.376386]  ret_from_fork+0x22/0x30
[   34.379982]  </TASK>

This confirms that a potential UAF situation can happen.

Fix that by delaying the css_put() until after the blkcg_unpin_online()
call. Also use css_tryget() in blkcg_destroy_blkgs() and issue a warning
if css_tryget() fails with no RCU read lock held.

The reproducing system can no longer produce a warning with this patch.
All the runnable block/0* tests including block/027 were run successfully
without failure.

Fixes: 59b57717fff8 ("blkcg: delay blkg destruction until after writeback has finished")
Suggested-by: Michal Koutný <mkoutny@...e.com>
Reported-by: Yi Zhang <yi.zhang@...hat.com>
Signed-off-by: Waiman Long <longman@...hat.com>
---
 block/blk-cgroup.c | 10 +++++++++-
 mm/backing-dev.c   |  8 ++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 57941d2a8ba3..904372bb96f1 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
 
 	might_sleep();
 
-	css_get(&blkcg->css);
+	/*
+	 * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
+	 * references gone and rcu_read_lock not held.
+	 */
+	if (!css_tryget(&blkcg->css)) {
+		WARN_ON_ONCE(!rcu_read_lock_held());
+		return;
+	}
+
 	spin_lock_irq(&blkcg->lock);
 	while (!hlist_empty(&blkcg->blkg_list)) {
 		struct blkcg_gq *blkg = hlist_entry(blkcg->blkg_list.first,
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c30419a5e119..36f75b072325 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -390,11 +390,15 @@ static void cgwb_release_workfn(struct work_struct *work)
 	wb_shutdown(wb);
 
 	css_put(wb->memcg_css);
-	css_put(wb->blkcg_css);
 	mutex_unlock(&wb->bdi->cgwb_release_mutex);
 
-	/* triggers blkg destruction if no online users left */
+	/*
+	 * Triggers blkg destruction if no online users left
+	 * The final blkcg css_put() has to be done after blkcg_unpin_online()
+	 * to avoid use-after-free.
+	 */
 	blkcg_unpin_online(wb->blkcg_css);
+	css_put(wb->blkcg_css);
 
 	fprop_local_destroy_percpu(&wb->memcg_completions);
 
-- 
2.31.1