general protection fault in fanotify_handle_event

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: e53f31bf Merge tag '5.1-rc5-smb3-fixes' of git://git.samba..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=174ea35b200000
kernel config: https://syzkaller.appspot.com/x/.config?x=856fc6d0fbbeede9
dashboard link: https://syzkaller.appspot.com/bug?extid=15927486a4f1bfcbaf91
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=166cf35b200000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+159274...@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: 1 PID: 12999 Comm: syz-executor.0 Not tainted 5.1.0-rc5+ #73
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:fanotify_get_fsid fs/notify/fanotify/fanotify.c:352 [inline]
RIP: 0010:fanotify_handle_event+0x7d0/0xc40
fs/notify/fanotify/fanotify.c:412
Code: ff ff 48 8b 18 48 8d 7b 68 48 89 f8 48 c1 e8 03 42 80 3c 38 00 0f 85
47 04 00 00 48 8b 5b 68 48 8d 7b 3c 48 89 fa 48 c1 ea 03 <42> 0f b6 0c 3a
48 89 fa 83 e2 07 83 c2 03 38 ca 7c 08 84 c9 0f 85
RSP: 0018:ffff8880a8947b78 EFLAGS: 00010203
RAX: 1ffff110127b460d RBX: 0000000000000000 RCX: ffffffff81c41e9e
RDX: 0000000000000007 RSI: ffffffff81c41eab RDI: 000000000000003c
RBP: ffff8880a8947cc0 R08: ffff888087904240 R09: 0000000000000000
R10: ffff888087904b10 R11: ffff888087904240 R12: 0000000000000002
R13: 0000000000000000 R14: 0000000000000001 R15: dffffc0000000000
FS: 00007f9f15bf7700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f9f15bb4db8 CR3: 000000008d545000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
send_to_group fs/notify/fsnotify.c:243 [inline]
fsnotify+0x725/0xbc0 fs/notify/fsnotify.c:381
fsnotify_path include/linux/fsnotify.h:54 [inline]
fsnotify_path include/linux/fsnotify.h:47 [inline]
fsnotify_modify include/linux/fsnotify.h:263 [inline]
vfs_write+0x4dc/0x580 fs/read_write.c:551
ksys_write+0x14f/0x2d0 fs/read_write.c:599
__do_sys_write fs/read_write.c:611 [inline]
__se_sys_write fs/read_write.c:608 [inline]
__x64_sys_write+0x73/0xb0 fs/read_write.c:608
do_syscall_64+0x103/0x610 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x458c29
Code: ad b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 7b b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f9f15bf6c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000458c29
RDX: 0000000000000007 RSI: 0000000020000080 RDI: 0000000000000005
RBP: 000000000073bf00 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f9f15bf76d4
R13: 00000000004c8386 R14: 00000000004de8b8 R15: 00000000ffffffff
Modules linked in:
---[ end trace 026b9c3311d87f36 ]---
RIP: 0010:fanotify_get_fsid fs/notify/fanotify/fanotify.c:352 [inline]
RIP: 0010:fanotify_handle_event+0x7d0/0xc40
fs/notify/fanotify/fanotify.c:412
Code: ff ff 48 8b 18 48 8d 7b 68 48 89 f8 48 c1 e8 03 42 80 3c 38 00 0f 85
47 04 00 00 48 8b 5b 68 48 8d 7b 3c 48 89 fa 48 c1 ea 03 <42> 0f b6 0c 3a
48 89 fa 83 e2 07 83 c2 03 38 ca 7c 08 84 c9 0f 85
RSP: 0018:ffff8880a8947b78 EFLAGS: 00010203
RAX: 1ffff110127b460d RBX: 0000000000000000 RCX: ffffffff81c41e9e
RDX: 0000000000000007 RSI: ffffffff81c41eab RDI: 000000000000003c
RBP: ffff8880a8947cc0 R08: ffff888087904240 R09: 0000000000000000
R10: ffff888087904b10 R11: ffff888087904240 R12: 0000000000000002
R13: 0000000000000000 R14: 0000000000000001 R15: dffffc0000000000
FS: 00007f9f15bf7700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f9f15bb4db8 CR3: 000000008d545000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

[syzbot]

syzbot has bisected this bug to:

