| Message ID | 20210325092203.10251-1-qiang.zhang@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | loop: Fix use of unsafe lo->lo_mutex locks | expand |
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. > > Fixes: 6cc8e7430801 ("loop: scale loop device by introducing per device lock") > Signed-off-by: Zqiang <qiang.zhang@windriver.com> > --- > 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
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,