[PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Theodore Ts'o]

Refactor the validation code used in LOOP_SET_FD so it is also used in
LOOP_CHANGE_FD.  Otherwise it is possible to construct a set of loop
devices that all refer to each other.  This can lead to a infinite
loop in starting with "while (is_loop_device(f)) .." in loop_set_fd().

Fix this by refactoring out the validation code and using it for
LOOP_CHANGE_FD as well as LOOP_SET_FD.

Reported-by: syzbot+4349872271ece473a7c91190b68b4bac7c5dbc87@syzkaller.appspotmail.com
Reported-by: syzbot+40bd32c4d9a3cc12a339@syzkaller.appspotmail.com
Reported-by: syzbot+769c54e66f994b041be7@syzkaller.appspotmail.com
Reported-by: syzbot+0a89a9ce473936c57065@syzkaller.appspotmail.com
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 drivers/block/loop.c | 70 ++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5d4e31655d96..fd2ddd60d267 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -644,6 +644,36 @@ static void loop_reread_partitions(struct loop_device *lo,
 			__func__, lo->lo_number, lo->lo_file_name, rc);
 }
 
+static inline int is_loop_device(struct file *file)
+{
+	struct inode *i = file->f_mapping->host;
+
+	return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
+}
+
+static int loop_validate_file(struct file *file, struct block_device *bdev)
+{
+	struct inode	*inode = file->f_mapping->host;
+	struct file	*f = file;
+
+	/* Avoid recursion */
+	while (is_loop_device(f)) {
+		struct loop_device *l;
+
+		if (f->f_mapping->host->i_bdev == bdev)
+			return -EBADF;
+
+		l = f->f_mapping->host->i_bdev->bd_disk->private_data;
+		if (l->lo_state == Lo_unbound) {
+			return -EINVAL;
+		}
+		f = l->lo_backing_file;
+	}
+	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
+		return -EINVAL;
+	return 0;
+}
+
 /*
  * loop_change_fd switched the backing store of a loopback device to
  * a new file. This is useful for operating system installers to free up
@@ -673,14 +703,13 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	if (!file)
 		goto out;
 
+	error = loop_validate_file(file, bdev);
+	if (error)
+		goto out_putf;
+
 	inode = file->f_mapping->host;
 	old_file = lo->lo_backing_file;
 
-	error = -EINVAL;
-
-	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
-		goto out_putf;
-
 	/* size of the new backing store needs to be the same */
 	if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
 		goto out_putf;
@@ -706,13 +735,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	return error;
 }
 
-static inline int is_loop_device(struct file *file)
-{
-	struct inode *i = file->f_mapping->host;
-
-	return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
-}
-
 /* loop sysfs attributes */
 
 static ssize_t loop_attr_show(struct device *dev, char *page,
@@ -877,7 +899,7 @@ static int loop_prepare_queue(struct loop_device *lo)
 static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 		       struct block_device *bdev, unsigned int arg)
 {
-	struct file	*file, *f;
+	struct file	*file;
 	struct inode	*inode;
 	struct address_space *mapping;
 	int		lo_flags = 0;
@@ -896,29 +918,13 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	if (lo->lo_state != Lo_unbound)
 		goto out_putf;
 
-	/* Avoid recursion */
-	f = file;
-	while (is_loop_device(f)) {
-		struct loop_device *l;
-
-		if (f->f_mapping->host->i_bdev == bdev)
-			goto out_putf;
-
-		l = f->f_mapping->host->i_bdev->bd_disk->private_data;
-		if (l->lo_state == Lo_unbound) {
-			error = -EINVAL;
-			goto out_putf;
-		}
-		f = l->lo_backing_file;
-	}
+	error = loop_validate_file(file, bdev);
+	if (error)
+		goto out_putf;
 
 	mapping = file->f_mapping;
 	inode = mapping->host;
 
-	error = -EINVAL;
-	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
-		goto out_putf;
-
 	if (!(file->f_mode & FMODE_WRITE) || !(mode & FMODE_WRITE) ||
 	    !file->f_op->write_iter)
 		lo_flags |= LO_FLAGS_READ_ONLY;
-- 
2.16.1.72.g5be1f00a9a

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Tetsuo Handa]

Oh, your message did not arrive at news.gmane.org and I didn't notice that you
already wrote this patch. But please update yours or review mine shown below.

