Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] loop: Don't change loop device under exclusive opener
@ 2019-05-16 14:01 Jan Kara
  2019-05-16 20:44 ` Jens Axboe
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Kara @ 2019-05-16 14:01 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Tetsuo Handa, linux-block, Jan Kara

Loop module allows calling LOOP_SET_FD while there are other openers of
the loop device. Even exclusive ones. This can lead to weird
consequences such as kernel deadlocks like:

mount_bdev()				lo_ioctl()
  udf_fill_super()
    udf_load_vrs()
      sb_set_blocksize() - sets desired block size B
      udf_tread()
        sb_bread()
          __bread_gfp(bdev, block, B)
					  loop_set_fd()
					    set_blocksize()
            - now __getblk_slow() indefinitely loops because B != bdev
              block size

Fix the problem by disallowing LOOP_SET_FD ioctl when there are
exclusive openers of a loop device.

[Deliberately chosen not to CC stable as a user with priviledges to
trigger this race has other means of taking the system down and this
has a potential of breaking some weird userspace setup]

Reported-and-tested-by: syzbot+10007d66ca02b08f0e60@syzkaller.appspotmail.com
Signed-off-by: Jan Kara <jack@suse.cz>
---
 drivers/block/loop.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Hi Jens!

What do you think about this patch? It fixes the problem but it also changes
user visible behavior so there are chances it breaks some existing setup
(although I have hard time coming up with a realistic scenario where it would
matter).

Alternatively we could change getblk() code handle changing block size. That
would fix the particular issue syzkaller found as well but I'm not sure what
else is broken when block device changes while fs driver is working with it.

Honza

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 102d79575895..f11b7dc16e9d 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -945,9 +945,20 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	if (!file)
 		goto out;
 
+	/*
+	 * If we don't hold exclusive handle for the device, upgrade to it
+	 * here to avoid changing device under exclusive owner.
+	 */
+	if (!(mode & FMODE_EXCL)) {
+		bdgrab(bdev);
+		error = blkdev_get(bdev, mode | FMODE_EXCL, loop_set_fd);
+		if (error)
+			goto out_putf;
+	}
+
 	error = mutex_lock_killable(&loop_ctl_mutex);
 	if (error)
-		goto out_putf;
+		goto out_bdev;
 
 	error = -EBUSY;
 	if (lo->lo_state != Lo_unbound)
@@ -1012,10 +1023,15 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	mutex_unlock(&loop_ctl_mutex);
 	if (partscan)
 		loop_reread_partitions(lo, bdev);
+	if (!(mode & FMODE_EXCL))
+		blkdev_put(bdev, mode | FMODE_EXCL);
 	return 0;
 
 out_unlock:
 	mutex_unlock(&loop_ctl_mutex);
+out_bdev:
+	if (!(mode & FMODE_EXCL))
+		blkdev_put(bdev, mode | FMODE_EXCL);
 out_putf:
 	fput(file);
 out:
-- 
2.16.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] loop: Don't change loop device under exclusive opener
  2019-05-16 14:01 [PATCH] loop: Don't change loop device under exclusive opener Jan Kara
@ 2019-05-16 20:44 ` Jens Axboe
  2019-05-27 12:29   ` Jan Kara
  0 siblings, 1 reply; 4+ messages in thread
From: Jens Axboe @ 2019-05-16 20:44 UTC (permalink / raw)
  To: Jan Kara; +Cc: Tetsuo Handa, linux-block

On 5/16/19 8:01 AM, Jan Kara wrote:
> Loop module allows calling LOOP_SET_FD while there are other openers of
> the loop device. Even exclusive ones. This can lead to weird
> consequences such as kernel deadlocks like:
> 
> mount_bdev()				lo_ioctl()
>   udf_fill_super()
>     udf_load_vrs()
>       sb_set_blocksize() - sets desired block size B
>       udf_tread()
>         sb_bread()
>           __bread_gfp(bdev, block, B)
> 					  loop_set_fd()
> 					    set_blocksize()
>             - now __getblk_slow() indefinitely loops because B != bdev
>               block size
> 
> Fix the problem by disallowing LOOP_SET_FD ioctl when there are
> exclusive openers of a loop device.
> 
> [Deliberately chosen not to CC stable as a user with priviledges to
> trigger this race has other means of taking the system down and this
> has a potential of breaking some weird userspace setup]
> 
> Reported-and-tested-by: syzbot+10007d66ca02b08f0e60@syzkaller.appspotmail.com
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  drivers/block/loop.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> Hi Jens!
> 
> What do you think about this patch? It fixes the problem but it also
> changes user visible behavior so there are chances it breaks some
> existing setup (although I have hard time coming up with a realistic
> scenario where it would matter).

I also have a hard time thinking about valid cases where this would be a
problem. I think, in the end, that fixing the issue is more important
than a potentially hypothetical use case.

> Alternatively we could change getblk() code handle changing block
> size. That would fix the particular issue syzkaller found as well but
> I'm not sure what else is broken when block device changes while fs
> driver is working with it.

I think your solution here is saner.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] loop: Don't change loop device under exclusive opener
  2019-05-16 20:44 ` Jens Axboe
