回复: [PATCH] loop: Fix use of unsafe lo->lo_mutex locks
From: Zhang, Qiang <hidden>
Date: 2021-03-26 09:37:46
Also in:
lkml
________________________________________ 发件人: Pavel Tatashin [off-list ref] 发送时间: 2021年3月25日 21:09 收件人: Zhang, Qiang 抄送: Jens Axboe; linux-block@vger.kernel.org; LKML 主题: Re: [PATCH] loop: Fix use of unsafe lo->lo_mutex locks [Please note: This e-mail is from an EXTERNAL e-mail address]
Hi Qiang,
Thank you for root causing this issue. Did you encounter this issue >or
found by inspection?
I would change the title to what actually being changed, something >like:
loop: call __loop_clr_fd() with lo_mutex locked to avoid autoclear >race
...... kfree(lo)
UAF
When different tasks on two CPUs perform the above operations on the same
lo device, UAF may occur.
Please also explain the fix:
Do not drop lo->lo_mutex before calling __loop_clr_fd(), so refcnt >and
LO_FLAGS_AUTOCLEAR check in lo_release stay in sync.Sorry Pasha, please Ignore I sent v2 patch. In lo_release() , we set lo->lo_state = Lo_rundown In loop_control_ioctl(), LOOP_CTL_REMOVE: if (lo->lo_state != Lo_unbound) is true will return, not call loop_remove(). I'm sorry to mislead you. Thanks Qiang
quoted hunk ↗ jump to hunk
Fixes: 6cc8e7430801 ("loop: scale loop device by introducing per device lock") Signed-off-by: Zqiang <redacted> --- drivers/block/loop.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)diff --git a/drivers/block/loop.c b/drivers/block/loop.c index d58d68f3c7cd..5712f1698a66 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c@@ -1201,7 +1201,6 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) bool partscan = false; int lo_number; - mutex_lock(&lo->lo_mutex); if (WARN_ON_ONCE(lo->lo_state != Lo_rundown)) { err = -ENXIO; goto out_unlock;@@ -1257,7 +1256,6 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) lo_number = lo->lo_number; loop_unprepare_queue(lo); out_unlock: - mutex_unlock(&lo->lo_mutex); if (partscan) { /* * bd_mutex has been held already in release path, so don't@@ -1288,12 +1286,11 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) * protects us from all the other places trying to change the 'lo' * device. */ - mutex_lock(&lo->lo_mutex); + lo->lo_flags = 0; if (!part_shift) lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN; lo->lo_state = Lo_unbound; - mutex_unlock(&lo->lo_mutex); /* * Need not hold lo_mutex to fput backing file. Calling fput holding@@ -1332,9 +1329,10 @@ static int loop_clr_fd(struct loop_device *lo) return 0; } lo->lo_state = Lo_rundown; + err = __loop_clr_fd(lo, false); mutex_unlock(&lo->lo_mutex); - return __loop_clr_fd(lo, false); + return err; } static int@@ -1916,13 +1914,12 @@ static void lo_release(struct gendisk *disk, fmode_t mode) if (lo->lo_state != Lo_bound) goto out_unlock; lo->lo_state = Lo_rundown; - mutex_unlock(&lo->lo_mutex); /* * In autoclear mode, stop the loop thread * and remove configuration after last close. */ __loop_clr_fd(lo, true); - return; + goto out_unlock; } else if (lo->lo_state == Lo_bound) { /* * Otherwise keep thread (if running) and config, --2.17.1
LGTM Reviewed-by: Pavel Tatashin <pasha.tatashin@soleen.com> Thank you, Pasha