在 2022/10/20 3:40, Mike Snitzer 写道:
On Tue, Oct 18 2022 at 4:17P -0400,
Luo Meng <luomeng@xxxxxxxxxxxxxxx> wrote:
在 2022/10/18 3:38, Mike Snitzer 写道:
On Mon, Oct 10 2022 at 10:39P -0400,
Luo Meng <luomeng@xxxxxxxxxxxxxxx> wrote:
From: Luo Meng <luomeng12@xxxxxxxxxx>
When dm_resume() and dm_destroy() are concurrent, it will
lead to UAF.
One of the concurrency UAF can be shown as below:
use free
do_resume |
__find_device_hash_cell |
dm_get |
atomic_inc(&md->holders) |
| dm_destroy
| __dm_destroy
| if (!dm_suspended_md(md))
| atomic_read(&md->holders)
| msleep(1)
dm_resume |
__dm_resume |
dm_table_resume_targets |
pool_resume |
do_waker #add delay work |
| dm_table_destroy
| pool_dtr
| __pool_dec
| __pool_destroy
| destroy_workqueue
| kfree(pool) # free pool
time out
__do_softirq
run_timer_softirq # pool has already been freed
This can be easily reproduced using:
1. create thin-pool
2. dmsetup suspend pool
3. dmsetup resume pool
4. dmsetup remove_all # Concurrent with 3
The root cause of UAF bugs is that dm_resume() adds timer after
dm_destroy() skips cancel timer beause of suspend status. After
timeout, it will call run_timer_softirq(), however pool has already
been freed. The concurrency UAF bug will happen.
Therefore, canceling timer is moved after md->holders is zero.
Signed-off-by: Luo Meng <luomeng12@xxxxxxxxxx>
---
drivers/md/dm.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 60549b65c799..379525313628 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2420,6 +2420,19 @@ static void __dm_destroy(struct mapped_device *md, bool wait)
blk_mark_disk_dead(md->disk);
+ /*
+ * Rare, but there may be I/O requests still going to complete,
+ * for example. Wait for all references to disappear.
+ * No one should increment the reference count of the mapped_device,
+ * after the mapped_device state becomes DMF_FREEING.
+ */
+ if (wait)
+ while (atomic_read(&md->holders))
+ msleep(1);
+ else if (atomic_read(&md->holders))
+ DMWARN("%s: Forcibly removing mapped_device still in use! (%d users)",
+ dm_device_name(md), atomic_read(&md->holders));
+
/*
* Take suspend_lock so that presuspend and postsuspend methods
* do not race with internal suspend.
@@ -2436,19 +2449,6 @@ static void __dm_destroy(struct mapped_device *md, bool wait)
dm_put_live_table(md, srcu_idx);
mutex_unlock(&md->suspend_lock);
- /*
- * Rare, but there may be I/O requests still going to complete,
- * for example. Wait for all references to disappear.
- * No one should increment the reference count of the mapped_device,
- * after the mapped_device state becomes DMF_FREEING.
- */
- if (wait)
- while (atomic_read(&md->holders))
- msleep(1);
- else if (atomic_read(&md->holders))
- DMWARN("%s: Forcibly removing mapped_device still in use! (%d users)",
- dm_device_name(md), atomic_read(&md->holders));
-
dm_table_destroy(__unbind(md));
free_dev(md);
}
--
2.31.1
Thanks for the report but your fix seems wrong. A thin-pool specific
fix seems much more appropriate. Does this fix the issue?
diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index e76c96c760a9..dc271c107fb5 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -2889,6 +2889,8 @@ static void __pool_destroy(struct pool *pool)
dm_bio_prison_destroy(pool->prison);
dm_kcopyd_client_destroy(pool->copier);
+ cancel_delayed_work_sync(&pool->waker);
+ cancel_delayed_work_sync(&pool->no_space_timeout);
if (pool->wq)
destroy_workqueue(pool->wq);
Thanks for your reply.
However this issue exits not only in thin-pool, cache_target also has
thisissus. Generic fix maybe more appropriate.
No, you've pointed out a problem with dm-thinp not properly tearing
down its resources. Same for dm-cache. So a generic fix doesn't make
sense. And your particular generic fix merely papers over the
resource leak, while also causing DM core to no longer properly wait
for outstanding IO to complete.
After adding cancel_delayed_work_sync() in __pool_destroy(), this will make
it possible to call cancel_delayed_work_sync(&pool->waker) twice when dm is
not suspend.
That's not a problem, that's the cost of properly accounting for
resources where we must.
Thanks.
I will send v2 soon.
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