@ 2019-05-27 12:29   ` Jan Kara
  2019-05-27 13:34     ` Jens Axboe
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Kara @ 2019-05-27 12:29 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Jan Kara, Tetsuo Handa, linux-block

On Thu 16-05-19 14:44:07, Jens Axboe wrote:
> On 5/16/19 8:01 AM, Jan Kara wrote:
> > Loop module allows calling LOOP_SET_FD while there are other openers of
> > the loop device. Even exclusive ones. This can lead to weird
> > consequences such as kernel deadlocks like:
> > 
> > mount_bdev()				lo_ioctl()
> >   udf_fill_super()
> >     udf_load_vrs()
> >       sb_set_blocksize() - sets desired block size B
> >       udf_tread()
> >         sb_bread()
> >           __bread_gfp(bdev, block, B)
> > 					  loop_set_fd()
> > 					    set_blocksize()
> >             - now __getblk_slow() indefinitely loops because B != bdev
> >               block size
> > 
> > Fix the problem by disallowing LOOP_SET_FD ioctl when there are
> > exclusive openers of a loop device.
> > 
> > [Deliberately chosen not to CC stable as a user with priviledges to
> > trigger this race has other means of taking the system down and this
> > has a potential of breaking some weird userspace setup]
> > 
> > Reported-and-tested-by: syzbot+10007d66ca02b08f0e60@syzkaller.appspotmail.com
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  drivers/block/loop.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > Hi Jens!
> > 
> > What do you think about this patch? It fixes the problem but it also
> > changes user visible behavior so there are chances it breaks some
> > existing setup (although I have hard time coming up with a realistic
> > scenario where it would matter).
> 
> I also have a hard time thinking about valid cases where this would be a
> problem. I think, in the end, that fixing the issue is more important
> than a potentially hypothetical use case.
> 
> > Alternatively we could change getblk() code handle changing block
> > size. That would fix the particular issue syzkaller found as well but
> > I'm not sure what else is broken when block device changes while fs
> > driver is working with it.
> 
> I think your solution here is saner.

Will you pick up the patch please? I cannot find it in your tree... Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] loop: Don't change loop device under exclusive opener
  2019-05-27 12:29   ` Jan Kara
@ 2019-05-27 13:34     ` Jens Axboe
  0 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2019-05-27 13:34 UTC (permalink / raw)
  To: Jan Kara; +Cc: Tetsuo Handa, linux-block

On 5/27/19 6:29 AM, Jan Kara wrote:
> On Thu 16-05-19 14:44:07, Jens Axboe wrote:
>> On 5/16/19 8:01 AM, Jan Kara wrote:
>>> Loop module allows calling LOOP_SET_FD while there are other openers of
>>> the loop device. Even exclusive ones. This can lead to weird
>>> consequences such as kernel deadlocks like:
>>>
>>> mount_bdev()				lo_ioctl()
>>>    udf_fill_super()
>>>      udf_load_vrs()
>>>        sb_set_blocksize() - sets desired block size B
>>>        udf_tread()
>>>          sb_bread()
>>>            __bread_gfp(bdev, block, B)
>>> 					  loop_set_fd()
>>> 					    set_blocksize()
>>>              - now __getblk_slow() indefinitely loops because B != bdev
>>>                block size
>>>
>>> Fix the problem by disallowing LOOP_SET_FD ioctl when there are
>>> exclusive openers of a loop device.
>>>
>>> [Deliberately chosen not to CC stable as a user with priviledges to
>>> trigger this race has other means of taking the system down and this
>>> has a potential of breaking some weird userspace setup]
>>>
>>> Reported-and-tested-by: syzbot+10007d66ca02b08f0e60@syzkaller.appspotmail.com
>>> Signed-off-by: Jan Kara <jack@suse.cz>
>>> ---
>>>   drivers/block/loop.c | 18 +++++++++++++++++-
>>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>> Hi Jens!
>>>
>>> What do you think about this patch? It fixes the problem but it also
>>> changes user visible behavior so there are chances it breaks some
>>> existing setup (although I have hard time coming up with a realistic
>>> scenario where it would matter).
>>
>> I also have a hard time thinking about valid cases where this would be a
>> problem. I think, in the end, that fixing the issue is more important
>> than a potentially hypothetical use case.
>>
>>> Alternatively we could change getblk() code handle changing block
>>> size. That would fix the particular issue syzkaller found as well but
>>> I'm not sure what else is broken when block device changes while fs
>>> driver is working with it.
>>
>> I think your solution here is saner.
> 
> Will you pick up the patch please? I cannot find it in your tree... Thanks!

Done!

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-16 14:01 [PATCH] loop: Don't change loop device under exclusive opener Jan Kara
2019-05-16 20:44 ` Jens Axboe
2019-05-27 12:29   ` Jan Kara
2019-05-27 13:34     ` Jens Axboe

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org linux-block@archiver.kernel.org
	public-inbox-index linux-block


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/ public-inbox