[PATCH] loop: Fix use of unsafe lo->lo_mutex locks
Subsystems:
block layer , the rest
1995d
4 messages,
2 authors,
2021-03-26 · open the first message on its own page
From: Zqiang <redacted>
lo->lo_refcnt = 0
CPU0 CPU1
lo_open() lo_open()
mutex_lock(&lo->lo_mutex)
atomic_inc(&lo->lo_refcnt)
lo_refcnt == 1
mutex_unlock(&lo->lo_mutex)
mutex_lock(&lo->lo_mutex)
atomic_inc(&lo->lo_refcnt)
lo_refcnt == 2
mutex_unlock(&lo->lo_mutex)
loop_clr_fd()
mutex_lock(&lo->lo_mutex)
atomic_read(&lo->lo_refcnt) > 1
lo->lo_flags |= LO_FLAGS_AUTOCLEAR lo_release()
mutex_unlock(&lo->lo_mutex)
return mutex_lock(&lo->lo_mutex)
atomic_dec_return(&lo->lo_refcnt)
lo_refcnt == 1
mutex_unlock(&lo->lo_mutex)
return
lo_release()
mutex_lock(&lo->lo_mutex)
atomic_dec_return(&lo->lo_refcnt)
lo_refcnt == 0
lo->lo_flags & LO_FLAGS_AUTOCLEAR
== true
mutex_unlock(&lo->lo_mutex) loop_control_ioctl()
case LOOP_CTL_REMOVE:
mutex_lock(&lo->lo_mutex)
atomic_read(&lo->lo_refcnt)==0
__loop_clr_fd(lo, true) mutex_unlock(&lo->lo_mutex)
mutex_lock(&lo->lo_mutex) loop_remove(lo)
mutex_destroy(&lo->lo_mutex)
...... kfree(lo)
UAF
When different tasks on two CPUs perform the above operations on the same
lo device, UAF may occur.
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
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.
quoted 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
________________________________________
发件人: 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?
Hi Pasha
I found the problem during the 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
agree it
...... 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.
I will modify it and resend it
Thanks
Qiang quoted 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
________________________________________
发件人: 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
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