commit 77115225acc67d9ac4b15f04dd138006b9cd1ef2
Author: Amir Goldstein <amir...@gmail.com>
Date: Thu Jan 10 17:04:37 2019 +0000

fanotify: cache fsid in fsnotify_mark_connector

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1627632d200000
start commit: 3f018f4a Add linux-next specific files for 20190418
git tree: linux-next
final crash: https://syzkaller.appspot.com/x/report.txt?x=1527632d200000
console output: https://syzkaller.appspot.com/x/log.txt?x=1127632d200000
kernel config: https://syzkaller.appspot.com/x/.config?x=faa7bdc352fc157e
dashboard link: https://syzkaller.appspot.com/bug?extid=15927486a4f1bfcbaf91
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=155543d3200000

Reported-by: syzbot+159274...@syzkaller.appspotmail.com
Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

[Amir Goldstein]

Jan,

It looks like lockless access to mark->connector is not safe as there is nothing
preventing a reader from seeing a mark on object list without seeing the
mark->connector assignment.

It made me wonder if (!mark->connector) check in fsnotify_put_mark() is safe.
I couldn't find any call site where that would be a problem, but perhaps
we should be more careful?

Anyway, it seems that fsnotify_put_mark() uses the non NULL mark->connector
as the indication that mark is on object list, so just assigning mark->connector
before adding to object list won't do.

Since a reference of mark is our guaranty that mark->connector is not
going away, I guess we could do opportunistic test for non NULL
mark->connector from lockless path, if that fails, we check again
with mark->lock held and if that fails something went wrong.

Another option is to teach fsnotify_first_mark() and fsnotify_next_mark()
to skip over marks with NULL mark->connector.

What do you think? Did I over complicate this?

Thanks,
Amir.

[Jan Kara]

On Fri 19-04-19 12:33:02, Amir Goldstein wrote:
> On Thu, Apr 18, 2019 at 8:14 PM syzbot
> <syzbot+159274...@syzkaller.appspotmail.com> wrote:
> >
> > syzbot has bisected this bug to:
> >
> > commit 77115225acc67d9ac4b15f04dd138006b9cd1ef2
> > Author: Amir Goldstein <amir...@gmail.com>
> > Date: Thu Jan 10 17:04:37 2019 +0000
> >
> > fanotify: cache fsid in fsnotify_mark_connector
> >
> > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1627632d200000
> > start commit: 3f018f4a Add linux-next specific files for 20190418
> > git tree: linux-next
> > final crash: https://syzkaller.appspot.com/x/report.txt?x=1527632d200000
> > console output: https://syzkaller.appspot.com/x/log.txt?x=1127632d200000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=faa7bdc352fc157e
> > dashboard link: https://syzkaller.appspot.com/bug?extid=15927486a4f1bfcbaf91
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=155543d3200000
> >
> > Reported-by: syzbot+159274...@syzkaller.appspotmail.com
> > Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")
> >

Hi Amir!

> It looks like lockless access to mark->connector is not safe as there is
> nothing preventing a reader from seeing a mark on object list without
> seeing the mark->connector assignment.

Yes, that seems to be possible.

> It made me wonder if (!mark->connector) check in fsnotify_put_mark() is safe.
> I couldn't find any call site where that would be a problem, but perhaps
> we should be more careful?

Why? That check is there really only to catch destruction of a mark that
was never attached (i.e., allocated but later never used due to some
error). Otherwise we should always have mark->connector set.

> Anyway, it seems that fsnotify_put_mark() uses the non NULL mark->connector
> as the indication that mark is on object list, so just assigning mark->connector
> before adding to object list won't do.
>
> Since a reference of mark is our guaranty that mark->connector is not
> going away, I guess we could do opportunistic test for non NULL
> mark->connector from lockless path, if that fails, we check again
> with mark->lock held and if that fails something went wrong.
>
> Another option is to teach fsnotify_first_mark() and fsnotify_next_mark()
> to skip over marks with NULL mark->connector.
>
> What do you think? Did I over complicate this?

I'd prefer if we could make only fully initialized marks visible on
connector's list. Just to make things simple in the fast path of handling
events. So I'd just set mark->connector before adding mark to connector's
list and having smp_wmb() there to force ordering in
fsnotify_add_mark_list(). And we should use READ_ONCE() as a counter-part
to this in fanotify_get_fsid(). It is somewhat unfortunate that there are
three different ways how fsnotify_add_mark_list() can add mark to a list so
we may need to either repeat this in three different places or just use
some macro magic like:

#define fanotify_attach_obj_list(where, mark, conn, head) \
do { \
mark->connector = conn; \
smp_wmb(); \
hlist_add_##where##_rcu(&mark->obj_list, head); \
} while (0)

And I would not really worry about fsnotify_put_mark() - if it ever sees
mark->connector set, mark really is on the connector's list and
fsnotify_put_mark() does the right thing (i.e. locks connector->lock if
needed).

Honza
--
Jan Kara <ja...@suse.com>
SUSE Labs, CR

[Amir Goldstein]

On Tue, Apr 23, 2019 at 7:27 PM Jan Kara <ja...@suse.cz> wrote:
>
> On Fri 19-04-19 12:33:02, Amir Goldstein wrote:
> > On Thu, Apr 18, 2019 at 8:14 PM syzbot
> > <syzbot+159274...@syzkaller.appspotmail.com> wrote:
> > >
> > > syzbot has bisected this bug to:
> > >
> > > commit 77115225acc67d9ac4b15f04dd138006b9cd1ef2
> > > Author: Amir Goldstein <amir...@gmail.com>
> > > Date: Thu Jan 10 17:04:37 2019 +0000
> > >
> > > fanotify: cache fsid in fsnotify_mark_connector
> > >
> > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1627632d200000
> > > start commit: 3f018f4a Add linux-next specific files for 20190418
> > > git tree: linux-next
> > > final crash: https://syzkaller.appspot.com/x/report.txt?x=1527632d200000
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=1127632d200000
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=faa7bdc352fc157e
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=15927486a4f1bfcbaf91
> > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=155543d3200000
> > >
> > > Reported-by: syzbot+159274...@syzkaller.appspotmail.com
> > > Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")
> > >
>

[...]

>
> I'd prefer if we could make only fully initialized marks visible on
> connector's list. Just to make things simple in the fast path of handling
> events. So I'd just set mark->connector before adding mark to connector's
> list and having smp_wmb() there to force ordering in
> fsnotify_add_mark_list(). And we should use READ_ONCE() as a counter-part
> to this in fanotify_get_fsid(). It is somewhat unfortunate that there are
> three different ways how fsnotify_add_mark_list() can add mark to a list so
> we may need to either repeat this in three different places or just use
> some macro magic like:
>
> #define fanotify_attach_obj_list(where, mark, conn, head) \
> do { \
> mark->connector = conn; \
> smp_wmb(); \
> hlist_add_##where##_rcu(&mark->obj_list, head); \
> } while (0)
>
> And I would not really worry about fsnotify_put_mark() - if it ever sees
> mark->connector set, mark really is on the connector's list and
> fsnotify_put_mark() does the right thing (i.e. locks connector->lock if
> needed).
>

Sounds good to me.

I was worried about fsnotify_put_mark() NOT seeing mark->connector set
even though it is on the list (add mark; get lockless ref; remove
mark; put lockless ref).
Its subtle, not sure if possible.

I am assuming you will be sending the above patch to syzbot for testing.
If you want me do help with anything please let me know.

Thanks,
Amir.

[Jan Kara]

Not possible for the cases it matters. fsnotify_put_mark() just decrements
the refcount. If someone is adding mark to the list, he is by definition
holding one reference so racing fsnotify_put_mark() cannot be dropping the
last one and so the result of the check doesn't really matter.

> I am assuming you will be sending the above patch to syzbot for testing.
> If you want me do help with anything please let me know.

OK, I'll write it and send it.

[Jan Kara]

Let syzbot test the fix:

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer still triggered
crash:
general protection fault in fanotify_handle_event

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: 13565 Comm: syz-executor.2 Not tainted 5.1.0-rc6+ #1

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:fanotify_get_fsid fs/notify/fanotify/fanotify.c:352 [inline]
RIP: 0010:fanotify_handle_event+0x7d0/0xc40
fs/notify/fanotify/fanotify.c:412
Code: ff ff 48 8b 18 48 8d 7b 68 48 89 f8 48 c1 e8 03 42 80 3c 38 00 0f 85
47 04 00 00 48 8b 5b 68 48 8d 7b 3c 48 89 fa 48 c1 ea 03 <42> 0f b6 0c 3a
48 89 fa 83 e2 07 83 c2 03 38 ca 7c 08 84 c9 0f 85