> @@ -673,14 +703,13 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
>  	if (!file)
>  		goto out;
>  
> +	error = loop_validate_file(file, bdev);
> +	if (error)
> +		goto out_putf;
> +
>  	inode = file->f_mapping->host;
>  	old_file = lo->lo_backing_file;
>  
> -	error = -EINVAL;
> -
> -	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
> -		goto out_putf;
> -
>  	/* size of the new backing store needs to be the same */
>  	if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
>  		goto out_putf;

error == 0 upon "goto out_putf" is wrong.



>>From eed54c6ae475860a9c63b37b58f34735e792eef7 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 5 May 2018 12:59:12 +0900
Subject: [PATCH] block/loop: Add recursion check for LOOP_CHANGE_FD request.

syzbot is reporting hung tasks at blk_freeze_queue() [1]. This is
due to ioctl(loop_fd, LOOP_CHANGE_FD, loop_fd) request which should be
rejected. Fix this by adding same recursion check which is used by
LOOP_SET_FD request.

[1] https://syzkaller.appspot.com/bug?id=078805a692853552e08154b1bcd2bc2c729eda88

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+2ab52b8d94df5e2eaa01@syzkaller.appspotmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/block/loop.c | 59 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5d4e316..cee3c01 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -644,6 +644,34 @@ static void loop_reread_partitions(struct loop_device *lo,
 			__func__, lo->lo_number, lo->lo_file_name, rc);
 }
 
+static inline int is_loop_device(struct file *file)
+{
+	struct inode *i = file->f_mapping->host;
+
+	return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
+}
+
+static int check_loop_recursion(struct file *f, struct block_device *bdev)
+{
+	/*
+	 * FIXME: Traversing on other loop devices without corresponding
+	 * lo_ctl_mutex is not safe. l->lo_state can become Lo_rundown and
+	 * l->lo_backing_file can become NULL when raced with LOOP_CLR_FD.
+	 */
+	while (is_loop_device(f)) {
+		struct loop_device *l;
+
+		if (f->f_mapping->host->i_bdev == bdev)
+			return -EBUSY;
+
+		l = f->f_mapping->host->i_bdev->bd_disk->private_data;
+		if (l->lo_state == Lo_unbound)
+			return -EINVAL;
+		f = l->lo_backing_file;
+	}
+	return 0;
+}
+
 /*
  * loop_change_fd switched the backing store of a loopback device to
  * a new file. This is useful for operating system installers to free up
@@ -673,6 +701,11 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	if (!file)
 		goto out;
 
+	/* Avoid recursion */
+	error = check_loop_recursion(file, bdev);
+	if (error)
+		goto out_putf;
+
 	inode = file->f_mapping->host;
 	old_file = lo->lo_backing_file;
 
@@ -706,13 +739,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	return error;
 }
 
-static inline int is_loop_device(struct file *file)
-{
-	struct inode *i = file->f_mapping->host;
-
-	return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
-}
-
 /* loop sysfs attributes */
 
 static ssize_t loop_attr_show(struct device *dev, char *page,
@@ -877,7 +903,7 @@ static int loop_prepare_queue(struct loop_device *lo)
 static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 		       struct block_device *bdev, unsigned int arg)
 {
-	struct file	*file, *f;
+	struct file	*file;
 	struct inode	*inode;
 	struct address_space *mapping;
 	int		lo_flags = 0;
@@ -897,20 +923,9 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 		goto out_putf;
 
 	/* Avoid recursion */
-	f = file;
-	while (is_loop_device(f)) {
-		struct loop_device *l;
-
-		if (f->f_mapping->host->i_bdev == bdev)
-			goto out_putf;
-
-		l = f->f_mapping->host->i_bdev->bd_disk->private_data;
-		if (l->lo_state == Lo_unbound) {
-			error = -EINVAL;
-			goto out_putf;
-		}
-		f = l->lo_backing_file;
-	}
+	error = check_loop_recursion(file, bdev);
+	if (error)
+		goto out_putf;
 
 	mapping = file->f_mapping;
 	inode = mapping->host;
-- 
1.8.3.1

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Theodore Y. Ts'o]

On Mon, May 07, 2018 at 08:16:58PM +0900, Tetsuo Handa wrote:
> Oh, your message did not arrive at news.gmane.org and I didn't notice that you
> already wrote this patch. But please update yours or review mine shown below.
> 
> > @@ -673,14 +703,13 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
> >  	if (!file)
> >  		goto out;
> >  
> > +	error = loop_validate_file(file, bdev);
> > +	if (error)
> > +		goto out_putf;
> > +
> >  	inode = file->f_mapping->host;
> >  	old_file = lo->lo_backing_file;
> >  
> > -	error = -EINVAL;
> > -
> > -	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
> > -		goto out_putf;
> > -
> >  	/* size of the new backing store needs to be the same */
> >  	if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
> >  		goto out_putf;
> 
> error == 0 upon "goto out_putf" is wrong.

I don't understand your concern; where are we going to out_putf when
error == 0?

The relevant code that was added is:

	error = loop_validate_file(file, bdev);
	if (error)
		goto out_putf;


						- Ted

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Tetsuo Handa]

