| Message ID | 201805072016.GAC48495.VSJQFtFHLFMOOO@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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?
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
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.
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
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.
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.
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
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.
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;