Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 3ae3d167 authored by Tetsuo Handa's avatar Tetsuo Handa Committed by Greg Kroah-Hartman
Browse files

block/loop: Use global lock for ioctl() operation. 

commit 310ca162d779efee8a2dc3731439680f3e9c1e86 upstream.

syzbot is reporting NULL pointer dereference [1] which is caused by
race condition between ioctl(loop_fd, LOOP_CLR_FD, 0) versus
ioctl(other_loop_fd, LOOP_SET_FD, loop_fd) due to traversing other
loop devices at loop_validate_file() without holding corresponding
lo->lo_ctl_mutex locks.

Since ioctl() request on loop devices is not frequent operation, we don't
need fine grained locking. Let's use global lock in order to allow safe
traversal at loop_validate_file().

Note that syzbot is also reporting circular locking dependency between
bdev->bd_mutex and lo->lo_ctl_mutex [2] which is caused by calling
blkdev_reread_part() with lock held. This patch does not address it.

[1] https://syzkaller.appspot.com/bug?id=f3cfe26e785d85f9ee259f385515291d21bd80a3
[2] https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889



Signed-off-by: default avatarTetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: default avatarsyzbot <syzbot+bf89c128e05dd6c62523@syzkaller.appspotmail.com>
Reviewed-by: default avatarJan Kara <jack@suse.cz>
Signed-off-by: default avatarJan Kara <jack@suse.cz>
Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent 02862eb9

drivers/block/loop.c

+21 −21
Original line number Diff line number Diff line
@@ -82,6 +82,7 @@ 


static DEFINE_IDR(loop_index_idr);

static DEFINE_MUTEX(loop_index_mutex);

static DEFINE_MUTEX(loop_ctl_mutex);



static int max_part;

static int part_shift;
@@ -1033,7 +1034,7 @@ static int loop_clr_fd(struct loop_device *lo) 
	 */

	if (atomic_read(&lo->lo_refcnt) > 1) {

		lo->lo_flags |= LO_FLAGS_AUTOCLEAR;

		mutex_unlock(&lo->lo_ctl_mutex);

		mutex_unlock(&loop_ctl_mutex);

		return 0;

	}
@@ -1082,12 +1083,12 @@ static int loop_clr_fd(struct loop_device *lo) 
	if (!part_shift)

		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;

	loop_unprepare_queue(lo);

	mutex_unlock(&lo->lo_ctl_mutex);

	mutex_unlock(&loop_ctl_mutex);

	/*

	 * Need not hold lo_ctl_mutex to fput backing file.

	 * Calling fput holding lo_ctl_mutex triggers a circular

	 * Need not hold loop_ctl_mutex to fput backing file.

	 * Calling fput holding loop_ctl_mutex triggers a circular

	 * lock dependency possibility warning as fput can take

	 * bd_mutex which is usually taken before lo_ctl_mutex.

	 * bd_mutex which is usually taken before loop_ctl_mutex.

	 */

	fput(filp);

	return 0;
@@ -1350,7 +1351,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode, 
	struct loop_device *lo = bdev->bd_disk->private_data;

	int err;



	mutex_lock_nested(&lo->lo_ctl_mutex, 1);

	mutex_lock_nested(&loop_ctl_mutex, 1);

	switch (cmd) {

	case LOOP_SET_FD:

		err = loop_set_fd(lo, mode, bdev, arg);
@@ -1359,7 +1360,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode, 
		err = loop_change_fd(lo, bdev, arg);

		break;

	case LOOP_CLR_FD:

		/* loop_clr_fd would have unlocked lo_ctl_mutex on success */

		/* loop_clr_fd would have unlocked loop_ctl_mutex on success */

		err = loop_clr_fd(lo);

		if (!err)

			goto out_unlocked;
@@ -1395,7 +1396,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode, 
	default:

		err = lo->ioctl ? lo->ioctl(lo, cmd, arg) : -EINVAL;

	}

	mutex_unlock(&lo->lo_ctl_mutex);

	mutex_unlock(&loop_ctl_mutex);