Theodore Y. Ts'o wrote:
> On Mon, May 07, 2018 at 08:16:58PM +0900, Tetsuo Handa wrote:
> > Oh, your message did not arrive at news.gmane.org and I didn't notice that you
> > already wrote this patch. But please update yours or review mine shown below.
> > 
> > > @@ -673,14 +703,13 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
> > >  	if (!file)
> > >  		goto out;
> > >  
> > > +	error = loop_validate_file(file, bdev);
> > > +	if (error)
> > > +		goto out_putf;
> > > +
> > >  	inode = file->f_mapping->host;
> > >  	old_file = lo->lo_backing_file;
> > >  
> > > -	error = -EINVAL;
> > > -
> > > -	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
> > > -		goto out_putf;
> > > -
> > >  	/* size of the new backing store needs to be the same */
> > >  	if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
> > >  		goto out_putf;
> > 
> > error == 0 upon "goto out_putf" is wrong.
> 
> I don't understand your concern; where are we going to out_putf when
> error == 0?

-	error = -EINVAL;  /* <= You are trying to remove this assignment. */
-
-	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
-		goto out_putf;
 	/* size of the new backing store needs to be the same */
 	if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
 		goto out_putf; /* <= Hence error == 0 at this point. */

By the way, are you aware that current "/* Avoid recursion */" loop is not thread safe?

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Theodore Y. Ts'o]

On Mon, May 07, 2018 at 10:21:04PM +0900, Tetsuo Handa wrote:
> > I don't understand your concern; where are we going to out_putf when
> > error == 0?

Ah, now I see it, thanks.  I'll send a revised patch.

> By the way, are you aware that current "/* Avoid recursion */" loop is not thread safe?

Actually, it is safe.  While the child loop device has an open file on
the parent, lo_refcnt is elevated, which prevents loop_clr_fd from
actually set setting lo_state to Lo_rundown and clearing
lo_backing_file

						- Ted

[PATCH -v2] loop: add recursion validation to LOOP_CHANGE_FD

[Theodore Ts'o]

Refactor the validation code used in LOOP_SET_FD so it is also used in
LOOP_CHANGE_FD.  Otherwise it is possible to construct a set of loop
devices that all refer to each other.  This can lead to a infinite
loop in starting with "while (is_loop_device(f)) .." in loop_set_fd().

Fix this by refactoring out the validation code and using it for
LOOP_CHANGE_FD as well as LOOP_SET_FD.

Reported-by: syzbot+4349872271ece473a7c91190b68b4bac7c5dbc87@syzkaller.appspotmail.com
Reported-by: syzbot+40bd32c4d9a3cc12a339@syzkaller.appspotmail.com
Reported-by: syzbot+769c54e66f994b041be7@syzkaller.appspotmail.com
Reported-by: syzbot+0a89a9ce473936c57065@syzkaller.appspotmail.com
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 drivers/block/loop.c | 68 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 30 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5d4e31655d96..0373d64e174c 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -644,6 +644,36 @@ static void loop_reread_partitions(struct loop_device *lo,
 			__func__, lo->lo_number, lo->lo_file_name, rc);
 }
 
