* Re: general protection fault in relay_open_buf
2019-01-31 10:51 ` Greg KH
@ 2019-01-31 11:16 ` Dmitry Vyukov
2019-01-31 11:22 ` Greg KH
2019-01-31 11:35 ` syzbot
2019-01-31 11:29 ` Tetsuo Handa
` (2 subsequent siblings)
3 siblings, 2 replies; 15+ messages in thread
From: Dmitry Vyukov @ 2019-01-31 11:16 UTC (permalink / raw)
To: Greg KH
Cc: Kees Cook, Andrew Morton, syzbot, Eric Biggers, Souptick Joarder,
LKML, David Rientjes, syzkaller-bugs
[-- Attachment #1: Type: text/plain, Size: 5469 bytes --]
On Thu, Jan 31, 2019 at 11:51 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jan 31, 2019 at 10:54:18PM +1300, Kees Cook wrote:
> > On Thu, Jan 31, 2019 at 7:53 AM syzbot
> > <syzbot+16c3a70e1e9b29346c43@syzkaller.appspotmail.com> wrote:
> > >
> > > Hello,
> > >
> > > syzbot found the following crash on:
> > >
> > > HEAD commit: 02495e76ded5 Add linux-next specific files for 20190130
> > > git tree: linux-next
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=12cf10df400000
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=a2b2e9c0bc43c14d
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=16c3a70e1e9b29346c43
> > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13266698c00000
> > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1715bb64c00000
> > >
> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+16c3a70e1e9b29346c43@syzkaller.appspotmail.com
> > >
> > > kasan: CONFIG_KASAN_INLINE enabled
> > > kasan: GPF could be caused by NULL-ptr deref or user memory access
> > > general protection fault: 0000 [#1] PREEMPT SMP KASAN
> > > CPU: 0 PID: 8092 Comm: syz-executor405 Not tainted 5.0.0-rc4-next-20190130
> > > #22
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > > Google 01/01/2011
> > > RIP: 0010:relay_set_buf_dentry kernel/relay.c:412 [inline]
> >
> > static inline void relay_set_buf_dentry(struct rchan_buf *buf,
> > struct dentry *dentry)
> > {
> > buf->dentry = dentry;
> > d_inode(buf->dentry)->i_size = buf->early_bytes; <--
> > }
> >
> > Doing a bisect landed on this:
> >
> > ff9fb72bc07705c00795ca48631f7fffe24d2c6b ("debugfs: return error
> > values, not NULL")
> >
> > If I revert this patch, I can't reproduce any more. I don't see a
> > relationship, though...
> >
> > My crash appears as:
> > [ 121.934378] BUG: unable to handle kernel NULL pointer dereference
> > at 0000000000000047
> > [ 121.937187] #PF error: [normal kernel read fault]
> > [ 121.938824] PGD 800000041f699067 P4D 800000041f699067 PUD 42d08f067 PMD 0
> > [ 121.941166] Oops: 0000 [#1] SMP PTI
> > [ 121.942381] CPU: 2 PID: 3134 Comm: relay Not tainted
> > 5.0.0-rc4-next-20190130 #1020
> > [ 121.943873] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > BIOS 1.10.2-1ubuntu1 04/01/2014
> > [ 121.945395] RIP: 0010:relay_open_buf.part.10+0x2b8/0x330
> > ...
> > [ 121.960021] Call Trace:
> > [ 121.960453] relay_open+0x18e/0x2c0
> > [ 121.961070] __blk_trace_setup+0x1af/0x350
> > [ 121.961777] blk_trace_ioctl+0x93/0x100
> >
> >
> > $ ./scripts/faddr2line vmlinux relay_open_buf.part.10+0x2b8/0x330
> > relay_open_buf.part.10+0x2b8/0x330:
> > relay_set_buf_dentry at kernel/relay.c:412
> > (inlined by) relay_open_buf at kernel/relay.c:458
> >
> > So it's the same location, but not sure about 0x47 offset. d_inode is
> > 0x58 from dentry. And i_size is 0x50 from inode. If this isn't NULL,
> > but rather an ERR_PTR, the errno is either:
> >
> > EBADF 9 Bad file descriptor
> > EEXIST 17 File exists
> >
> > Neither are used in the debugfs patch, but debugfs is clearly used in
> > do_blk_trace_setup():
> >
> > if (!blk_debugfs_root)
> > return -ENOENT;
> > ...
> > dir = debugfs_lookup(buts->name, blk_debugfs_root);
> > if (!dir)
> > bt->dir = dir = debugfs_create_dir(buts->name,
> > blk_debugfs_root);
> > if (!dir)
> > goto err;
> > ...
> > bt->rchan = relay_open("trace", dir, buts->buf_size,
> > buts->buf_nr, &blk_relay_callbacks, bt);
> >
> > Which is confirmed by the next line in my traceback:
> >
> > $ ./scripts/faddr2line vmlinux __blk_trace_setup+0x1af/0x350
> > __blk_trace_setup+0x1af/0x350:
> > do_blk_trace_setup at kernel/trace/blktrace.c:534
> > (inlined by) __blk_trace_setup at kernel/trace/blktrace.c:577
>
> Can you test the patch below?
This can be done as self-service by saying:
#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
master
(is it the right tree/base commit for your change? a patch can
generally be applied only to the tree/base commit that you used to
obtain the diff)
See https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
for details.
> thanks,
>
> greg k-h
>
> --------------
>
> diff --git a/kernel/relay.c b/kernel/relay.c
> index 04f248644e06..9e0f52375487 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -428,6 +428,8 @@ static struct dentry *relay_create_buf_file(struct rchan *chan,
> dentry = chan->cb->create_buf_file(tmpname, chan->parent,
> S_IRUSR, buf,
> &chan->is_global);
> + if (IS_ERR(dentry))
> + dentry = NULL;
>
> kfree(tmpname);
>
> @@ -461,7 +463,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
> dentry = chan->cb->create_buf_file(NULL, NULL,
> S_IRUSR, buf,
> &chan->is_global);
> - if (WARN_ON(dentry))
> + if (IS_ERR_OR_NULL(dentry))
> goto free_buf;
> }
[-- Attachment #2: relay.patch --]
[-- Type: text/x-patch, Size: 664 bytes --]
diff --git a/kernel/relay.c b/kernel/relay.c
index 04f248644e06..9e0f52375487 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -428,6 +428,8 @@ static struct dentry *relay_create_buf_file(struct rchan *chan,
dentry = chan->cb->create_buf_file(tmpname, chan->parent,
S_IRUSR, buf,
&chan->is_global);
+ if (IS_ERR(dentry))
+ dentry = NULL;
kfree(tmpname);
@@ -461,7 +463,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
dentry = chan->cb->create_buf_file(NULL, NULL,
S_IRUSR, buf,
&chan->is_global);
- if (WARN_ON(dentry))
+ if (IS_ERR_OR_NULL(dentry))
goto free_buf;
}
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: general protection fault in relay_open_buf
2019-01-31 11:16 ` Dmitry Vyukov
@ 2019-01-31 11:22 ` Greg KH
2019-01-31 11:28 ` Dmitry Vyukov
2019-01-31 11:53 ` syzbot
2019-01-31 11:35 ` syzbot
1 sibling, 2 replies; 15+ messages in thread
From: Greg KH @ 2019-01-31 11:22 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Kees Cook, Andrew Morton, syzbot, Eric Biggers, Souptick Joarder,
LKML, David Rientjes, syzkaller-bugs
[-- Attachment #1: Type: text/plain, Size: 4830 bytes --]
On Thu, Jan 31, 2019 at 12:16:42PM +0100, Dmitry Vyukov wrote:
> On Thu, Jan 31, 2019 at 11:51 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jan 31, 2019 at 10:54:18PM +1300, Kees Cook wrote:
> > > On Thu, Jan 31, 2019 at 7:53 AM syzbot
> > > <syzbot+16c3a70e1e9b29346c43@syzkaller.appspotmail.com> wrote:
> > > >
> > > > Hello,
> > > >
> > > > syzbot found the following crash on:
> > > >
> > > > HEAD commit: 02495e76ded5 Add linux-next specific files for 20190130
> > > > git tree: linux-next
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12cf10df400000
> > > > kernel config: https://syzkaller.appspot.com/x/.config?x=a2b2e9c0bc43c14d
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=16c3a70e1e9b29346c43
> > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13266698c00000
> > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1715bb64c00000
> > > >
> > > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > > Reported-by: syzbot+16c3a70e1e9b29346c43@syzkaller.appspotmail.com
> > > >
> > > > kasan: CONFIG_KASAN_INLINE enabled
> > > > kasan: GPF could be caused by NULL-ptr deref or user memory access
> > > > general protection fault: 0000 [#1] PREEMPT SMP KASAN
> > > > CPU: 0 PID: 8092 Comm: syz-executor405 Not tainted 5.0.0-rc4-next-20190130
> > > > #22
> > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > > > Google 01/01/2011
> > > > RIP: 0010:relay_set_buf_dentry kernel/relay.c:412 [inline]
> > >
> > > static inline void relay_set_buf_dentry(struct rchan_buf *buf,
> > > struct dentry *dentry)
> > > {
> > > buf->dentry = dentry;
> > > d_inode(buf->dentry)->i_size = buf->early_bytes; <--
> > > }
> > >
> > > Doing a bisect landed on this:
> > >
> > > ff9fb72bc07705c00795ca48631f7fffe24d2c6b ("debugfs: return error
> > > values, not NULL")
> > >
> > > If I revert this patch, I can't reproduce any more. I don't see a
> > > relationship, though...
> > >
> > > My crash appears as:
> > > [ 121.934378] BUG: unable to handle kernel NULL pointer dereference
> > > at 0000000000000047
> > > [ 121.937187] #PF error: [normal kernel read fault]
> > > [ 121.938824] PGD 800000041f699067 P4D 800000041f699067 PUD 42d08f067 PMD 0
> > > [ 121.941166] Oops: 0000 [#1] SMP PTI
> > > [ 121.942381] CPU: 2 PID: 3134 Comm: relay Not tainted
> > > 5.0.0-rc4-next-20190130 #1020
> > > [ 121.943873] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > > BIOS 1.10.2-1ubuntu1 04/01/2014
> > > [ 121.945395] RIP: 0010:relay_open_buf.part.10+0x2b8/0x330
> > > ...
> > > [ 121.960021] Call Trace:
> > > [ 121.960453] relay_open+0x18e/0x2c0
> > > [ 121.961070] __blk_trace_setup+0x1af/0x350
> > > [ 121.961777] blk_trace_ioctl+0x93/0x100
> > >
> > >
> > > $ ./scripts/faddr2line vmlinux relay_open_buf.part.10+0x2b8/0x330
> > > relay_open_buf.part.10+0x2b8/0x330:
> > > relay_set_buf_dentry at kernel/relay.c:412
> > > (inlined by) relay_open_buf at kernel/relay.c:458
> > >
> > > So it's the same location, but not sure about 0x47 offset. d_inode is
> > > 0x58 from dentry. And i_size is 0x50 from inode. If this isn't NULL,
> > > but rather an ERR_PTR, the errno is either:
> > >
> > > EBADF 9 Bad file descriptor
> > > EEXIST 17 File exists
> > >
> > > Neither are used in the debugfs patch, but debugfs is clearly used in
> > > do_blk_trace_setup():
> > >
> > > if (!blk_debugfs_root)
> > > return -ENOENT;
> > > ...
> > > dir = debugfs_lookup(buts->name, blk_debugfs_root);
> > > if (!dir)
> > > bt->dir = dir = debugfs_create_dir(buts->name,
> > > blk_debugfs_root);
> > > if (!dir)
> > > goto err;
> > > ...
> > > bt->rchan = relay_open("trace", dir, buts->buf_size,
> > > buts->buf_nr, &blk_relay_callbacks, bt);
> > >
> > > Which is confirmed by the next line in my traceback:
> > >
> > > $ ./scripts/faddr2line vmlinux __blk_trace_setup+0x1af/0x350
> > > __blk_trace_setup+0x1af/0x350:
> > > do_blk_trace_setup at kernel/trace/blktrace.c:534
> > > (inlined by) __blk_trace_setup at kernel/trace/blktrace.c:577
> >
> > Can you test the patch below?
>
>
> This can be done as self-service by saying:
>
> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> master
>
> (is it the right tree/base commit for your change? a patch can
> generally be applied only to the tree/base commit that you used to
> obtain the diff)
It was close, wrong tree, try this:
#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git driver-core-linus
And let's see if it works :)
[-- Attachment #2: x --]
[-- Type: text/plain, Size: 665 bytes --]
diff --git a/kernel/relay.c b/kernel/relay.c
index 04f248644e06..9e0f52375487 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -428,6 +428,8 @@ static struct dentry *relay_create_buf_file(struct rchan *chan,
dentry = chan->cb->create_buf_file(tmpname, chan->parent,
S_IRUSR, buf,
&chan->is_global);
+ if (IS_ERR(dentry))
+ dentry = NULL;
kfree(tmpname);
@@ -461,7 +463,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
dentry = chan->cb->create_buf_file(NULL, NULL,
S_IRUSR, buf,
&chan->is_global);
- if (WARN_ON(dentry))
+ if (IS_ERR_OR_NULL(dentry))
goto free_buf;
}
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: general protection fault in relay_open_buf
2019-01-31 11:22 ` Greg KH
@ 2019-01-31 11:28 ` Dmitry Vyukov
2019-01-31 11:53 ` syzbot
1 sibling, 0 replies; 15+ messages in thread
From: Dmitry Vyukov @ 2019-01-31 11:28 UTC (permalink / raw)
To: Greg KH
Cc: Kees Cook, Andrew Morton, syzbot, Eric Biggers, Souptick Joarder,
LKML, David Rientjes, syzkaller-bugs
On Thu, Jan 31, 2019 at 12:22 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jan 31, 2019 at 12:16:42PM +0100, Dmitry Vyukov wrote:
> > On Thu, Jan 31, 2019 at 11:51 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Jan 31, 2019 at 10:54:18PM +1300, Kees Cook wrote:
> > > > On Thu, Jan 31, 2019 at 7:53 AM syzbot
> > > > <syzbot+16c3a70e1e9b29346c43@syzkaller.appspotmail.com> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > syzbot found the following crash on:
> > > > >
> > > > > HEAD commit: 02495e76ded5 Add linux-next specific files for 20190130
> > > > > git tree: linux-next
> > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12cf10df400000
> > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=a2b2e9c0bc43c14d
> > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=16c3a70e1e9b29346c43
> > > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13266698c00000
> > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1715bb64c00000
> > > > >
> > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > > > Reported-by: syzbot+16c3a70e1e9b29346c43@syzkaller.appspotmail.com
> > > > >
> > > > > kasan: CONFIG_KASAN_INLINE enabled
> > > > > kasan: GPF could be caused by NULL-ptr deref or user memory access
> > > > > general protection fault: 0000 [#1] PREEMPT SMP KASAN
> > > > > CPU: 0 PID: 8092 Comm: syz-executor405 Not tainted 5.0.0-rc4-next-20190130
> > > > > #22
> > > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > > > > Google 01/01/2011
> > > > > RIP: 0010:relay_set_buf_dentry kernel/relay.c:412 [inline]
> > > >
> > > > static inline void relay_set_buf_dentry(struct rchan_buf *buf,
> > > > struct dentry *dentry)
> > > > {
> > > > buf->dentry = dentry;
> > > > d_inode(buf->dentry)->i_size = buf->early_bytes; <--
> > > > }
> > > >
> > > > Doing a bisect landed on this:
> > > >
> > > > ff9fb72bc07705c00795ca48631f7fffe24d2c6b ("debugfs: return error
> > > > values, not NULL")
> > > >
> > > > If I revert this patch, I can't reproduce any more. I don't see a
> > > > relationship, though...
> > > >
> > > > My crash appears as:
> > > > [ 121.934378] BUG: unable to handle kernel NULL pointer dereference
> > > > at 0000000000000047
> > > > [ 121.937187] #PF error: [normal kernel read fault]
> > > > [ 121.938824] PGD 800000041f699067 P4D 800000041f699067 PUD 42d08f067 PMD 0
> > > > [ 121.941166] Oops: 0000 [#1] SMP PTI
> > > > [ 121.942381] CPU: 2 PID: 3134 Comm: relay Not tainted
> > > > 5.0.0-rc4-next-20190130 #1020
> > > > [ 121.943873] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > > > BIOS 1.10.2-1ubuntu1 04/01/2014
> > > > [ 121.945395] RIP: 0010:relay_open_buf.part.10+0x2b8/0x330
> > > > ...
> > > > [ 121.960021] Call Trace:
> > > > [ 121.960453] relay_open+0x18e/0x2c0
> > > > [ 121.961070] __blk_trace_setup+0x1af/0x350
> > > > [ 121.961777] blk_trace_ioctl+0x93/0x100
> > > >
> > > >
> > > > $ ./scripts/faddr2line vmlinux relay_open_buf.part.10+0x2b8/0x330
> > > > relay_open_buf.part.10+0x2b8/0x330:
> > > > relay_set_buf_dentry at kernel/relay.c:412
> > > > (inlined by) relay_open_buf at kernel/relay.c:458
> > > >
> > > > So it's the same location, but not sure about 0x47 offset. d_inode is
> > > > 0x58 from dentry. And i_size is 0x50 from inode. If this isn't NULL,
> > > > but rather an ERR_PTR, the errno is either:
> > > >
> > > > EBADF 9 Bad file descriptor
> > > > EEXIST 17 File exists
> > > >
> > > > Neither are used in the debugfs patch, but debugfs is clearly used in
> > > > do_blk_trace_setup():
> > > >
> > > > if (!blk_debugfs_root)
> > > > return -ENOENT;
> > > > ...
> > > > dir = debugfs_lookup(buts->name, blk_debugfs_root);
> > > > if (!dir)
> > > > bt->dir = dir = debugfs_create_dir(buts->name,
> > > > blk_debugfs_root);
> > > > if (!dir)
> > > > goto err;
> > > > ...
> > > > bt->rchan = relay_open("trace", dir, buts->buf_size,
> > > > buts->buf_nr, &blk_relay_callbacks, bt);
> > > >
> > > > Which is confirmed by the next line in my traceback:
> > > >
> > > > $ ./scripts/faddr2line vmlinux __blk_trace_setup+0x1af/0x350
> > > > __blk_trace_setup+0x1af/0x350:
> > > > do_blk_trace_setup at kernel/trace/blktrace.c:534
> > > > (inlined by) __blk_trace_setup at kernel/trace/blktrace.c:577
> > >
> > > Can you test the patch below?
> >
> >
> > This can be done as self-service by saying:
> >
> > #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > master
> >
> > (is it the right tree/base commit for your change? a patch can
> > generally be applied only to the tree/base commit that you used to
> > obtain the diff)
>
> It was close, wrong tree, try this:
>
> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git driver-core-linus
>
> And let's see if it works :)
Just in case, you can actually post patches inline, it should work.
It's just my client is incapable of preserving whitespaces, so I have
to re-attach them (they are also applied with --ignore-whitespace, but
just to be safe).
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: general protection fault in relay_open_buf
2019-01-31 11:22 ` Greg KH
2019-01-31 11:28 ` Dmitry Vyukov
@ 2019-01-31 11:53 ` syzbot
1 sibling, 0 replies; 15+ messages in thread
From: syzbot @ 2019-01-31 11:53 UTC (permalink / raw)
To: akpm, dvyukov, ebiggers, gregkh, jrdr.linux, keescook,
linux-kernel, rientjes, syzkaller-bugs
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger
crash:
Reported-and-tested-by:
syzbot+16c3a70e1e9b29346c43@syzkaller.appspotmail.com
Tested on:
commit: 37ea7b630ae5 debugfs: debugfs_lookup() should return NULL ..
git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
driver-core-linus
kernel config: https://syzkaller.appspot.com/x/.config?x=a04dcc98f2be2fd8
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
patch: https://syzkaller.appspot.com/x/patch.diff?x=16ecad28c00000
Note: testing is done by a robot and is best-effort only.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: general protection fault in relay_open_buf
2019-01-31 11:16 ` Dmitry Vyukov
2019-01-31 11:22 ` Greg KH
@ 2019-01-31 11:35 ` syzbot
1 sibling, 0 replies; 15+ messages in thread
From: syzbot @ 2019-01-31 11:35 UTC (permalink / raw)
To: akpm, dvyukov, ebiggers, gregkh, jrdr.linux, keescook,
linux-kernel, rientjes, syzkaller-bugs
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger
crash:
Reported-and-tested-by:
syzbot+16c3a70e1e9b29346c43@syzkaller.appspotmail.com
Tested on:
commit: af0c9af1b3f6 fs/dcache: Track & report number of negative ..
git tree: upstream
kernel config: https://syzkaller.appspot.com/x/.config?x=3495238483eab50f
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
patch: https://syzkaller.appspot.com/x/patch.diff?x=166d48df400000
Note: testing is done by a robot and is best-effort only.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: general protection fault in relay_open_buf
2019-01-31 10:51 ` Greg KH
2019-01-31 11:16 ` Dmitry Vyukov
@ 2019-01-31 11:29 ` Tetsuo Handa
2019-01-31 11:54 ` Greg KH
2019-01-31 18:31 ` Kees Cook
2019-02-01 3:57 ` Al Viro
3 siblings, 1 reply; 15+ messages in thread
From: Tetsuo Handa @ 2019-01-31 11:29 UTC (permalink / raw)
To: Greg KH, Kees Cook
Cc: Andrew Morton, syzbot, Eric Biggers, Souptick Joarder, LKML,
David Rientjes, syzkaller-bugs
On 2019/01/31 19:51, Greg KH wrote:
> Can you test the patch below?
You can ask syzbot to test the patch. But
> @@ -461,7 +463,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
> dentry = chan->cb->create_buf_file(NULL, NULL,
> S_IRUSR, buf,
> &chan->is_global);
> - if (WARN_ON(dentry))
> + if (IS_ERR_OR_NULL(dentry))
> goto free_buf;
are you trying to fix a different bug together that old code was by error bailing
out when chan->cb->create_buf_file() returned a valid "struct dentry *" ?
I don't know what WARN_ON() due to a valid "struct dentry *" means...
> }
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: general protection fault in relay_open_buf
2019-01-31 11:29 ` Tetsuo Handa
@ 2019-01-31 11:54 ` Greg KH
0 siblings, 0 replies; 15+ messages in thread
From: Greg KH @ 2019-01-31 11:54 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Kees Cook, Andrew Morton, syzbot, Eric Biggers, Souptick Joarder,
LKML, David Rientjes, syzkaller-bugs
On Thu, Jan 31, 2019 at 08:29:37PM +0900, Tetsuo Handa wrote:
> On 2019/01/31 19:51, Greg KH wrote:
> > Can you test the patch below?
>
> You can ask syzbot to test the patch. But
>
> > @@ -461,7 +463,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
> > dentry = chan->cb->create_buf_file(NULL, NULL,
> > S_IRUSR, buf,
> > &chan->is_global);
> > - if (WARN_ON(dentry))
> > + if (IS_ERR_OR_NULL(dentry))
> > goto free_buf;
>
> are you trying to fix a different bug together that old code was by error bailing
> out when chan->cb->create_buf_file() returned a valid "struct dentry *" ?
> I don't know what WARN_ON() due to a valid "struct dentry *" means...
I don't either, I'm guessing that's a code path that has never been
taken, or everyone just ignores it :)
Anyway, I fixed it up to be safe. And the reproducer shows it now works
properly too.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: general protection fault in relay_open_buf
2019-01-31 10:51 ` Greg KH
2019-01-31 11:16 ` Dmitry Vyukov
2019-01-31 11:29 ` Tetsuo Handa
@ 2019-01-31 18:31 ` Kees Cook
2019-01-31 18:46 ` Greg KH
2019-02-01 3:57 ` Al Viro
3 siblings, 1 reply; 15+ messages in thread
From: Kees Cook @ 2019-01-31 18:31 UTC (permalink / raw)
To: Greg KH
Cc: Andrew Morton, syzbot, Eric Biggers, Souptick Joarder, LKML,
David Rientjes, syzkaller-bugs
On Thu, Jan 31, 2019 at 11:51 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> Can you test the patch below?
>
> thanks,
>
> greg k-h
>
> --------------
>
> diff --git a/kernel/relay.c b/kernel/relay.c
> index 04f248644e06..9e0f52375487 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -428,6 +428,8 @@ static struct dentry *relay_create_buf_file(struct rchan *chan,
> dentry = chan->cb->create_buf_file(tmpname, chan->parent,
> S_IRUSR, buf,
> &chan->is_global);
> + if (IS_ERR(dentry))
> + dentry = NULL;
>
> kfree(tmpname);
>
> @@ -461,7 +463,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
> dentry = chan->cb->create_buf_file(NULL, NULL,
> S_IRUSR, buf,
> &chan->is_global);
> - if (WARN_ON(dentry))
> + if (IS_ERR_OR_NULL(dentry))
> goto free_buf;
> }
>
Thanks! (Can we find other cases of this with static analysis?)
Fixes: ff9fb72bc077 ("debugfs: return error values, not NULL")
Reported-by: syzbot+16c3a70e1e9b29346c43@syzkaller.appspotmail.com
Tested-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: general protection fault in relay_open_buf
2019-01-31 18:31 ` Kees Cook
@ 2019-01-31 18:46 ` Greg KH
0 siblings, 0 replies; 15+ messages in thread
From: Greg KH @ 2019-01-31 18:46 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, syzbot, Eric Biggers, Souptick Joarder, LKML,
David Rientjes, syzkaller-bugs
On Fri, Feb 01, 2019 at 07:31:48AM +1300, Kees Cook wrote:
> On Thu, Jan 31, 2019 at 11:51 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > Can you test the patch below?
> >
> > thanks,
> >
> > greg k-h
> >
> > --------------
> >
> > diff --git a/kernel/relay.c b/kernel/relay.c
> > index 04f248644e06..9e0f52375487 100644
> > --- a/kernel/relay.c
> > +++ b/kernel/relay.c
> > @@ -428,6 +428,8 @@ static struct dentry *relay_create_buf_file(struct rchan *chan,
> > dentry = chan->cb->create_buf_file(tmpname, chan->parent,
> > S_IRUSR, buf,
> > &chan->is_global);
> > + if (IS_ERR(dentry))
> > + dentry = NULL;
> >
> > kfree(tmpname);
> >
> > @@ -461,7 +463,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
> > dentry = chan->cb->create_buf_file(NULL, NULL,
> > S_IRUSR, buf,
> > &chan->is_global);
> > - if (WARN_ON(dentry))
> > + if (IS_ERR_OR_NULL(dentry))
> > goto free_buf;
> > }
> >
>
> Thanks! (Can we find other cases of this with static analysis?)
Probably. I have over 100 patches to help clean up a lot of the debugfs
mess. But it is very rare that someone actually tries to use the result
of a debugfs call as a "real" dentry, except to pass it back into
another debugfs call. I "think" I have now caught all of those cases,
and if you can come up with some kind of rule for this, that would be
great.
But note, the create_buf_file() callback is the one that does the
debugfs call, so trying to figure out where that is coming from, what it
does, and what the dentry is later used for, spans lots of different
subsystems and files. I don't think we have tools to do that, other
than grep :)
greg k-h
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: general protection fault in relay_open_buf
2019-01-31 10:51 ` Greg KH
` (2 preceding siblings ...)
2019-01-31 18:31 ` Kees Cook
@ 2019-02-01 3:57 ` Al Viro
2019-02-01 9:07 ` Greg KH
3 siblings, 1 reply; 15+ messages in thread
From: Al Viro @ 2019-02-01 3:57 UTC (permalink / raw)
To: Greg KH
Cc: Kees Cook, Andrew Morton, syzbot, Eric Biggers, Souptick Joarder,
LKML, David Rientjes, syzkaller-bugs
On Thu, Jan 31, 2019 at 11:51:52AM +0100, Greg KH wrote:
> Can you test the patch below?
>
> thanks,
>
> greg k-h
>
> @@ -461,7 +463,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
> dentry = chan->cb->create_buf_file(NULL, NULL,
> S_IRUSR, buf,
> &chan->is_global);
> - if (WARN_ON(dentry))
> + if (IS_ERR_OR_NULL(dentry))
> goto free_buf;
Huh? That makes no sense; is it IS_ERR on error or is it NULL
on error, or what? Besides, how did it work before?
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: general protection fault in relay_open_buf
2019-02-01 3:57 ` Al Viro
@ 2019-02-01 9:07 ` Greg KH
0 siblings, 0 replies; 15+ messages in thread
From: Greg KH @ 2019-02-01 9:07 UTC (permalink / raw)
To: Al Viro
Cc: Kees Cook, Andrew Morton, syzbot, Eric Biggers, Souptick Joarder,
LKML, David Rientjes, syzkaller-bugs
On Fri, Feb 01, 2019 at 03:57:48AM +0000, Al Viro wrote:
> On Thu, Jan 31, 2019 at 11:51:52AM +0100, Greg KH wrote:
> > Can you test the patch below?
> >
> > thanks,
> >
> > greg k-h
> >
>
> > @@ -461,7 +463,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
> > dentry = chan->cb->create_buf_file(NULL, NULL,
> > S_IRUSR, buf,
> > &chan->is_global);
> > - if (WARN_ON(dentry))
> > + if (IS_ERR_OR_NULL(dentry))
> > goto free_buf;
>
> Huh? That makes no sense; is it IS_ERR on error or is it NULL
> on error, or what?
Some of the fuction pointers seem to do one, some the other. I've
submitted patches to them to unify them now. Will take bit for those to
wind through the merges.
> Besides, how did it work before?
Obviously it never did at all :(
thanks,
greg k-h
^ permalink raw reply [flat|nested] 15+ messages in thread