out_unlocked:

	return err;
@@ -1528,16 +1529,16 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode, 


	switch(cmd) {

	case LOOP_SET_STATUS:

		mutex_lock(&lo->lo_ctl_mutex);

		mutex_lock(&loop_ctl_mutex);

		err = loop_set_status_compat(

			lo, (const struct compat_loop_info __user *) arg);

		mutex_unlock(&lo->lo_ctl_mutex);

		mutex_unlock(&loop_ctl_mutex);

		break;

	case LOOP_GET_STATUS:

		mutex_lock(&lo->lo_ctl_mutex);

		mutex_lock(&loop_ctl_mutex);

		err = loop_get_status_compat(

			lo, (struct compat_loop_info __user *) arg);

		mutex_unlock(&lo->lo_ctl_mutex);

		mutex_unlock(&loop_ctl_mutex);

		break;

	case LOOP_SET_CAPACITY:

	case LOOP_CLR_FD:
@@ -1581,7 +1582,7 @@ static void __lo_release(struct loop_device *lo) 
	if (atomic_dec_return(&lo->lo_refcnt))

		return;



	mutex_lock(&lo->lo_ctl_mutex);

	mutex_lock(&loop_ctl_mutex);

	if (lo->lo_flags & LO_FLAGS_AUTOCLEAR) {

		/*

		 * In autoclear mode, stop the loop thread
@@ -1598,7 +1599,7 @@ static void __lo_release(struct loop_device *lo) 
		loop_flush(lo);

	}



	mutex_unlock(&lo->lo_ctl_mutex);

	mutex_unlock(&loop_ctl_mutex);

}



static void lo_release(struct gendisk *disk, fmode_t mode)
@@ -1644,10 +1645,10 @@ static int unregister_transfer_cb(int id, void *ptr, void *data) 
	struct loop_device *lo = ptr;

	struct loop_func_table *xfer = data;



	mutex_lock(&lo->lo_ctl_mutex);

	mutex_lock(&loop_ctl_mutex);

	if (lo->lo_encryption == xfer)

		loop_release_xfer(lo);

	mutex_unlock(&lo->lo_ctl_mutex);

	mutex_unlock(&loop_ctl_mutex);

	return 0;

}
@@ -1813,7 +1814,6 @@ static int loop_add(struct loop_device **l, int i) 
	if (!part_shift)

		disk->flags |= GENHD_FL_NO_PART_SCAN;

	disk->flags |= GENHD_FL_EXT_DEVT;

	mutex_init(&lo->lo_ctl_mutex);

	atomic_set(&lo->lo_refcnt, 0);

	lo->lo_number		= i;

	spin_lock_init(&lo->lo_lock);
@@ -1926,19 +1926,19 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd, 
		ret = loop_lookup(&lo, parm);

		if (ret < 0)

			break;

		mutex_lock(&lo->lo_ctl_mutex);

		mutex_lock(&loop_ctl_mutex);

		if (lo->lo_state != Lo_unbound) {

			ret = -EBUSY;

			mutex_unlock(&lo->lo_ctl_mutex);

			mutex_unlock(&loop_ctl_mutex);

			break;

		}

		if (atomic_read(&lo->lo_refcnt) > 0) {

			ret = -EBUSY;

			mutex_unlock(&lo->lo_ctl_mutex);

			mutex_unlock(&loop_ctl_mutex);

			break;

		}

		lo->lo_disk->private_data = NULL;

		mutex_unlock(&lo->lo_ctl_mutex);

		mutex_unlock(&loop_ctl_mutex);

		idr_remove(&loop_index_idr, lo->lo_number);

		loop_remove(lo);

		break;

drivers/block/loop.h

+0 −1
Original line number Diff line number Diff line
@@ -55,7 +55,6 @@ struct loop_device { 


	spinlock_t		lo_lock;

	int			lo_state;

	struct mutex		lo_ctl_mutex;

	struct kthread_worker	worker;

	struct task_struct	*worker_task;

	bool			use_dio;