+static inline int is_loop_device(struct file *file)
+{
+	struct inode *i = file->f_mapping->host;
+
+	return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
+}
+
+static int loop_validate_file(struct file *file, struct block_device *bdev)
+{
+	struct inode	*inode = file->f_mapping->host;
+	struct file	*f = file;
+
+	/* Avoid recursion */
+	while (is_loop_device(f)) {
+		struct loop_device *l;
+
+		if (f->f_mapping->host->i_bdev == bdev)
+			return -EBADF;
+
+		l = f->f_mapping->host->i_bdev->bd_disk->private_data;
+		if (l->lo_state == Lo_unbound) {
+			return -EINVAL;
+		}
+		f = l->lo_backing_file;
+	}
+	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
+		return -EINVAL;
+	return 0;
+}
+
 /*
  * loop_change_fd switched the backing store of a loopback device to
  * a new file. This is useful for operating system installers to free up
@@ -673,14 +703,15 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	if (!file)
 		goto out;
 
+	error = loop_validate_file(file, bdev);
+	if (error)
+		goto out_putf;
+
 	inode = file->f_mapping->host;
 	old_file = lo->lo_backing_file;
 
 	error = -EINVAL;
 
-	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
-		goto out_putf;
-
 	/* size of the new backing store needs to be the same */
 	if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
 		goto out_putf;
@@ -706,13 +737,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	return error;
 }
 