RSP: 0018:ffff888085eb7b78 EFLAGS: 00010203
RAX: 1ffff11013db05ab RBX: 0000000000000000 RCX: ffffffff81c427ae
RDX: 0000000000000007 RSI: ffffffff81c427bb RDI: 000000000000003c
RBP: ffff888085eb7cc0 R08: ffff88808b0926c0 R09: 0000000000000000
R10: ffff88808b092f90 R11: ffff88808b0926c0 R12: 0000000000000002

R13: 0000000000000000 R14: 0000000000000001 R15: dffffc0000000000

FS: 00007ff801b58700(0000) GS:ffff8880ae800000(0000) knlGS:0000000000000000

CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033

CR2: 00007f500e416000 CR3: 00000000a3f17000 CR4: 00000000001406f0

Call Trace:
send_to_group fs/notify/fsnotify.c:243 [inline]
fsnotify+0x725/0xbc0 fs/notify/fsnotify.c:381
fsnotify_path include/linux/fsnotify.h:54 [inline]
fsnotify_path include/linux/fsnotify.h:47 [inline]
fsnotify_modify include/linux/fsnotify.h:263 [inline]
vfs_write+0x4dc/0x580 fs/read_write.c:551
ksys_write+0x14f/0x2d0 fs/read_write.c:599
__do_sys_write fs/read_write.c:611 [inline]
__se_sys_write fs/read_write.c:608 [inline]
__x64_sys_write+0x73/0xb0 fs/read_write.c:608
do_syscall_64+0x103/0x610 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x458c29
Code: ad b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 7b b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00

RSP: 002b:00007ff801b57c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000001

RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000458c29
RDX: 0000000000000007 RSI: 0000000020000080 RDI: 0000000000000005
RBP: 000000000073bf00 R08: 0000000000000000 R09: 0000000000000000

R10: 0000000000000000 R11: 0000000000000246 R12: 00007ff801b586d4

R13: 00000000004c8386 R14: 00000000004de8b8 R15: 00000000ffffffff
Modules linked in:

---[ end trace 1767f2144c7cb47e ]---

RIP: 0010:fanotify_get_fsid fs/notify/fanotify/fanotify.c:352 [inline]
RIP: 0010:fanotify_handle_event+0x7d0/0xc40
fs/notify/fanotify/fanotify.c:412
Code: ff ff 48 8b 18 48 8d 7b 68 48 89 f8 48 c1 e8 03 42 80 3c 38 00 0f 85
47 04 00 00 48 8b 5b 68 48 8d 7b 3c 48 89 fa 48 c1 ea 03 <42> 0f b6 0c 3a
48 89 fa 83 e2 07 83 c2 03 38 ca 7c 08 84 c9 0f 85

RSP: 0018:ffff888085eb7b78 EFLAGS: 00010203
RAX: 1ffff11013db05ab RBX: 0000000000000000 RCX: ffffffff81c427ae
RDX: 0000000000000007 RSI: ffffffff81c427bb RDI: 000000000000003c
RBP: ffff888085eb7cc0 R08: ffff88808b0926c0 R09: 0000000000000000
R10: ffff88808b092f90 R11: ffff88808b0926c0 R12: 0000000000000002

R13: 0000000000000000 R14: 0000000000000001 R15: dffffc0000000000

FS: 00007ff801b58700(0000) GS:ffff8880ae800000(0000) knlGS:0000000000000000

CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033

CR2: 00007f500e416000 CR3: 00000000a3f17000 CR4: 00000000001406f0


Tested on:

commit: 8c7008a0 fsnotify: Fix NULL ptr deref in fanotify_get_fsid()
git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
console output: https://syzkaller.appspot.com/x/log.txt?x=13020ef4a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=a42d110b47dd6b36

[Jan Kara]

Let's try new version of the patch:

[syzbot]

Hello,

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

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

Tested on:

commit: b1da6a51 fsnotify: Fix NULL ptr deref in fanotify_get_fsid()

git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify

kernel config: https://syzkaller.appspot.com/x/.config?x=a42d110b47dd6b36
compiler: gcc (GCC) 9.0.0 20181231 (experimental)

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