________________________________________
发件人: Pavel Tatashin <pasha.tatas...@soleen.com>
发送时间: 2021年3月25日 21:09
收件人: Zhang, Qiang
抄送: Jens Axboe; linux-bl...@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
>
> Fixes: 6cc8e7430801 ("loop: scale loop device by introducing per device lock")
> Signed-off-by: Zqiang <qiang.zh...@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.tatas...@soleen.com>
>
>Thank you,
>Pasha