-static inline int is_loop_device(struct file *file)
-{
-	struct inode *i = file->f_mapping->host;
-
-	return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
-}
-
 /* loop sysfs attributes */
 
 static ssize_t loop_attr_show(struct device *dev, char *page,
@@ -877,7 +901,7 @@ static int loop_prepare_queue(struct loop_device *lo)
 static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 		       struct block_device *bdev, unsigned int arg)
 {
-	struct file	*file, *f;
+	struct file	*file;
 	struct inode	*inode;
 	struct address_space *mapping;
 	int		lo_flags = 0;
@@ -896,29 +920,13 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	if (lo->lo_state != Lo_unbound)
 		goto out_putf;
 
-	/* Avoid recursion */
-	f = file;
-	while (is_loop_device(f)) {
-		struct loop_device *l;
-
-		if (f->f_mapping->host->i_bdev == bdev)
-			goto out_putf;
-
-		l = f->f_mapping->host->i_bdev->bd_disk->private_data;
-		if (l->lo_state == Lo_unbound) {
-			error = -EINVAL;
-			goto out_putf;
-		}
-		f = l->lo_backing_file;
-	}
+	error = loop_validate_file(file, bdev);
+	if (error)
+		goto out_putf;
 
 	mapping = file->f_mapping;
 	inode = mapping->host;
 
-	error = -EINVAL;
-	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
-		goto out_putf;
-
 	if (!(file->f_mode & FMODE_WRITE) || !(mode & FMODE_WRITE) ||
 	    !file->f_op->write_iter)
 		lo_flags |= LO_FLAGS_READ_ONLY;
-- 
2.16.1.72.g5be1f00a9a

Re: [PATCH -v2] loop: add recursion validation to LOOP_CHANGE_FD

[Tetsuo Handa]

I noticed that this patch is forgotten at ext4.git#loop-fix and therefore
is not available at linux-next.git . Please be sure to include for 4.18 .

On 2018/05/08 0:37, Theodore Ts'o wrote:
> Refactor the validation code used in LOOP_SET_FD so it is also used in
> LOOP_CHANGE_FD.  Otherwise it is possible to construct a set of loop
> devices that all refer to each other.  This can lead to a infinite
> loop in starting with "while (is_loop_device(f)) .." in loop_set_fd().
> 
> Fix this by refactoring out the validation code and using it for
> LOOP_CHANGE_FD as well as LOOP_SET_FD.
> 
> Reported-by: syzbot+4349872271ece473a7c91190b68b4bac7c5dbc87@syzkaller.appspotmail.com
> Reported-by: syzbot+40bd32c4d9a3cc12a339@syzkaller.appspotmail.com
> Reported-by: syzbot+769c54e66f994b041be7@syzkaller.appspotmail.com
> Reported-by: syzbot+0a89a9ce473936c57065@syzkaller.appspotmail.com
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  drivers/block/loop.c | 68 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 30 deletions(-)
> 

Re: [PATCH -v2] loop: add recursion validation to LOOP_CHANGE_FD

[Theodore Y. Ts'o]

On Tue, Jun 05, 2018 at 10:04:56PM +0900, Tetsuo Handa wrote:
> I noticed that this patch is forgotten at ext4.git#loop-fix and therefore
> is not available at linux-next.git . Please be sure to include for 4.18 .

I was assuming Jens was going to send it to Linus, after he does a
review.  I could send a pull request to Linus directly, but while I
was the original author of loop driver, that was like two decades ago
(and LOOP_CHANGE_FD was *not* my idea), and normally loop driver
changes goes up to Linus through the block tree, I believe.

Cheers,

						- Ted

> 
> On 2018/05/08 0:37, Theodore Ts'o wrote:
> > Refactor the validation code used in LOOP_SET_FD so it is also used in
> > LOOP_CHANGE_FD.  Otherwise it is possible to construct a set of loop
> > devices that all refer to each other.  This can lead to a infinite
> > loop in starting with "while (is_loop_device(f)) .." in loop_set_fd().
> > 
> > Fix this by refactoring out the validation code and using it for
> > LOOP_CHANGE_FD as well as LOOP_SET_FD.
> > 
> > Reported-by: syzbot+4349872271ece473a7c91190b68b4bac7c5dbc87@syzkaller.appspotmail.com
> > Reported-by: syzbot+40bd32c4d9a3cc12a339@syzkaller.appspotmail.com
> > Reported-by: syzbot+769c54e66f994b041be7@syzkaller.appspotmail.com
> > Reported-by: syzbot+0a89a9ce473936c57065@syzkaller.appspotmail.com
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > ---
> >  drivers/block/loop.c | 68 +++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 38 insertions(+), 30 deletions(-)
> > 

Re: [PATCH -v2] loop: add recursion validation to LOOP_CHANGE_FD

[Jens Axboe]

On 6/5/18 8:51 AM, Theodore Y. Ts'o wrote:
> On Tue, Jun 05, 2018 at 10:04:56PM +0900, Tetsuo Handa wrote:
>> I noticed that this patch is forgotten at ext4.git#loop-fix and therefore
>> is not available at linux-next.git . Please be sure to include for 4.18 .
> 
> I was assuming Jens was going to send it to Linus, after he does a
> review.  I could send a pull request to Linus directly, but while I
> was the original author of loop driver, that was like two decades ago
> (and LOOP_CHANGE_FD was *not* my idea), and normally loop driver
> changes goes up to Linus through the block tree, I believe.

I'll queue it up, thanks Ted.

-- 
Jens Axboe

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Tetsuo Handa]

Theodore Y. Ts'o wrote:
> On Mon, May 07, 2018 at 10:21:04PM +0900, Tetsuo Handa wrote:
> > > I don't understand your concern; where are we going to out_putf when
> > > error == 0?
> 
> Ah, now I see it, thanks.  I'll send a revised patch.
> 
> > By the way, are you aware that current "/* Avoid recursion */" loop is not thread safe?
> 
> Actually, it is safe.  While the child loop device has an open file on
> the parent, lo_refcnt is elevated, which prevents loop_clr_fd from
> actually set setting lo_state to Lo_rundown and clearing
> lo_backing_file

If you think it is safe, please explain that the crash referenced in a patch
at https://groups.google.com/d/msg/syzkaller-bugs/2Rw8-OM6IbM/PzdobV8kAgAJ is
no longer possible. syzbot is hitting crashes there.

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Theodore Y. Ts'o]

On Tue, May 08, 2018 at 05:45:21AM +0900, Tetsuo Handa wrote:
> > 
> > > By the way, are you aware that current "/* Avoid recursion */" loop is not thread safe?
> > 
> > Actually, it is safe.  While the child loop device has an open file on
> > the parent, lo_refcnt is elevated, which prevents loop_clr_fd from
> > actually set setting lo_state to Lo_rundown and clearing
> > lo_backing_file
> 
> If you think it is safe, please explain that the crash referenced in a patch
> at https://groups.google.com/d/msg/syzkaller-bugs/2Rw8-OM6IbM/PzdobV8kAgAJ is
> no longer possible. syzbot is hitting crashes there.

Huh?  You were worried about a race where loop_change_fd could race
with loop_clr_fd causing a NULL dereference of lo_backing_file.

The mail thread you are referencing is a deadlock problem with
loop_reread_partitions() and lo_release().  This is unreleated to the
possible race you were concerned about in loop_change_fd().

						- Ted

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Tetsuo Handa]

> On Tue, May 08, 2018 at 05:45:21AM +0900, Tetsuo Handa wrote:
> > > 
> > > > By the way, are you aware that current "/* Avoid recursion */" loop is not thread safe?
> > > 
> > > Actually, it is safe.  While the child loop device has an open file on
> > > the parent, lo_refcnt is elevated, which prevents loop_clr_fd from
> > > actually set setting lo_state to Lo_rundown and clearing
> > > lo_backing_file
> > 
> > If you think it is safe, please explain that the crash referenced in a patch
> > at https://groups.google.com/d/msg/syzkaller-bugs/2Rw8-OM6IbM/PzdobV8kAgAJ is
> > no longer possible. syzbot is hitting crashes there.
> 
> Huh?  You were worried about a race where loop_change_fd could race
> with loop_clr_fd causing a NULL dereference of lo_backing_file.
> 
> The mail thread you are referencing is a deadlock problem with
> loop_reread_partitions() and lo_release().  This is unreleated to the
> possible race you were concerned about in loop_change_fd().

