lkml.org 
[lkml]   [2021]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] loop: Fix use of unsafe lo->lo_mutex locks
    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

    \
     
     \ /
      Last update: 2021-03-25 14:11    [W:3.567 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site