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  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.

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 Tue, May 8, 2018 at 5:56 AM, 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.

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+bf89c128e05dd6c62...@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+unsubscr...@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 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+bf89c128e05dd6c62...@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

[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]

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 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

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 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]

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 
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 
Reported-by: syzbot 
Cc: Jens Axboe 
---
 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