The thread I mean is:

  general protection fault in lo_ioctl (2)
  https://syzkaller.appspot.com/bug?id=f3cfe26e785d85f9ee259f385515291d21bd80a3

Are you sure that your patch solves this problem as well?

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Theodore Y. Ts'o]

On Tue, May 08, 2018 at 09:28:17AM +0900, Tetsuo Handa wrote:
> The thread I mean is:
> 
>   general protection fault in lo_ioctl (2)
>   https://syzkaller.appspot.com/bug?id=f3cfe26e785d85f9ee259f385515291d21bd80a3
> 
> Are you sure that your patch solves this problem as well?

Well, I can't be sure, since there's not enough information in that
particular syzkaller report to definitively pin down the root cause.

And while I can't reproduce the crash using the syzkaller repro with
the patch; I can't reproduce the crash *without* the patch, either.

This is what Syzkaller has to say, but of course, in its own
documentation's words, "It's only a dumb bot".  :-)e

That being said, triggering the problem which it is so concerned about
requires root privilieges, so I would not consider it high priority to
track down --- especially given that we don't have a reliable
reproducer for it.


		       	     	    - Ted

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger  
crash:

Reported-and-tested-by:  
syzbot+bf89c128e05dd6c62523@syzkaller.appspotmail.com

Tested on:

commit:         170785a9cc72 loop: add recursion validation to LOOP_CHANGE..
git tree:        
git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/loop-fix
kernel config:  https://syzkaller.appspot.com/x/.config?x=5a1dc06635c10d27
compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
userspace arch: i386

Note: testing is done by a robot and is best-effort only.

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Tetsuo Handa]

Theodore Y. Ts'o wrote:
> On Tue, May 08, 2018 at 09:28:17AM +0900, Tetsuo Handa wrote:
> > The thread I mean is:
> > 
> >   general protection fault in lo_ioctl (2)
> >   https://syzkaller.appspot.com/bug?id=f3cfe26e785d85f9ee259f385515291d21bd80a3
> > 
> > Are you sure that your patch solves this problem as well?
> 
> Well, I can't be sure, since there's not enough information in that
> particular syzkaller report to definitively pin down the root cause.
> 
> And while I can't reproduce the crash using the syzkaller repro with
> the patch; I can't reproduce the crash *without* the patch, either.
> 
> This is what Syzkaller has to say, but of course, in its own
> documentation's words, "It's only a dumb bot".  :-)e
> 
> That being said, triggering the problem which it is so concerned about
> requires root privilieges, so I would not consider it high priority to
> track down --- especially given that we don't have a reliable
> reproducer for it.
> 

OK. Using sleep injection patch and reproducer shown below, I can reproduce
the crashes. Unless we hold corresponding lo->lo_ctl_mutex (or keep
lo->lo_refcnt elevated) when traversing other loop devices,
"/* Avoid recursion */" loop will suffer from races by loop_clr_fd().

------------------------------------------------------------
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -909,6 +909,9 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 			error = -EINVAL;
 			goto out_putf;
 		}
+		pr_err("Start sleeping\n");
+		schedule_timeout_killable(3 * HZ);
+		pr_err("End sleeping\n");
 		f = l->lo_backing_file;
 	}
 
------------------------------------------------------------

------------------------------------------------------------
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <linux/loop.h>
#include <sys/ioctl.h>

int main(int argc, char *argv[])
{
	int fd0 = open("/dev/loop0", O_RDONLY);
	int fd1 = open("/dev/loop1", O_RDONLY);
	int fd2 = open("/tmp/file", O_RDWR | O_CREAT | O_TRUNC, 0600);
	ioctl(fd1, LOOP_SET_FD, fd2);
	if (fork() == 0) {
		sleep(1);
		ioctl(fd1, LOOP_CLR_FD, 0);
		_exit(0);
	}
	ioctl(fd0, LOOP_SET_FD, fd1);
	return 0;
}
------------------------------------------------------------

