Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210325092203.10251-1-qiang.zhang@windriver.com>
In-Reply-To: <20210325092203.10251-1-qiang.zhang@windriver.com>
From:   Pavel Tatashin <pasha.tatashin@soleen.com>
Date:   Thu, 25 Mar 2021 09:09:39 -0400
Message-ID: <CA+CK2bCS9Ab+F9gzGEJejW6J3R26j1JNBOV3tvcR6+fz2uRfpw@mail.gmail.com>
Subject: Re: [PATCH] loop: Fix use of unsafe lo->lo_mutex locks
To:     qiang.zhang@windriver.com
Cc:     Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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