------------------------------------------------------------
[   14.119073] loop: module loaded
[   17.363610] Start sleeping
[   20.383442] End sleeping
[   20.386511] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
[   20.394779] PGD 13377d067 P4D 13377d067 PUD 131509067 PMD 0 
[   20.400847] Oops: 0000 [#1] SMP
[   20.403875] Modules linked in: loop
[   20.406188] CPU: 6 PID: 6470 Comm: a.out Tainted: G                T 4.17.0-rc4+ #540
[   20.411266] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 05/19/2017
[   20.418169] RIP: 0010:lo_ioctl+0x7ef/0x840 [loop]
[   20.421272] RSP: 0018:ffffc90000bbbd88 EFLAGS: 00010282
[   20.424661] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff83679478
[   20.429271] RDX: ffff8801332e9c00 RSI: 0000000000000086 RDI: 0000000000000286
[   20.434517] RBP: ffffc90000bbbdd8 R08: 0000000000000638 R09: 0000000000000000
[   20.436879] R10: 0000000000000190 R11: 0720072007200720 R12: ffff8801314ab118
[   20.439076] R13: ffff880138deae40 R14: ffff8801311f7780 R15: ffff8801314ab000
[   20.441144] FS:  00007f0b57743740(0000) GS:ffff88013a780000(0000) knlGS:0000000000000000
[   20.443588] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   20.445284] CR2: 0000000000000008 CR3: 0000000138efb002 CR4: 00000000000606e0
[   20.447381] Call Trace:
[   20.448149]  blkdev_ioctl+0x88d/0x950
[   20.449237]  block_ioctl+0x38/0x40
[   20.450269]  do_vfs_ioctl+0xaa/0x650
[   20.451479]  ? handle_mm_fault+0x108/0x250
[   20.452704]  ksys_ioctl+0x70/0x80
[   20.453737]  __x64_sys_ioctl+0x15/0x20
[   20.454887]  do_syscall_64+0x5d/0x100
[   20.456014]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   20.457519] RIP: 0033:0x7f0b57267107
[   20.458644] RSP: 002b:00007fff8a0fd698 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[   20.460853] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007f0b57267107
[   20.462952] RDX: 0000000000000004 RSI: 0000000000004c00 RDI: 0000000000000003
[   20.465023] RBP: 0000000000000003 R08: 00007f0b57743740 R09: 0000000000000000
[   20.467091] R10: 00007f0b57743a10 R11: 0000000000000246 R12: 00000000004005ef
[   20.469361] R13: 00007fff8a0fd790 R14: 0000000000000000 R15: 0000000000000000
[   20.471657] Code: a0 48 89 55 d0 e8 e0 5f 1d e1 bf b8 0b 00 00 e8 78 9e 7c e2 48 c7 c7 a9 40 00 a0 e8 ca 5f 1d e1 48 8b 55 d0 48 8b 82 f0 00 00 00 <48> 8b 40 08 48 8b 40 68 48 85 c0 0f 84 15 fd ff ff 0f b7 90 b8 
[   20.477207] RIP: lo_ioctl+0x7ef/0x840 [loop] RSP: ffffc90000bbbd88
[   20.479027] CR2: 0000000000000008
[   20.480063] ---[ end trace 925bc1b992d96cb3 ]---
[   20.481441] Kernel panic - not syncing: Fatal exception
[   20.483119] Kernel Offset: disabled
[   20.489564] Rebooting in 86400 seconds..
------------------------------------------------------------

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Dmitry Vyukov]

On Tue, May 8, 2018 at 5:56 AM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> On Tue, May 08, 2018 at 09:28:17AM +0900, Tetsuo Handa wrote:
>> The thread I mean is:
>>
>>   general protection fault in lo_ioctl (2)
>>   https://syzkaller.appspot.com/bug?id=f3cfe26e785d85f9ee259f385515291d21bd80a3
>>
>> Are you sure that your patch solves this problem as well?
>
> Well, I can't be sure, since there's not enough information in that
> particular syzkaller report to definitively pin down the root cause.
>
> And while I can't reproduce the crash using the syzkaller repro with
> the patch; I can't reproduce the crash *without* the patch, either.

Hi Ted,

Did you follow all instructions (commit, config, compiler, etc)?
syzbot does not have any special magic, it just executes the described
steps and then collects the kernel crash output it sees.


> This is what Syzkaller has to say, but of course, in its own
> documentation's words, "It's only a dumb bot".  :-)e

Well, not anymore. Now it's just a "bot":
https://github.com/google/syzkaller/commit/8180779d1d06e1cdf27882f50e7f72650f95797d
Too many people took "dumb bot" too seriously ;)


> That being said, triggering the problem which it is so concerned about
> requires root privilieges, so I would not consider it high priority to
> track down --- especially given that we don't have a reliable
> reproducer for it.
>
>
>                                     - Ted
>
> Hello,
>
> syzbot has tested the proposed patch and the reproducer did not trigger
> crash:
>
> Reported-and-tested-by:
> syzbot+bf89c128e05dd6c62523@syzkaller.appspotmail.com
>
> Tested on:
>
> commit:         170785a9cc72 loop: add recursion validation to LOOP_CHANGE..
> git tree:
> git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/loop-fix
> kernel config:  https://syzkaller.appspot.com/x/.config?x=5a1dc06635c10d27
> compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> userspace arch: i386
>
> Note: testing is done by a robot and is best-effort only.
>
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180508035626.GF999%40thunk.org.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Theodore Y. Ts'o]

On Wed, May 09, 2018 at 10:49:54AM +0200, Dmitry Vyukov wrote:
> Hi Ted,
> 
> Did you follow all instructions (commit, config, compiler, etc)?
> syzbot does not have any special magic, it just executes the described
> steps and then collects the kernel crash output it sees.

No, I didn't.  Unfortunately, I don't have the time to repro it on
exactly the config, commit, compiler.  And debugging tons of Syzkaller
commits is not on my OKR list, and I have lots of P0/P1 bugs and
projects that are competing for my attention.

I tried repro'ing it using the standard compiler, and using -rc4 as a
base.  If it doesn't repro there, using my standard kernel config ---
and it requires root, and in my judgement it's *highly* unlikely to
happen in real life --- then it becomes a P2 or P3 bug, it's not worth
my time to build a kernel using exactly the commit, config, and
compiler that Syzkaller specified.  Someday, you or I or someone else
will build at tool that builds the kernel in a GCE VM, using the
specified config and a compiler, and which minimizes the amount of
human toil needed to do the sort of investigation you seem to want to
dump on upstream developers.

There's a *reason* why many upstream developers have been asking for
improvements in the syzkaller tool to reduce toil.  If it's fair for
you to ask for us to do work, then it's also fair for us to ask you to
do work.  And if the answer is "sorry, I don't have the time; other
things are higher priority", that's a fair answer coming from both
sides.

Regards,

						- Ted

Re: [PATCH] loop: add recursion validation to LOOP_CHANGE_FD

[Dmitry Vyukov]

On Wed, May 9, 2018 at 4:02 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> On Wed, May 09, 2018 at 10:49:54AM +0200, Dmitry Vyukov wrote:
>> Hi Ted,
>>
>> Did you follow all instructions (commit, config, compiler, etc)?
>> syzbot does not have any special magic, it just executes the described
>> steps and then collects the kernel crash output it sees.
>
> No, I didn't.  Unfortunately, I don't have the time to repro it on
> exactly the config, commit, compiler.  And debugging tons of Syzkaller
> commits is not on my OKR list, and I have lots of P0/P1 bugs and
> projects that are competing for my attention.
>
> I tried repro'ing it using the standard compiler, and using -rc4 as a
> base.  If it doesn't repro there, using my standard kernel config ---
> and it requires root, and in my judgement it's *highly* unlikely to
> happen in real life --- then it becomes a P2 or P3 bug, it's not worth
> my time to build a kernel using exactly the commit, config, and
> compiler that Syzkaller specified.  Someday, you or I or someone else
> will build at tool that builds the kernel in a GCE VM, using the
> specified config and a compiler, and which minimizes the amount of
> human toil needed to do the sort of investigation you seem to want to
> dump on upstream developers.
>
> There's a *reason* why many upstream developers have been asking for
> improvements in the syzkaller tool to reduce toil.  If it's fair for
> you to ask for us to do work, then it's also fair for us to ask you to
> do work.  And if the answer is "sorry, I don't have the time; other
> things are higher priority", that's a fair answer coming from both
> sides.


Hi Ted,

I am working on syzbot/syzkaller all of my time. Repro script is on my
plate: https://github.com/google/syzkaller/issues/563. I will do it.
Not because somebody asks me, but because I am interested in making
kernel more correct, stable and secure.