Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1335969 > unrolled thread
| Started by | Mickaël Salaün <mic@digikod.net> |
|---|---|
| First post | 2016-02-17 02:00 +0100 |
| Last post | 2016-02-26 22:30 +0100 |
| Articles | 20 on this page of 41 — 6 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: fs: NULL deref in atime_needs_update Mickaël Salaün <mic@digikod.net> - 2016-02-17 02:00 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-19 20:40 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-20 04:30 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-20 05:00 +0100
Re: fs: NULL deref in atime_needs_update Mickaël Salaün <mic@digikod.net> - 2016-02-20 17:00 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-20 18:20 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-20 22:00 +0100
Re: fs: NULL deref in atime_needs_update Mickaël Salaün <mic@digikod.net> - 2016-02-20 22:10 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-22 12:30 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-22 18:30 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-23 16:40 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-23 19:20 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-20 11:40 +0100
Re: fs: NULL deref in atime_needs_update Ian Kent <raven@themaw.net> - 2016-02-24 04:20 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-24 05:50 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-24 11:10 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-24 11:20 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-24 14:40 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-24 16:20 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-25 09:30 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-25 17:40 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-26 22:30 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-26 23:10 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-27 23:30 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-28 17:10 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-28 18:10 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-28 21:10 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-29 10:40 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-29 13:40 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 17:20 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 14:10 +0100
Re: fs: NULL deref in atime_needs_update David Howells <dhowells@redhat.com> - 2016-02-29 14:50 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-29 17:00 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 17:30 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-29 19:30 +0100
Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-03-01 10:00 +0100
Re: fs: NULL deref in atime_needs_update Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-29 17:50 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 18:00 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 18:30 +0100
Re: fs: NULL deref in atime_needs_update Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-29 18:30 +0100
Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-26 22:30 +0100
Page 1 of 3 [1] 2 3 Next page →
| From | Mickaël Salaün <mic@digikod.net> |
|---|---|
| Date | 2016-02-17 02:00 +0100 |
| Subject | Re: fs: NULL deref in atime_needs_update |
| Message-ID | <r2YDg-73D-7@gated-at.bofh.it> |
[Multipart message — attachments visible in raw view] — view raw
Hi,
Actually I found the same bug (without fuzzing) and I can reproduce it in a deterministic way (e.g. by creating a LSM that return 1 for the security_file_open hook). At least, from v4.2.8 I can easily trigger traces like this :
BUG: unable to handle kernel NULL pointer dereference at 0000000000000050
IP: [<ffffffff81170871>] atime_needs_update+0x11/0xc0
PGD 127b17067 PUD 12ab2e067 PMD 0
Oops: 0000 [#45] SMP
[...]
RIP: 0010:[<ffffffff81170871>] [<ffffffff81170871>] atime_needs_update+0x11/0xc0
RSP: 0018:ffff880127853c18 EFLAGS: 00010246
RAX: ffff88012ad0c080 RBX: ffff88012ad0c1d8 RCX: ffff88012ad0c080
RDX: 0000000000000000 RSI: ffff88012ad0c1d8 RDI: ffff880127853d98
RBP: ffff880127853c28 R08: ffff8800cc0a2540 R09: ffff8800cfbfc320
R10: ffff8800cc0a2540 R11: 0000000000000001 R12: ffff8800cb5d6300
R13: 0000000000000000 R14: ffff88012ad0c080 R15: ffff880127853e7c
FS: 00007f1054aae700(0000) GS:ffff88012fc40000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000050 CR3: 0000000127977000 CR4: 00000000000406e0
Stack:
ffff88012ad0c1d8 ffff8800cb5d6300 ffff880127853c60 ffffffff8117094e
ffff8800c9ade3c0 0000000000000000 00000000a670294f ffff880127853d70
ffff880127853d98 ffff880127853c98 ffffffff8116071c ffff8800cb4ada80
Call Trace:
[<ffffffff8117094e>] ? touch_atime+0x2e/0xd0
[<ffffffff8116071c>] ? trailing_symlink+0xec/0x280
[<ffffffff81163a78>] ? path_openat+0x468/0x1240
[<ffffffff8111856d>] ? pagevec_lru_move_fn+0xed/0x110
[<ffffffff81117ff0>] ? __activate_page+0x130/0x130
[<ffffffff8116593c>] ? do_filp_open+0x8c/0x100
[<ffffffff81164dec>] ? filename_lookup+0xec/0x180
[<ffffffff8115bc24>] ? do_open_execat+0x74/0x170
[<ffffffff8115d437>] ? do_execveat_common.isra.42+0x1a7/0x6a0
[<ffffffff8115db90>] ? SyS_execve+0x30/0x40
[<ffffffff8156ad65>] ? stub_execve+0x5/0x5
[<ffffffff8156aadb>] ? entry_SYSCALL_64_fastpath+0x16/0x6a
Code: 89 c7 e8 63 eb ff ff 48 89 d8 5b c3 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 41 54 53 f6 46 0c 02 75 72 48 8b 56 28 <48> 8b 42 50 a9 01 04 00 00 75 63 f6 c4 08 75 65 4c 8b 27 41 8b
RIP [<ffffffff81170871>] atime_needs_update+0x11/0xc0
RSP <ffff880127853c18>
CR2: 0000000000000050
---[ end trace 97dc4f4bb0214bd8 ]---
Regards,
Mickaël
On 05/02/2016 22:11, Dmitry Vyukov wrote:
> Hello,
>
> I've hit the following GPF while running syzkaller fuzzer:
>
> general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
> Modules linked in:
> CPU: 1 PID: 5178 Comm: syz-executor Not tainted 4.5.0-rc2+ #65
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: ffff880064768000 ti: ffff8800622c0000 task.ti: ffff8800622c0000
> RIP: 0010:[<ffffffff8181aa5d>] [<ffffffff8181aa5d>]
> atime_needs_update+0x2d/0x460
> RSP: 0018:ffff8800622c7a30 EFLAGS: 00010203
> RAX: dffffc0000000000 RBX: 0000000000000000 RCX: dffffc0000000000
> RDX: 0000000000000001 RSI: 0000000000000000 RDI: 000000000000000c
> RBP: ffff8800622c7a58 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000001 R12: ffff8800622c7c08
> R13: ffff8800622c7c08 R14: ffff8800301ca322 R15: ffff8800622c7bb0
> FS: 00007fd1c9f8b700(0000) GS:ffff88003ed00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 0000000020f31000 CR3: 0000000062274000 CR4: 00000000000006e0
> Stack:
> ffff8800622c7bf4 0000000000000000 ffff8800622c7c08 ffff8800301ca322
> ffff8800622c7bb0 ffff8800622c7b38 ffffffff817ecd91 ffff880030bf5200
> ffff8800622c7bb8 1ffff1000c458f56 ffff8800622c7c00 ffff8800622c7be0
> Call Trace:
> [< inline >] get_link fs/namei.c:1006
> [<ffffffff817ecd91>] link_path_walk+0xaf1/0x1030 fs/namei.c:1968
> [<ffffffff817ed311>] path_parentat+0x41/0x150 fs/namei.c:2176
> [<ffffffff817f4c5c>] filename_parentat+0x17c/0x3c0 fs/namei.c:2198
> [< inline >] user_path_parent fs/namei.c:2412
> [< inline >] SYSC_renameat2 fs/namei.c:4411
> [< inline >] SyS_renameat2 fs/namei.c:4375
> [< inline >] SYSC_renameat fs/namei.c:4521
> [<ffffffff817f9a72>] SyS_renameat+0x192/0x820 fs/namei.c:4518
> [<ffffffff8669e0b6>] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
> Code: 89 e5 41 57 41 56 41 55 41 54 49 89 fc 53 48 89 f3 e8 08 25 d5
> ff 48 8d 7b 0c 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f>
> b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85
> RIP [<ffffffff8181aa5d>] atime_needs_update+0x2d/0x460 fs/inode.c:1611
> RSP <ffff8800622c7a30>
> ---[ end trace 1a4c9bda4680ce46 ]---
>
> On commit df48ab3c2f5ffca88b7803ffbadd074bd5a0a2ef.
>
> Objdump shows that inode is NULL in atime_needs_update.
>
> Unfortunately reproduction of this crash is very hard. The program
> executes something along the lines of:
>
> mmap(0x20000000, 15945728, PROT_READ|PROT_WRITE,
> MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20000000
> mkdir("./bus", 0662515705056234013740) = 0
> openat(AT_FDCWD, "./bus", O_RDONLY|O_EXCL) = 3
> symlinkat("../bus", 3, "./bus") = 0
> renameat(3, "./bus", 3, "./bus/file0") = 0
> mmap(0x20f35000, 4096, PROT_READ|PROT_WRITE,
> MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20f35000
> mount("./bus", "./bus", 0x20f2aee4,
> MS_RDONLY|MS_NODEV|MS_RELATIME|MS_NODIRATIME|MS_BIND|MS_MOVE|MS_REC|MS_UNBINDABLE|MS_SLAVE|MS_SHARED|0xc000380,
> 0x20093f5f) = 0
> open("./bus/file0", O_RDWR|O_EXCL) = -1 EISDIR (Is a directory)
> exit_group(0) = ?
>
> But in multiple threads so that some calls can be doubled and/or
> overlapped. And all this happens on a tmpfs mount.
>
> But I was able to reproduce it 8 or so times, so I am sure that it is real.
>
> For future reference, I was running these programs:
> https://gist.githubusercontent.com/dvyukov/124c457d308fa724d88a/raw/fec2d86e125a7fd2fa2916791d65d7daead7cbbb/gistfile1.txt
> Following these instructions:
> https://github.com/google/syzkaller/wiki/How-to-execute-syzkaller-programs
>
[toc] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2016-02-19 20:40 +0100 |
| Message-ID | <r3Z4e-1MW-15@gated-at.bofh.it> |
| In reply to | #1335969 |
On Wed, Feb 17, 2016 at 12:40 AM, Mickaël Salaün <mic@digikod.net> wrote:
> Hi,
>
> Actually I found the same bug (without fuzzing) and I can reproduce it in a deterministic way (e.g. by creating a LSM that return 1 for the security_file_open hook). At least, from v4.2.8 I can easily trigger traces like this :
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000050
> IP: [<ffffffff81170871>] atime_needs_update+0x11/0xc0
> PGD 127b17067 PUD 12ab2e067 PMD 0
> Oops: 0000 [#45] SMP
> [...]
> RIP: 0010:[<ffffffff81170871>] [<ffffffff81170871>] atime_needs_update+0x11/0xc0
> RSP: 0018:ffff880127853c18 EFLAGS: 00010246
> RAX: ffff88012ad0c080 RBX: ffff88012ad0c1d8 RCX: ffff88012ad0c080
> RDX: 0000000000000000 RSI: ffff88012ad0c1d8 RDI: ffff880127853d98
> RBP: ffff880127853c28 R08: ffff8800cc0a2540 R09: ffff8800cfbfc320
> R10: ffff8800cc0a2540 R11: 0000000000000001 R12: ffff8800cb5d6300
> R13: 0000000000000000 R14: ffff88012ad0c080 R15: ffff880127853e7c
> FS: 00007f1054aae700(0000) GS:ffff88012fc40000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000050 CR3: 0000000127977000 CR4: 00000000000406e0
> Stack:
> ffff88012ad0c1d8 ffff8800cb5d6300 ffff880127853c60 ffffffff8117094e
> ffff8800c9ade3c0 0000000000000000 00000000a670294f ffff880127853d70
> ffff880127853d98 ffff880127853c98 ffffffff8116071c ffff8800cb4ada80
> Call Trace:
> [<ffffffff8117094e>] ? touch_atime+0x2e/0xd0
> [<ffffffff8116071c>] ? trailing_symlink+0xec/0x280
> [<ffffffff81163a78>] ? path_openat+0x468/0x1240
> [<ffffffff8111856d>] ? pagevec_lru_move_fn+0xed/0x110
> [<ffffffff81117ff0>] ? __activate_page+0x130/0x130
> [<ffffffff8116593c>] ? do_filp_open+0x8c/0x100
> [<ffffffff81164dec>] ? filename_lookup+0xec/0x180
> [<ffffffff8115bc24>] ? do_open_execat+0x74/0x170
> [<ffffffff8115d437>] ? do_execveat_common.isra.42+0x1a7/0x6a0
> [<ffffffff8115db90>] ? SyS_execve+0x30/0x40
> [<ffffffff8156ad65>] ? stub_execve+0x5/0x5
> [<ffffffff8156aadb>] ? entry_SYSCALL_64_fastpath+0x16/0x6a
> Code: 89 c7 e8 63 eb ff ff 48 89 d8 5b c3 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 41 54 53 f6 46 0c 02 75 72 48 8b 56 28 <48> 8b 42 50 a9 01 04 00 00 75 63 f6 c4 08 75 65 4c 8b 27 41 8b
> RIP [<ffffffff81170871>] atime_needs_update+0x11/0xc0
> RSP <ffff880127853c18>
> CR2: 0000000000000050
> ---[ end trace 97dc4f4bb0214bd8 ]---
>
>
> Regards,
> Mickaël
>
>
> On 05/02/2016 22:11, Dmitry Vyukov wrote:
>> Hello,
>>
>> I've hit the following GPF while running syzkaller fuzzer:
>>
>> general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
>> Modules linked in:
>> CPU: 1 PID: 5178 Comm: syz-executor Not tainted 4.5.0-rc2+ #65
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> task: ffff880064768000 ti: ffff8800622c0000 task.ti: ffff8800622c0000
>> RIP: 0010:[<ffffffff8181aa5d>] [<ffffffff8181aa5d>]
>> atime_needs_update+0x2d/0x460
>> RSP: 0018:ffff8800622c7a30 EFLAGS: 00010203
>> RAX: dffffc0000000000 RBX: 0000000000000000 RCX: dffffc0000000000
>> RDX: 0000000000000001 RSI: 0000000000000000 RDI: 000000000000000c
>> RBP: ffff8800622c7a58 R08: 0000000000000001 R09: 0000000000000000
>> R10: 0000000000000000 R11: 0000000000000001 R12: ffff8800622c7c08
>> R13: ffff8800622c7c08 R14: ffff8800301ca322 R15: ffff8800622c7bb0
>> FS: 00007fd1c9f8b700(0000) GS:ffff88003ed00000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
>> CR2: 0000000020f31000 CR3: 0000000062274000 CR4: 00000000000006e0
>> Stack:
>> ffff8800622c7bf4 0000000000000000 ffff8800622c7c08 ffff8800301ca322
>> ffff8800622c7bb0 ffff8800622c7b38 ffffffff817ecd91 ffff880030bf5200
>> ffff8800622c7bb8 1ffff1000c458f56 ffff8800622c7c00 ffff8800622c7be0
>> Call Trace:
>> [< inline >] get_link fs/namei.c:1006
>> [<ffffffff817ecd91>] link_path_walk+0xaf1/0x1030 fs/namei.c:1968
>> [<ffffffff817ed311>] path_parentat+0x41/0x150 fs/namei.c:2176
>> [<ffffffff817f4c5c>] filename_parentat+0x17c/0x3c0 fs/namei.c:2198
>> [< inline >] user_path_parent fs/namei.c:2412
>> [< inline >] SYSC_renameat2 fs/namei.c:4411
>> [< inline >] SyS_renameat2 fs/namei.c:4375
>> [< inline >] SYSC_renameat fs/namei.c:4521
>> [<ffffffff817f9a72>] SyS_renameat+0x192/0x820 fs/namei.c:4518
>> [<ffffffff8669e0b6>] entry_SYSCALL_64_fastpath+0x16/0x7a
>> arch/x86/entry/entry_64.S:185
>> Code: 89 e5 41 57 41 56 41 55 41 54 49 89 fc 53 48 89 f3 e8 08 25 d5
>> ff 48 8d 7b 0c 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f>
>> b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85
>> RIP [<ffffffff8181aa5d>] atime_needs_update+0x2d/0x460 fs/inode.c:1611
>> RSP <ffff8800622c7a30>
>> ---[ end trace 1a4c9bda4680ce46 ]---
>>
>> On commit df48ab3c2f5ffca88b7803ffbadd074bd5a0a2ef.
>>
>> Objdump shows that inode is NULL in atime_needs_update.
>>
>> Unfortunately reproduction of this crash is very hard. The program
>> executes something along the lines of:
>>
>> mmap(0x20000000, 15945728, PROT_READ|PROT_WRITE,
>> MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20000000
>> mkdir("./bus", 0662515705056234013740) = 0
>> openat(AT_FDCWD, "./bus", O_RDONLY|O_EXCL) = 3
>> symlinkat("../bus", 3, "./bus") = 0
>> renameat(3, "./bus", 3, "./bus/file0") = 0
>> mmap(0x20f35000, 4096, PROT_READ|PROT_WRITE,
>> MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20f35000
>> mount("./bus", "./bus", 0x20f2aee4,
>> MS_RDONLY|MS_NODEV|MS_RELATIME|MS_NODIRATIME|MS_BIND|MS_MOVE|MS_REC|MS_UNBINDABLE|MS_SLAVE|MS_SHARED|0xc000380,
>> 0x20093f5f) = 0
>> open("./bus/file0", O_RDWR|O_EXCL) = -1 EISDIR (Is a directory)
>> exit_group(0) = ?
>>
>> But in multiple threads so that some calls can be doubled and/or
>> overlapped. And all this happens on a tmpfs mount.
>>
>> But I was able to reproduce it 8 or so times, so I am sure that it is real.
>>
>> For future reference, I was running these programs:
>> https://gist.githubusercontent.com/dvyukov/124c457d308fa724d88a/raw/fec2d86e125a7fd2fa2916791d65d7daead7cbbb/gistfile1.txt
>> Following these instructions:
>> https://github.com/google/syzkaller/wiki/How-to-execute-syzkaller-programs
I've hit another GPF in atime_needs_update, but this time from SyS_openat:
kasan: GPF could be caused by NULL-ptr deref or user memory
accessgeneral protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
Modules linked in:
CPU: 0 PID: 20147 Comm: syz-executor Not tainted 4.5.0-rc4+ #329
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: ffff88005f154740 ti: ffff88005f048000 task.ti: ffff88005f048000
RIP: 0010:[<ffffffff81818b5d>] [<ffffffff81818b5d>]
atime_needs_update+0x2d/0x460
RSP: 0018:ffff88005f04fa48 EFLAGS: 00010203
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffff88005f04fd88
RDX: 0000000000000001 RSI: 0000000000000000 RDI: 000000000000000c
RBP: ffff88005f04fa70 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88005f04fd98
R13: 0000000000000000 R14: ffff88005f04fd98 R15: ffff88005f04fd78
FS: 00007f612639b700(0000) GS:ffff88003ec00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 000000002003ef84 CR3: 000000006073e000 CR4: 00000000000006f0
Stack:
ffff88005f04fd40 ffff88005f04fe08 0000000000000000 ffff88005f04fd98
ffff88005f04fd78 ffff88005f04fab8 ffffffff817e5572 ffff88005f04fd78
ffff88002bcf02f8 0000000000000001 0000000000000000 ffff88002bcf02f8
Call Trace:
[< inline >] get_link fs/namei.c:1006
[<ffffffff817e5572>] trailing_symlink+0x142/0x760 fs/namei.c:2094
[<ffffffff817ec531>] path_openat+0xbc1/0x5e30 fs/namei.c:3389
[<ffffffff817f4fde>] do_filp_open+0x18e/0x250 fs/namei.c:3421
[<ffffffff817b970c>] do_sys_open+0x1fc/0x420 fs/open.c:1022
[< inline >] SYSC_openat fs/open.c:1049
[<ffffffff817b99a0>] SyS_openat+0x30/0x40 fs/open.c:1043
[<ffffffff86662636>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
Code: 89 e5 41 57 41 56 41 55 41 54 49 89 fc 53 48 89 f3 e8 c8 32 d5
ff 48 8d 7b 0c 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f>
b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85
RIP [<ffffffff81818b5d>] atime_needs_update+0x2d/0x460 fs/inode.c:1611
RSP <ffff88005f04fa48>
---[ end trace 0790795e3dea8fc8 ]---
The program that triggered it was:
mmap(&(0x7f0000000000)=nil, (0x51000), 0x3, 0x32, 0xffffffffffffffff, 0x0)
mkdir(&(0x7f0000002000)="2e2f66696c653000", 0x40)
mount(&(0x7f000001a000)="2e2f66696c653000",
&(0x7f000001a000+0x4cf)="2e2f66696c653000",
&(0x7f000001a000+0xc79)="72616d667300", 0x800,
&(0x7f000003f000-0x7c)="3dd496054f9a5d4176272c354e968b1cf2c6c18792461e7325b5774e9197aad240b4b45ac67faa72d988a86dd8ba348739259df192d01f08ca39a8524fee967d7f39f0ec53ce000ff78ed4b2510c5ae8812ed421db038390d8f3fea242e682c907334bc6fc74f2490e4aac983bdf85c9f5b6f6b288b0074ab201ab")
r1 = open$dir(&(0x7f000004c000+0x7df)="2e2f66696c653000", 0x200000, 0x82)
symlinkat(&(0x7f000002e000)="2e2f66696c65302f66696c653000", r1,
&(0x7f0000033000)="2e2f66696c653000")
openat(r1, &(0x7f000004b000-0x8)="2e2f66696c653000", 0x40000, 0x2)
On commit 1926e54f115725a9248d0c4c65c22acaf94de4c4.
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-02-20 04:30 +0100 |
| Message-ID | <r46p3-7fD-1@gated-at.bofh.it> |
| In reply to | #1338425 |
On Fri, Feb 19, 2016 at 08:32:10PM +0100, Dmitry Vyukov wrote: > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000050 NULL inode->i_sb, by the look of the offset, but I really don't understand where the hell is that code doing (or how is that instruction going to generate dereferencing of 0x50, for that matter). > I've hit another GPF in atime_needs_update, but this time from SyS_openat: > > kasan: GPF could be caused by NULL-ptr deref or user memory > accessgeneral protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN > Modules linked in: > CPU: 0 PID: 20147 Comm: syz-executor Not tainted 4.5.0-rc4+ #329 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > task: ffff88005f154740 ti: ffff88005f048000 task.ti: ffff88005f048000 > RIP: 0010:[<ffffffff81818b5d>] [<ffffffff81818b5d>] > atime_needs_update+0x2d/0x460 > RSP: 0018:ffff88005f04fa48 EFLAGS: 00010203 > RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffff88005f04fd88 > RDX: 0000000000000001 RSI: 0000000000000000 RDI: 000000000000000c > RBP: ffff88005f04fa70 R08: 0000000000000001 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000001 R12: ffff88005f04fd98 > R13: 0000000000000000 R14: ffff88005f04fd98 R15: ffff88005f04fd78 > FS: 00007f612639b700(0000) GS:ffff88003ec00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b > CR2: 000000002003ef84 CR3: 000000006073e000 CR4: 00000000000006f0 > Stack: > ffff88005f04fd40 ffff88005f04fe08 0000000000000000 ffff88005f04fd98 > ffff88005f04fd78 ffff88005f04fab8 ffffffff817e5572 ffff88005f04fd78 > ffff88002bcf02f8 0000000000000001 0000000000000000 ffff88002bcf02f8 > Call Trace: > [< inline >] get_link fs/namei.c:1006 > [<ffffffff817e5572>] trailing_symlink+0x142/0x760 fs/namei.c:2094 > [<ffffffff817ec531>] path_openat+0xbc1/0x5e30 fs/namei.c:3389 > [<ffffffff817f4fde>] do_filp_open+0x18e/0x250 fs/namei.c:3421 > [<ffffffff817b970c>] do_sys_open+0x1fc/0x420 fs/open.c:1022 > [< inline >] SYSC_openat fs/open.c:1049 > [<ffffffff817b99a0>] SyS_openat+0x30/0x40 fs/open.c:1043 > [<ffffffff86662636>] entry_SYSCALL_64_fastpath+0x16/0x7a > arch/x86/entry/entry_64.S:185 > Code: 89 e5 41 57 41 56 41 55 41 54 49 89 fc 53 48 89 f3 e8 c8 32 d5 > ff 48 8d 7b 0c 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> > b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 > RIP [<ffffffff81818b5d>] atime_needs_update+0x2d/0x460 fs/inode.c:1611 > RSP <ffff88005f04fa48> What was the fault address here and what config are you using for those builds?
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-02-20 05:00 +0100 |
| Message-ID | <r46S6-7qj-5@gated-at.bofh.it> |
| In reply to | #1338624 |
On Sat, Feb 20, 2016 at 03:21:27AM +0000, Al Viro wrote:
> On Fri, Feb 19, 2016 at 08:32:10PM +0100, Dmitry Vyukov wrote:
> > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000050
>
> NULL inode->i_sb, by the look of the offset, but I really don't understand
> where the hell is that code doing (or how is that instruction going to
> generate dereferencing of 0x50, for that matter).
BTW, Mickaël's trace *does* make sense and it's definitely NULL inode->i_sb
(inode itself - in %rsi, inode->i_sb - in %rdx, offset of s_flags is 0x50,
the line in question is
if ((inode->i_sb->s_flags & MS_NODIRATIME) && S_ISDIR(inode->i_mode))
What I don't understand is what could possibly have NULL ->i_sb in *any*
instance of struct inode.
[toc] | [prev] | [next] | [standalone]
| From | Mickaël Salaün <mic@digikod.net> |
|---|---|
| Date | 2016-02-20 17:00 +0100 |
| Message-ID | <r4i6S-7Jo-19@gated-at.bofh.it> |
| In reply to | #1338628 |
[Multipart message — attachments visible in raw view] — view raw
On 20/02/2016 04:54, Al Viro wrote: > On Sat, Feb 20, 2016 at 03:21:27AM +0000, Al Viro wrote: >> On Fri, Feb 19, 2016 at 08:32:10PM +0100, Dmitry Vyukov wrote: >>>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000050 >> >> NULL inode->i_sb, by the look of the offset, but I really don't understand >> where the hell is that code doing (or how is that instruction going to >> generate dereferencing of 0x50, for that matter). > > BTW, Mickaël's trace *does* make sense and it's definitely NULL inode->i_sb > (inode itself - in %rsi, inode->i_sb - in %rdx, offset of s_flags is 0x50, > the line in question is > if ((inode->i_sb->s_flags & MS_NODIRATIME) && S_ISDIR(inode->i_mode)) > > What I don't understand is what could possibly have NULL ->i_sb in *any* > instance of struct inode. > I can also trigger bugs with a bad inode pointer dereference in atime_needs_update: if (inode->i_flags & S_NOATIME) I think the bug may be somewhere in the nd->depth handling (when its value is 0) in fs/namei.c:get_link(): struct saved *last = nd->stack + nd->depth - 1 Moreover, is it intentional that touch_atime() is called by get_link() even if the access (previously checked with security_file_open(), e.g. by do_last) is denied?
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-02-20 18:20 +0100 |
| Message-ID | <r4jmi-xc-15@gated-at.bofh.it> |
| In reply to | #1338743 |
On Sat, Feb 20, 2016 at 02:25:40PM +0100, Mickaël Salaün wrote:
> I think the bug may be somewhere in the nd->depth handling (when its value is 0) in fs/namei.c:get_link(): struct saved *last = nd->stack + nd->depth - 1
Getting there with nd->depth == 0 would certainly be a bug - it would mean
that we got there without should_follow_link() having returned 1.
In case of open() it would be "do_last() has returned positive without
should_follow_link() having returned 1".
<looks>
OK, there are several places where we rely on not getting bogus return values
- inode_permission() should not return positives, neither should vfs_open(),
security_path_truncate() and notify_change().
Other similar "handle the last component" functions are guaranteed to
never return positives other than directly from should_follow_link(), so
they are OK.
IIRC, you used LSM to inject a positive value to inode_permission(), right?
Another way to trigger that would've been ->open() returning positive -
a bug on *anything* since ->open() had been introduced in 0.95. Amount of
harm would vary - e.g. 0.95 would simply have that positive number returned
to userland, looking like successful open(2). With no new descriptor, of
course...
Short-term we probably want just
if (unlikely(error > 0)) {
WARN_ON(1);
error = -EINVAL;
}
added right after out: in do_last(), try to trigger Dmitry's reproducers
on it and then work back to the source of that thing *if* that's what's
happening in his case. Yours almost certainly is just that.
Longer-term... I'm not sure. Having a method that is supposed to return 0
or -E<something> actually return positive is going to be a bad thing, no
matter what, but "that bogus value gets passed to userland" is a lot
more tolerable than "kernel memory corruption". do_last() calling conventions
make it vulnerable to the latter, and as far as nd->stack underruns that's
it, but I'm not sure we don't have other places where such bug in driver,
etc. would translate into mess ;-/
OK, in any case, let's start with checking if Dmitry is seeing that and not
something else. I still don't understand his stack traces - the fault
address quoted in his first posting doesn't match the register values in
the same trace, and there's also a possibility that it's an RCU-related
crap. This should give a warning and prevent an oops if we are hitting
a stack underrun on bogus positive from do_last(). Dmitry, could you try
to build with delta below and run your reproducer(s)?
diff --git a/fs/namei.c b/fs/namei.c
index f624d13..e30deef 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3273,6 +3273,10 @@ opened:
goto exit_fput;
}
out:
+ if (unlikely(error > 0)) {
+ WARN_ON(1);
+ error = -EINVAL;
+ }
if (got_write)
mnt_drop_write(nd->path.mnt);
path_put(&save_parent);
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-02-20 22:00 +0100 |
| Message-ID | <r4mNb-2Zt-1@gated-at.bofh.it> |
| In reply to | #1338750 |
On Sat, Feb 20, 2016 at 09:26:28PM +0100, Mickaël Salaün wrote:
> > + if (unlikely(error > 0)) {
> > + WARN_ON(1);
> > + error = -EINVAL;
> > + }
> > if (got_write)
> > mnt_drop_write(nd->path.mnt);
> > path_put(&save_parent);
> >
>
> I think your warning patch should be upstreamed to detect such cases :)
I'm not sure whether it's better done there or one step closer to the
source - e.g. telling whether it was bogus ->open() or a bogus LSM hook
(and _which_ bogus LSM hook had it been) would be hard with that location
of test.
It still leaves the question of what's going on in Dmitry's tests - it might
be one of those source or it might be something else entirely; this location
of test would at least tell whether a stack underrun is involved or not...
[toc] | [prev] | [next] | [standalone]
| From | Mickaël Salaün <mic@digikod.net> |
|---|---|
| Date | 2016-02-20 22:10 +0100 |
| Message-ID | <r4mNb-2Zt-3@gated-at.bofh.it> |
| In reply to | #1338750 |
[Multipart message — attachments visible in raw view] — view raw
On 20/02/2016 18:10, Al Viro wrote:
> On Sat, Feb 20, 2016 at 02:25:40PM +0100, Mickaël Salaün wrote:
>
>> I think the bug may be somewhere in the nd->depth handling (when its value is 0) in fs/namei.c:get_link(): struct saved *last = nd->stack + nd->depth - 1
>
> Getting there with nd->depth == 0 would certainly be a bug - it would mean
> that we got there without should_follow_link() having returned 1.
>
> In case of open() it would be "do_last() has returned positive without
> should_follow_link() having returned 1".
OK, the do_last() return value was the origin of my bug in fs/namei.c:path_openat(): while (!(error = link_path_walk(s, nd)) && (error = do_last(nd, file, op, &opened)) > 0)
>
> <looks>
>
> OK, there are several places where we rely on not getting bogus return values
> - inode_permission() should not return positives, neither should vfs_open(),
> security_path_truncate() and notify_change().
>
> Other similar "handle the last component" functions are guaranteed to
> never return positives other than directly from should_follow_link(), so
> they are OK.
>
> IIRC, you used LSM to inject a positive value to inode_permission(), right?
Yes, my test hook was wrong because it returned 1 (instead of -EPERM or -EACCES) which is an invalid return value.
> diff --git a/fs/namei.c b/fs/namei.c
> index f624d13..e30deef 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3273,6 +3273,10 @@ opened:
> goto exit_fput;
> }
> out:
> + if (unlikely(error > 0)) {
> + WARN_ON(1);
> + error = -EINVAL;
> + }
> if (got_write)
> mnt_drop_write(nd->path.mnt);
> path_put(&save_parent);
>
I think your warning patch should be upstreamed to detect such cases :)
Thanks,
Mickaël
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2016-02-22 12:30 +0100 |
| Message-ID | <r4WQG-51k-7@gated-at.bofh.it> |
| In reply to | #1338750 |
On Sat, Feb 20, 2016 at 6:10 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sat, Feb 20, 2016 at 02:25:40PM +0100, Mickaël Salaün wrote:
>
>> I think the bug may be somewhere in the nd->depth handling (when its value is 0) in fs/namei.c:get_link(): struct saved *last = nd->stack + nd->depth - 1
>
> Getting there with nd->depth == 0 would certainly be a bug - it would mean
> that we got there without should_follow_link() having returned 1.
>
> In case of open() it would be "do_last() has returned positive without
> should_follow_link() having returned 1".
>
> <looks>
>
> OK, there are several places where we rely on not getting bogus return values
> - inode_permission() should not return positives, neither should vfs_open(),
> security_path_truncate() and notify_change().
>
> Other similar "handle the last component" functions are guaranteed to
> never return positives other than directly from should_follow_link(), so
> they are OK.
>
> IIRC, you used LSM to inject a positive value to inode_permission(), right?
>
> Another way to trigger that would've been ->open() returning positive -
> a bug on *anything* since ->open() had been introduced in 0.95. Amount of
> harm would vary - e.g. 0.95 would simply have that positive number returned
> to userland, looking like successful open(2). With no new descriptor, of
> course...
>
> Short-term we probably want just
> if (unlikely(error > 0)) {
> WARN_ON(1);
> error = -EINVAL;
> }
> added right after out: in do_last(), try to trigger Dmitry's reproducers
> on it and then work back to the source of that thing *if* that's what's
> happening in his case. Yours almost certainly is just that.
>
> Longer-term... I'm not sure. Having a method that is supposed to return 0
> or -E<something> actually return positive is going to be a bad thing, no
> matter what, but "that bogus value gets passed to userland" is a lot
> more tolerable than "kernel memory corruption". do_last() calling conventions
> make it vulnerable to the latter, and as far as nd->stack underruns that's
> it, but I'm not sure we don't have other places where such bug in driver,
> etc. would translate into mess ;-/
>
> OK, in any case, let's start with checking if Dmitry is seeing that and not
> something else. I still don't understand his stack traces - the fault
> address quoted in his first posting doesn't match the register values in
> the same trace, and there's also a possibility that it's an RCU-related
> crap. This should give a warning and prevent an oops if we are hitting
> a stack underrun on bogus positive from do_last(). Dmitry, could you try
> to build with delta below and run your reproducer(s)?
>
> diff --git a/fs/namei.c b/fs/namei.c
> index f624d13..e30deef 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3273,6 +3273,10 @@ opened:
> goto exit_fput;
> }
> out:
> + if (unlikely(error > 0)) {
> + WARN_ON(1);
> + error = -EINVAL;
> + }
> if (got_write)
> mnt_drop_write(nd->path.mnt);
> path_put(&save_parent);
I've reproduced the second report (the one originating in openat) with
this patch and the WARNING did _not_ fire:
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
Modules linked in:
CPU: 2 PID: 17525 Comm: syz-executor Not tainted 4.5.0-rc5+ #331
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: ffff88002c6ddf00 ti: ffff88002c740000 task.ti: ffff88002c740000
RIP: 0010:[<ffffffff81821ded>] [<ffffffff81821ded>]
atime_needs_update+0x2d/0x460
RSP: 0018:ffff88002c747a48 EFLAGS: 00010203
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffff88002c747d88
RDX: 0000000000000001 RSI: 0000000000000000 RDI: 000000000000000c
RBP: ffff88002c747a70 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88002c747d98
R13: 0000000000000000 R14: ffff88002c747d98 R15: ffff88002c747d78
FS: 00007f24da3d9700(0000) GS:ffff88006d600000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 000000002003f000 CR3: 000000002e2d0000 CR4: 00000000000006e0
Stack:
ffff88002c747d40 ffff88002c747e08 0000000000000000 ffff88002c747d98
ffff88002c747d78 ffff88002c747ab8 ffffffff817eeda2 ffff88002c747d78
ffff880030fa88e8 ffff88002c747c98 0000000000000000 ffff88002c747d40
Call Trace:
[< inline >] get_link fs/namei.c:1006
[<ffffffff817eeda2>] trailing_symlink+0x142/0x760 fs/namei.c:2094
[<ffffffff817f5cec>] path_openat+0xb4c/0x5760 fs/namei.c:3393
[<ffffffff817fe13e>] do_filp_open+0x18e/0x250 fs/namei.c:3425
[<ffffffff817c2dbc>] do_sys_open+0x1fc/0x420 fs/open.c:1022
[< inline >] SYSC_openat fs/open.c:1049
[<ffffffff817c3050>] SyS_openat+0x30/0x40 fs/open.c:1043
[<ffffffff8669f6b6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
Code: 89 e5 41 57 41 56 41 55 41 54 49 89 fc 53 48 89 f3 e8 98 17 d5
ff 48 8d 7b 0c 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f>
b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85
RIP [<ffffffff81821ded>] atime_needs_update+0x2d/0x460 fs/inode.c:1617
RSP <ffff88002c747a48>
---[ end trace 872348222bfe81b0 ]---
This is _not_ on a tmpfs mount.
Regarding the registers, here is disassembly. The crash happens on a
KASAN check of the
ffffffff81821dc0 <atime_needs_update>:
ffffffff81821dc0: 55 push %rbp
ffffffff81821dc1: 48 89 e5 mov %rsp,%rbp
ffffffff81821dc4: 41 57 push %r15
ffffffff81821dc6: 41 56 push %r14
ffffffff81821dc8: 41 55 push %r13
ffffffff81821dca: 41 54 push %r12
ffffffff81821dcc: 49 89 fc mov %rdi,%r12
ffffffff81821dcf: 53 push %rbx
ffffffff81821dd0: 48 89 f3 mov %rsi,%rbx
ffffffff81821dd3: e8 98 17 d5 ff callq
ffffffff81573570 <__sanitizer_cov_trace_pc>
ffffffff81821dd8: 48 8d 7b 0c lea 0xc(%rbx),%rdi
ffffffff81821ddc: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax
ffffffff81821de3: fc ff df
ffffffff81821de6: 48 89 fa mov %rdi,%rdx
ffffffff81821de9: 48 c1 ea 03 shr $0x3,%rdx
ffffffff81821ded: 0f b6 14 02 movzbl (%rdx,%rax,1),%edx
ffffffff81821df1: 48 89 f8 mov %rdi,%rax
ffffffff81821df4: 83 e0 07 and $0x7,%eax
ffffffff81821df7: 83 c0 03 add $0x3,%eax
ffffffff81821dfa: 38 d0 cmp %dl,%al
ffffffff81821dfc: 7c 08 jl
ffffffff81821e06 <atime_needs_update+0x46>
ffffffff81821dfe: 84 d2 test %dl,%dl
ffffffff81821e00: 0f 85 03 03 00 00 jne
ffffffff81822109 <atime_needs_update+0x349>
ffffffff81821e06: f6 43 0c 02 testb $0x2,0xc(%rbx)
ffffffff81821e0a: 0f 85 1a 02 00 00 jne
ffffffff8182202a <atime_needs_update+0x26a>
ffffffff81821e10: e8 5b 17 d5 ff callq
ffffffff81573570 <__sanitizer_cov_trace_pc>
ffffffff81821e15: 48 8d 7b 28 lea 0x28(%rbx),%rdi
ffffffff81821e19: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax
ffffffff81821e20: fc ff df
ffffffff81821e23: 48 89 fa mov %rdi,%rdx
ffffffff81821e26: 48 c1 ea 03 shr $0x3,%rdx
ffffffff81821e2a: 80 3c 02 00 cmpb $0x0,(%rdx,%rax,1)
ffffffff81821e2e: 0f 85 c4 03 00 00 jne
ffffffff818221f8 <atime_needs_update+0x438>
It means that inode is NULL here:
bool atime_needs_update(const struct path *path, struct inode *inode)
{
struct vfsmount *mnt = path->mnt;
struct timespec now;
if (inode->i_flags & S_NOATIME)
return false;
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-02-22 18:30 +0100 |
| Message-ID | <r52t3-LR-9@gated-at.bofh.it> |
| In reply to | #1339391 |
On Mon, Feb 22, 2016 at 12:20:30PM +0100, Dmitry Vyukov wrote: > I've reproduced the second report (the one originating in openat) with > this patch and the WARNING did _not_ fire: Lovely... Could you dump your inode.o on anonftp somewhere? Or post the disassembled atime_needs_update, for that matter - shouldn't be all that long... .config (and gcc version) you are using would also be nice. On the face of it, NULL inode is a plausible source of that one, but it's _very_ odd. It would have to be NULL ->link_inode, and since the warning hadn't triggered, there was a successful should_follow_link(), with NULL inode argument. Could you slap WARN_ON(!inode) in pick_link()? Or even WARN_ON(IS_ERR_OR_NULL(inode)), for that matter...
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2016-02-23 16:40 +0100 |
| Message-ID | <r5nea-7n1-17@gated-at.bofh.it> |
| In reply to | #1339746 |
On Mon, Feb 22, 2016 at 6:23 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Feb 22, 2016 at 12:20:30PM +0100, Dmitry Vyukov wrote:
>
>> I've reproduced the second report (the one originating in openat) with
>> this patch and the WARNING did _not_ fire:
>
> Lovely... Could you dump your inode.o on anonftp somewhere? Or post
> the disassembled atime_needs_update, for that matter - shouldn't be
> all that long... .config (and gcc version) you are using would also be nice.
>
> On the face of it, NULL inode is a plausible source of that one, but
> it's _very_ odd. It would have to be NULL ->link_inode, and since the
> warning hadn't triggered, there was a successful should_follow_link(),
> with NULL inode argument.
>
> Could you slap WARN_ON(!inode) in pick_link()? Or even
> WARN_ON(IS_ERR_OR_NULL(inode)), for that matter...
I was able to reproduce the crash on vanilla kernel (no KASAN, no
KCOV) with the new WARNING, this one does fire.
So I am on commit 4de8ebeff8ddefaceeb7fc6a9b1a514fc9624509 (Feb 22), with:
$ git diff
diff --git a/fs/namei.c b/fs/namei.c
index f624d13..9675e9e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1669,6 +1669,7 @@ static int pick_link(struct nameidata *nd,
struct path *link,
{
int error;
struct saved *last;
+ WARN_ON(IS_ERR_OR_NULL(inode));
if (unlikely(nd->total_link_count++ >= MAXSYMLINKS)) {
path_to_nameidata(link, nd);
return -ELOOP;
@@ -3273,6 +3274,10 @@ opened:
goto exit_fput;
}
out:
+ if (unlikely(error > 0)) {
+ WARN_ON(1);
+ error = -EINVAL;
+ }
if (got_write)
mnt_drop_write(nd->path.mnt);
path_put(&save_parent);
The crash:
[ 8095.048336] ------------[ cut here ]------------
[ 8095.048864] WARNING: CPU: 3 PID: 5532 at fs/namei.c:1672
should_follow_link.part.25+0x55/0x21a()
[ 8095.049830] Modules linked in:
[ 8095.050155] CPU: 3 PID: 5532 Comm: syz-executor Not tainted 4.5.0-rc5+ #69
[ 8095.050760] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Bochs 01/01/2011
[ 8095.051104] 0000000000000000 ffff8800005ffc78 ffffffff8194e5f9
0000000000000000
[ 8095.051649] ffffffff8334c24c ffff8800005ffcb0 ffffffff81172291
ffff8800005ffde0
[ 8095.051649] ffff8800005ffd98 0000000000048000 ffff8800005ffde0
ffff8800005ffefc
[ 8095.051649] Call Trace:
[ 8095.051649] [<ffffffff8194e5f9>] dump_stack+0x99/0xd0
[ 8095.054784] [<ffffffff81172291>] warn_slowpath_common+0x81/0xc0
[ 8095.054784] [<ffffffff81172385>] warn_slowpath_null+0x15/0x20
[ 8095.054784] [<ffffffff8185a55c>] should_follow_link.part.25+0x55/0x21a
[ 8095.054784] [<ffffffff813180c9>] path_openat+0x1229/0x1500
[ 8095.057172] [<ffffffff813193d9>] do_filp_open+0x79/0xd0
[ 8095.057172] [<ffffffff82b170c2>] ? _raw_spin_unlock+0x22/0x30
[ 8095.057172] [<ffffffff81328d98>] ? __alloc_fd+0xf8/0x200
[ 8095.058364] [<ffffffff81306eb0>] do_sys_open+0x110/0x1f0
[ 8095.058364] [<ffffffff81306fbf>] SyS_openat+0xf/0x20
[ 8095.058364] [<ffffffff82b17b76>] entry_SYSCALL_64_fastpath+0x16/0x7a
[ 8095.060124] ---[ end trace 50e32daa426e4c92 ]---
[ 8095.060526] BUG: unable to handle kernel NULL pointer dereference
at 000000000000000c
[ 8095.061111] IP: [<ffffffff813272b9>] atime_needs_update+0x9/0xc0
[ 8095.061549] PGD 6ae6e067 PUD 6ae6a067 PMD 0
[ 8095.061549] Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
[ 8095.061549] Modules linked in:
[ 8095.061549] CPU: 3 PID: 5532 Comm: syz-executor Tainted: G W
4.5.0-rc5+ #69
[ 8095.061549] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Bochs 01/01/2011
[ 8095.061549] task: ffff88002bc00000 ti: ffff8800005fc000 task.ti:
ffff8800005fc000
[ 8095.061549] RIP: 0010:[<ffffffff813272b9>] [<ffffffff813272b9>]
atime_needs_update+0x9/0xc0
[ 8095.061549] RSP: 0018:ffff8800005ffcb0 EFLAGS: 00010282
[ 8095.061549] RAX: 0000000000000030 RBX: ffff8800005ffde0 RCX: 0000000000000000
[ 8095.061549] RDX: ffff8800005ffe38 RSI: 0000000000000000 RDI: ffff8800005ffe38
[ 8095.061549] RBP: ffff8800005ffcc0 R08: 0000000000000002 R09: 0000000000000001
[ 8095.061549] R10: 0000000000000001 R11: 0000000000001828 R12: 0000000000000000
[ 8095.061549] R13: ffff8800005ffe38 R14: ffff880031950be0 R15: ffff8800005ffefc
[ 8095.061549] FS: 00007fc3bf2ee700(0000) GS:ffff88007f900000(0000)
knlGS:0000000000000000
[ 8095.069746] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 8095.069746] CR2: 000000000000000c CR3: 000000006b982000 CR4: 00000000000006e0
[ 8095.071108] Stack:
[ 8095.071538] ffff8800005ffcc0 ffff8800005ffde0 ffff8800005ffd00
ffffffff81314ca2
[ 8095.072227] 0000000200000002 0000000000000005 ffff8800005ffd98
0000000000048000
[ 8095.072227] ffff8800005ffde0 ffff8800005ffefc ffff8800005ffdd0
ffffffff81317173
[ 8095.072227] Call Trace:
[ 8095.072227] [<ffffffff81314ca2>] trailing_symlink+0x62/0x260
[ 8095.072227] [<ffffffff81317173>] path_openat+0x2d3/0x1500
[ 8095.072227] [<ffffffff813193d9>] do_filp_open+0x79/0xd0
[ 8095.072227] [<ffffffff82b170c2>] ? _raw_spin_unlock+0x22/0x30
[ 8095.072227] [<ffffffff81328d98>] ? __alloc_fd+0xf8/0x200
[ 8095.072227] [<ffffffff81306eb0>] do_sys_open+0x110/0x1f0
[ 8095.072227] [<ffffffff81306fbf>] SyS_openat+0xf/0x20
[ 8095.072227] [<ffffffff82b17b76>] entry_SYSCALL_64_fastpath+0x16/0x7a
[ 8095.072227] Code: ff ff ff 48 85 c0 48 89 c3 74 08 48 89 c7 e8 ef
dc ff ff 48 89 d8 5b 5d c3 66 0f 1f 84 00 00 00 00 00 55 48 89 e5 53
48 83 ec 08 <f6> 46 0c 02 48 8b 1f 75 6b 48 8b 7e 28 48 8b 47 50 a9 01
04 00
[ 8095.072227] RIP [<ffffffff813272b9>] atime_needs_update+0x9/0xc0
[ 8095.072227] RSP <ffff8800005ffcb0>
[ 8095.072227] CR2: 000000000000000c
[ 8095.116838] ---[ end trace 50e32daa426e4c93 ]---
[ 8095.116838] BUG: sleeping function called from invalid context at
include/linux/sched.h:2795
[ 8095.116838] in_atomic(): 1, irqs_disabled(): 1, pid: 5532, name: syz-executor
[ 8095.116838] INFO: lockdep is turned off.
[ 8095.116838] irq event stamp: 636
[ 8095.116838] hardirqs last enabled at (635): [<ffffffff811d9926>]
vprintk_emit+0x2d6/0x5f0
[ 8095.116838] hardirqs last disabled at (636): [<ffffffff82b1a089>]
error_entry+0x69/0xc0
[ 8095.116838] softirqs last enabled at (632): [<ffffffff81178172>]
__do_softirq+0x222/0x4a0
[ 8095.116838] softirqs last disabled at (623): [<ffffffff81178767>]
irq_exit+0xa7/0xc0
[ 8095.116838] CPU: 3 PID: 5532 Comm: syz-executor Tainted: G D W
4.5.0-rc5+ #69
[ 8095.116838] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Bochs 01/01/2011
[ 8095.116838] 0000000000000000 ffff8800005ff9a8 ffffffff8194e5f9
ffff88002bc00000
[ 8095.116838] 000000000000159c ffff8800005ff9d0 ffffffff811a0659
ffffffff832a29c3
[ 8095.116838] 0000000000000aeb 0000000000000000 ffff8800005ff9f8
ffffffff811a0764
[ 8095.116838] Call Trace:
[ 8095.116838] [<ffffffff8194e5f9>] dump_stack+0x99/0xd0
[ 8095.116838] [<ffffffff811a0659>] ___might_sleep+0x179/0x240
[ 8095.116838] [<ffffffff811a0764>] __might_sleep+0x44/0x80
[ 8095.116838] [<ffffffff811842cf>] exit_signals+0x1f/0x130
[ 8095.116838] [<ffffffff811756ef>] do_exit+0xbf/0xd10
[ 8095.116838] [<ffffffff811da524>] ? kmsg_dump+0x104/0x180
[ 8095.116838] [<ffffffff8108549f>] oops_end+0x9f/0xe0
[ 8095.116838] [<ffffffff810ce6e8>] no_context+0x108/0x390
[ 8095.116838] [<ffffffff811d7837>] ? print_time.part.13+0x67/0x90
[ 8095.116838] [<ffffffff810cea8d>] __bad_area_nosemaphore+0x11d/0x220
[ 8095.116838] [<ffffffff810ceb9e>] bad_area_nosemaphore+0xe/0x10
[ 8095.116838] [<ffffffff810cf2c4>] __do_page_fault+0x84/0x470
[ 8095.116838] [<ffffffff810cf764>] trace_do_page_fault+0x74/0x2c0
[ 8095.116838] [<ffffffff810c9ba4>] do_async_page_fault+0x14/0x90
[ 8095.116838] [<ffffffff82b19eb8>] async_page_fault+0x28/0x30
[ 8095.116838] [<ffffffff813272b9>] ? atime_needs_update+0x9/0xc0
[ 8095.116838] [<ffffffff81314ca2>] trailing_symlink+0x62/0x260
[ 8095.116838] [<ffffffff81317173>] path_openat+0x2d3/0x1500
[ 8095.116838] [<ffffffff813193d9>] do_filp_open+0x79/0xd0
[ 8095.116838] [<ffffffff82b170c2>] ? _raw_spin_unlock+0x22/0x30
[ 8095.116838] [<ffffffff81328d98>] ? __alloc_fd+0xf8/0x200
[ 8095.116838] [<ffffffff81306eb0>] do_sys_open+0x110/0x1f0
[ 8095.116838] [<ffffffff81306fbf>] SyS_openat+0xf/0x20
[ 8095.116838] [<ffffffff82b17b76>] entry_SYSCALL_64_fastpath+0x16/0x7a
[ 8095.195133] note: syz-executor[5532] exited with preempt_count 1
And here is my inode.o:
https://gist.githubusercontent.com/dvyukov/27ec88c2c1a83c2e0f38/raw/2514d0ddd7720a978e6a2f67c2dcb391046ce0e7/gistfile1.txt
This can be reproduced following the instructions here:
https://github.com/google/syzkaller/wiki/How-to-execute-syzkaller-programs
Using this command line:
# ./syz-execprog -cover=0 -procs=60 -repeat=0 prog
with the following program:
https://gist.githubusercontent.com/dvyukov/fc026f36f9f76d1a440b/raw/0e133afa99eb7de45880523fbd48256cd2ae4a6c/gistfile1.txt
(requires CONFIG_USER_NS=y). The crash triggers after hours of execution.
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-02-23 19:20 +0100 |
| Message-ID | <r5pJ1-G1-33@gated-at.bofh.it> |
| In reply to | #1340799 |
On Tue, Feb 23, 2016 at 04:34:59PM +0100, Dmitry Vyukov wrote:
> The crash:
>
> [ 8095.048336] ------------[ cut here ]------------
> [ 8095.048864] WARNING: CPU: 3 PID: 5532 at fs/namei.c:1672
> should_follow_link.part.25+0x55/0x21a()
NULL or ERR_PTR() passed as inode to should_follow_link().
> [ 8095.060526] BUG: unable to handle kernel NULL pointer dereference
> at 000000000000000c
OK, NULL inode it is. And that was in do_last().
> And here is my inode.o:
> https://gist.githubusercontent.com/dvyukov/27ec88c2c1a83c2e0f38/raw/2514d0ddd7720a978e6a2f67c2dcb391046ce0e7/gistfile1.txt
>
> This can be reproduced following the instructions here:
> https://github.com/google/syzkaller/wiki/How-to-execute-syzkaller-programs
> Using this command line:
> # ./syz-execprog -cover=0 -procs=60 -repeat=0 prog
> with the following program:
> https://gist.githubusercontent.com/dvyukov/fc026f36f9f76d1a440b/raw/0e133afa99eb7de45880523fbd48256cd2ae4a6c/gistfile1.txt
> (requires CONFIG_USER_NS=y). The crash triggers after hours of execution.
Joy... Another interesting question is whether we'd been in RCU mode at
the time of that should_follow_link(). The thing is, we could've come there
either from
if (!(open_flag & O_CREAT)) {
if (nd->last.name[nd->last.len])
nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
/* we _can_ be in RCU mode here */
error = lookup_fast(nd, &path, &inode, &seq);
if (likely(!error))
goto finish_lookup;
or from
BUG_ON(nd->flags & LOOKUP_RCU);
inode = d_backing_inode(path.dentry);
seq = 0; /* out of RCU mode, so the value doesn't matter */
if (unlikely(d_is_negative(path.dentry))) {
path_to_nameidata(&path, nd);
return -ENOENT;
}
finish_lookup:
In the latter case we are holding a reference to path.dentry, so d_is_negative
ought to be reliable and refering to the same backing inode. In the former,
if we leave still in RCU mode, we went through
*inode = d_backing_inode(dentry);
negative = d_is_negative(dentry);
[check that dentry->d_seq is still unchanged]
...
[check that negative is false]
and that guarantees that both inode and negative had been taken while dentry
remained stable, so we couldn't pass through the second check with NULL
inode. And returning 0 in non-RCU mode means that we go through
if (unlikely(d_is_negative(dentry))) {
dput(dentry);
return -ENOENT;
}
path->mnt = mnt;
path->dentry = dentry;
err = follow_managed(path, nd);
if (likely(!err))
*inode = d_backing_inode(path->dentry);
return err;
with dentry pinned, so NULL inode here is also bloody odd - we have positive
dentry that will remain positive through all that and somehow follow_managed()
(in non-RCU mode) gets us a negative one. Now, follow_managed() either
leaves path->dentry unchanged (and keeps it pinned through all of that), or
does
struct vfsmount *mounted = lookup_mnt(path);
if (mounted) {
dput(path->dentry);
if (need_mntput)
mntput(path->mnt);
path->mnt = mounted;
path->dentry = dget(mounted->mnt_root);
(and ->mnt_root should never be negative), or goes into follow_autmount(),
where we either leave the damn thing unchanged or hit
path->dentry = dget(mnt->mnt_root);
... or we have ->d_automount() instance doing something nasty to it. Damn.
OK, a look through the instances shows that only autofs4 one might modify
path->dentry:
struct dentry *new = d_lookup(parent, &dentry->d_name);
if (!new)
return NULL;
ino = autofs4_dentry_ino(new);
ino->last_used = jiffies;
dput(path->dentry);
path->dentry = new;
in autofs4_mountpoint_changed()... I doubt that this is the cause here,
but let's slap WARN_ON(d_is_negative(new)) there.
The thing is, I *do* see one bug around should_follow_link(), but it would
manifest differently. So you must be hitting something else there, to get
that NULL inode... Could you try to reproduce it with the patch below
and see what warnings trigger? I'll try to reproduce it as well, but since
you already have a working setup...
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index c6d7d3d..86f81e3 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -323,6 +323,7 @@ static struct dentry *autofs4_mountpoint_changed(struct path *path)
struct dentry *new = d_lookup(parent, &dentry->d_name);
if (!new)
return NULL;
+ WARN_ON(d_is_negative(new));
ino = autofs4_dentry_ino(new);
ino->last_used = jiffies;
dput(path->dentry);
diff --git a/fs/namei.c b/fs/namei.c
index f624d13..ac00bcb 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1209,6 +1209,7 @@ static int follow_managed(struct path *path, struct nameidata *nd)
/* Handle an automount point */
if (managed & DCACHE_NEED_AUTOMOUNT) {
ret = follow_automount(path, nd, &need_mntput);
+ WARN_ON(d_is_negative(path->dentry));
if (ret < 0)
break;
continue;
@@ -1613,8 +1614,10 @@ unlazy:
path->mnt = mnt;
path->dentry = dentry;
err = follow_managed(path, nd);
- if (likely(!err))
+ if (likely(!err)) {
*inode = d_backing_inode(path->dentry);
+ WARN_ON(!inode);
+ }
return err;
need_lookup:
@@ -1712,6 +1715,17 @@ static inline int should_follow_link(struct nameidata *nd, struct path *link,
return 0;
if (!follow)
return 0;
+ /* make sure that d_is_symlink above matches inode */
+ if (nd->flags & LOOKUP_RCU) {
+ if (read_seqcount_retry(&link->dentry->d_seq, seq)) {
+ WARN_ON(1); // just as way to report hitting
+ // that path; it's OK to get
+ // here, in the final variant
+ // WARN_ON will disappear.
+ return -ECHILD;
+ }
+ }
+ WARN_ON(!inode); // now, _that_ should not happen.
return pick_link(nd, link, inode, seq);
}
@@ -3273,6 +3287,10 @@ opened:
goto exit_fput;
}
out:
+ if (unlikely(error > 0)) {
+ WARN_ON(1);
+ error = -EINVAL;
+ }
if (got_write)
mnt_drop_write(nd->path.mnt);
path_put(&save_parent);
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2016-02-20 11:40 +0100 |
| Message-ID | <r4d7c-3z6-1@gated-at.bofh.it> |
| In reply to | #1338624 |
On Sat, Feb 20, 2016 at 4:21 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Fri, Feb 19, 2016 at 08:32:10PM +0100, Dmitry Vyukov wrote: >> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000050 > > NULL inode->i_sb, by the look of the offset, but I really don't understand > where the hell is that code doing (or how is that instruction going to > generate dereferencing of 0x50, for that matter). > >> I've hit another GPF in atime_needs_update, but this time from SyS_openat: >> >> kasan: GPF could be caused by NULL-ptr deref or user memory >> accessgeneral protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN >> Modules linked in: >> CPU: 0 PID: 20147 Comm: syz-executor Not tainted 4.5.0-rc4+ #329 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 >> task: ffff88005f154740 ti: ffff88005f048000 task.ti: ffff88005f048000 >> RIP: 0010:[<ffffffff81818b5d>] [<ffffffff81818b5d>] >> atime_needs_update+0x2d/0x460 >> RSP: 0018:ffff88005f04fa48 EFLAGS: 00010203 >> RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffff88005f04fd88 >> RDX: 0000000000000001 RSI: 0000000000000000 RDI: 000000000000000c >> RBP: ffff88005f04fa70 R08: 0000000000000001 R09: 0000000000000000 >> R10: 0000000000000000 R11: 0000000000000001 R12: ffff88005f04fd98 >> R13: 0000000000000000 R14: ffff88005f04fd98 R15: ffff88005f04fd78 >> FS: 00007f612639b700(0000) GS:ffff88003ec00000(0000) knlGS:0000000000000000 >> CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b >> CR2: 000000002003ef84 CR3: 000000006073e000 CR4: 00000000000006f0 >> Stack: >> ffff88005f04fd40 ffff88005f04fe08 0000000000000000 ffff88005f04fd98 >> ffff88005f04fd78 ffff88005f04fab8 ffffffff817e5572 ffff88005f04fd78 >> ffff88002bcf02f8 0000000000000001 0000000000000000 ffff88002bcf02f8 >> Call Trace: >> [< inline >] get_link fs/namei.c:1006 >> [<ffffffff817e5572>] trailing_symlink+0x142/0x760 fs/namei.c:2094 >> [<ffffffff817ec531>] path_openat+0xbc1/0x5e30 fs/namei.c:3389 >> [<ffffffff817f4fde>] do_filp_open+0x18e/0x250 fs/namei.c:3421 >> [<ffffffff817b970c>] do_sys_open+0x1fc/0x420 fs/open.c:1022 >> [< inline >] SYSC_openat fs/open.c:1049 >> [<ffffffff817b99a0>] SyS_openat+0x30/0x40 fs/open.c:1043 >> [<ffffffff86662636>] entry_SYSCALL_64_fastpath+0x16/0x7a >> arch/x86/entry/entry_64.S:185 >> Code: 89 e5 41 57 41 56 41 55 41 54 49 89 fc 53 48 89 f3 e8 c8 32 d5 >> ff 48 8d 7b 0c 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> >> b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 >> RIP [<ffffffff81818b5d>] atime_needs_update+0x2d/0x460 fs/inode.c:1611 >> RSP <ffff88005f04fa48> > > What was the fault address here and what config are you using for those > builds? This is with CONFIG_KASAN. GPF happens on shadow check for NULL address. In both cases: RAX: dffffc0000000000 RDX: 0000000000000001 where dffffc0000000000 is shadow base and 0000000000000001 is offset. So the original memory access was to range [8-16) (i.e. NULL with small offset).
[toc] | [prev] | [next] | [standalone]
| From | Ian Kent <raven@themaw.net> |
|---|---|
| Date | 2016-02-24 04:20 +0100 |
| Message-ID | <r5y9A-6QY-1@gated-at.bofh.it> |
| In reply to | #1335969 |
On Wed, 2016-02-17 at 00:40 +0100, Mickaël Salaün wrote:
> Hi,
>
> Actually I found the same bug (without fuzzing) and I can reproduce it
> in a deterministic way (e.g. by creating a LSM that return 1 for the
> security_file_open hook). At least, from v4.2.8 I can easily trigger
> traces like this :
Reading through this thread I wonder if this is a new problem.
Is there a previous kernel it can't be reproduced on?
Perhaps a bisect will shed some light on what's happening.
>
> BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000050
> IP: [<ffffffff81170871>] atime_needs_update+0x11/0xc0
> PGD 127b17067 PUD 12ab2e067 PMD 0
> Oops: 0000 [#45] SMP
> [...]
> RIP: 0010:[<ffffffff81170871>] [<ffffffff81170871>]
> atime_needs_update+0x11/0xc0
> RSP: 0018:ffff880127853c18 EFLAGS: 00010246
> RAX: ffff88012ad0c080 RBX: ffff88012ad0c1d8 RCX: ffff88012ad0c080
> RDX: 0000000000000000 RSI: ffff88012ad0c1d8 RDI: ffff880127853d98
> RBP: ffff880127853c28 R08: ffff8800cc0a2540 R09: ffff8800cfbfc320
> R10: ffff8800cc0a2540 R11: 0000000000000001 R12: ffff8800cb5d6300
> R13: 0000000000000000 R14: ffff88012ad0c080 R15: ffff880127853e7c
> FS: 00007f1054aae700(0000) GS:ffff88012fc40000(0000)
> knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000050 CR3: 0000000127977000 CR4: 00000000000406e0
> Stack:
> ffff88012ad0c1d8 ffff8800cb5d6300 ffff880127853c60 ffffffff8117094e
> ffff8800c9ade3c0 0000000000000000 00000000a670294f ffff880127853d70
> ffff880127853d98 ffff880127853c98 ffffffff8116071c ffff8800cb4ada80
> Call Trace:
> [<ffffffff8117094e>] ? touch_atime+0x2e/0xd0
> [<ffffffff8116071c>] ? trailing_symlink+0xec/0x280
> [<ffffffff81163a78>] ? path_openat+0x468/0x1240
> [<ffffffff8111856d>] ? pagevec_lru_move_fn+0xed/0x110
> [<ffffffff81117ff0>] ? __activate_page+0x130/0x130
> [<ffffffff8116593c>] ? do_filp_open+0x8c/0x100
> [<ffffffff81164dec>] ? filename_lookup+0xec/0x180
> [<ffffffff8115bc24>] ? do_open_execat+0x74/0x170
> [<ffffffff8115d437>] ? do_execveat_common.isra.42+0x1a7/0x6a0
> [<ffffffff8115db90>] ? SyS_execve+0x30/0x40
> [<ffffffff8156ad65>] ? stub_execve+0x5/0x5
> [<ffffffff8156aadb>] ? entry_SYSCALL_64_fastpath+0x16/0x6a
> Code: 89 c7 e8 63 eb ff ff 48 89 d8 5b c3 0f 1f 40 00 66 2e 0f 1f 84
> 00 00 00 00 00 55 48 89 e5 41 54 53 f6 46 0c 02 75 72 48 8b 56 28 <48>
> 8b 42 50 a9 01 04 00 00 75 63 f6 c4 08 75 65 4c 8b 27 41 8b
> RIP [<ffffffff81170871>] atime_needs_update+0x11/0xc0
> RSP <ffff880127853c18>
> CR2: 0000000000000050
> ---[ end trace 97dc4f4bb0214bd8 ]---
>
>
> Regards,
> Mickaël
>
>
> On 05/02/2016 22:11, Dmitry Vyukov wrote:
> > Hello,
> >
> > I've hit the following GPF while running syzkaller fuzzer:
> >
> > general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
> > Modules linked in:
> > CPU: 1 PID: 5178 Comm: syz-executor Not tainted 4.5.0-rc2+ #65
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
> > 01/01/2011
> > task: ffff880064768000 ti: ffff8800622c0000 task.ti:
> > ffff8800622c0000
> > RIP: 0010:[<ffffffff8181aa5d>] [<ffffffff8181aa5d>]
> > atime_needs_update+0x2d/0x460
> > RSP: 0018:ffff8800622c7a30 EFLAGS: 00010203
> > RAX: dffffc0000000000 RBX: 0000000000000000 RCX: dffffc0000000000
> > RDX: 0000000000000001 RSI: 0000000000000000 RDI: 000000000000000c
> > RBP: ffff8800622c7a58 R08: 0000000000000001 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000001 R12: ffff8800622c7c08
> > R13: ffff8800622c7c08 R14: ffff8800301ca322 R15: ffff8800622c7bb0
> > FS: 00007fd1c9f8b700(0000) GS:ffff88003ed00000(0000)
> > knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> > CR2: 0000000020f31000 CR3: 0000000062274000 CR4: 00000000000006e0
> > Stack:
> > ffff8800622c7bf4 0000000000000000 ffff8800622c7c08 ffff8800301ca322
> > ffff8800622c7bb0 ffff8800622c7b38 ffffffff817ecd91 ffff880030bf5200
> > ffff8800622c7bb8 1ffff1000c458f56 ffff8800622c7c00 ffff8800622c7be0
> > Call Trace:
> > [< inline >] get_link fs/namei.c:1006
> > [<ffffffff817ecd91>] link_path_walk+0xaf1/0x1030 fs/namei.c:1968
> > [<ffffffff817ed311>] path_parentat+0x41/0x150 fs/namei.c:2176
> > [<ffffffff817f4c5c>] filename_parentat+0x17c/0x3c0 fs/namei.c:2198
> > [< inline >] user_path_parent fs/namei.c:2412
> > [< inline >] SYSC_renameat2 fs/namei.c:4411
> > [< inline >] SyS_renameat2 fs/namei.c:4375
> > [< inline >] SYSC_renameat fs/namei.c:4521
> > [<ffffffff817f9a72>] SyS_renameat+0x192/0x820 fs/namei.c:4518
> > [<ffffffff8669e0b6>] entry_SYSCALL_64_fastpath+0x16/0x7a
> > arch/x86/entry/entry_64.S:185
> > Code: 89 e5 41 57 41 56 41 55 41 54 49 89 fc 53 48 89 f3 e8 08 25 d5
> > ff 48 8d 7b 0c 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03
> > <0f>
> > b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85
> > RIP [<ffffffff8181aa5d>] atime_needs_update+0x2d/0x460
> > fs/inode.c:1611
> > RSP <ffff8800622c7a30>
> > ---[ end trace 1a4c9bda4680ce46 ]---
> >
> > On commit df48ab3c2f5ffca88b7803ffbadd074bd5a0a2ef.
> >
> > Objdump shows that inode is NULL in atime_needs_update.
> >
> > Unfortunately reproduction of this crash is very hard. The program
> > executes something along the lines of:
> >
> > mmap(0x20000000, 15945728, PROT_READ|PROT_WRITE,
> > MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20000000
> > mkdir("./bus", 0662515705056234013740) = 0
> > openat(AT_FDCWD, "./bus", O_RDONLY|O_EXCL) = 3
> > symlinkat("../bus", 3, "./bus") = 0
> > renameat(3, "./bus", 3, "./bus/file0") = 0
> > mmap(0x20f35000, 4096, PROT_READ|PROT_WRITE,
> > MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20f35000
> > mount("./bus", "./bus", 0x20f2aee4,
> > MS_RDONLY|MS_NODEV|MS_RELATIME|MS_NODIRATIME|MS_BIND|MS_MOVE|MS_REC|
> > MS_UNBINDABLE|MS_SLAVE|MS_SHARED|0xc000380,
> > 0x20093f5f) = 0
> > open("./bus/file0", O_RDWR|O_EXCL) = -1 EISDIR (Is a directory)
> > exit_group(0) = ?
> >
> > But in multiple threads so that some calls can be doubled and/or
> > overlapped. And all this happens on a tmpfs mount.
> >
> > But I was able to reproduce it 8 or so times, so I am sure that it
> > is real.
> >
> > For future reference, I was running these programs:
> > https://gist.githubusercontent.com/dvyukov/124c457d308fa724d88a/raw/
> > fec2d86e125a7fd2fa2916791d65d7daead7cbbb/gistfile1.txt
> > Following these instructions:
> > https://github.com/google/syzkaller/wiki/How-to-execute-syzkaller-pr
> > ograms
> >
>
>
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-02-24 05:50 +0100 |
| Message-ID | <r5zyG-7GA-25@gated-at.bofh.it> |
| In reply to | #1341265 |
On Wed, Feb 24, 2016 at 11:12:13AM +0800, Ian Kent wrote:
> On Wed, 2016-02-17 at 00:40 +0100, Mickaël Salaün wrote:
> > Hi,
> >
> > Actually I found the same bug (without fuzzing) and I can reproduce it
> > in a deterministic way (e.g. by creating a LSM that return 1 for the
> > security_file_open hook). At least, from v4.2.8 I can easily trigger
> > traces like this :
>
> Reading through this thread I wonder if this is a new problem.
>
> Is there a previous kernel it can't be reproduced on?
> Perhaps a bisect will shed some light on what's happening.
There are several things in the mix. What Mickaël has found is that a bunch
of places where _positive_ number returned instead of expected 0 or -E... can
get propagated all way back to caller of do_last(), confusing the hell out
of it. That's not what Dmitry has triggered, though. WARN_ON() in the
end of do_last() would've triggered, and IMO this one, along with mitigation
(map that "error value" to -EINVAL) should go into mainline and all -stable.
Bogus ->open() returning a positive number had always been bad news; in the
best case it would be returned to userland, leading to "open(2) has failed
and returned a positive number". Hell knows if we ever had such instances
(or have them right now), but I wouldn't bet on their absense. Rare
failure exits returning bogus values in an ->open() instance in some driver
can easily stay unnoticed for a long time. Starting from at least 3.6
(circa the atomic_open support) it got more unpleasant than simple "confuse
the hell out of userland". ->open() isn't the only vector for injection of
such crap - ->permission() would also serve, same for several LSM turds, etc.
Again, that's a separate problem. What Dmitry seems to be catching is getting
crap values fed to should_follow_link() as inode. I see one bug in that
area that does need fixing (fix present in the last patch I've posted, with
WARN_ON() to indicate that this thing has triggered; _that_ WARN_ON() should
be gone from the final variant, since this can trigger without driver bugs,
etc.) In RCU mode after we'd checked that dentry is a symlink one, we need
to verify that it hadn't been changed since before we'd fetched the inode.
It might have been e.g. a regular file, which got unlinked with symlink
created in its place. Then we'd go into get_link() with non-symlink inode
and oops on attempt to call its ->i_op->get_link(). That race is real, very
hard to hit (you need both the unlink(2) and symlink(2) to happen between
lookup_fast() and should_follow_link() and unless you have PREEMPT_RCU you
can't lose the timeslice there) and would've manifested differently.
But that leaves other two kinds of bugs getting triggered: inode of some
non-symlink is possible, but what we saw included NULL inode when we'd
reached finish_open: in do_last(). Should be flat-out impossible - we either
get lookup_fast(..., &inode, ...) return 0 and store NULL in inode, or
get NULL inode from pinned d_is_symlink() dentry after having grabbed
a reference and left RCU mode. Neither should be possible without either
something weird happening to lookup_fast() (and we would've seen oopsen in
link_path_walk() if that could happen, BTW) or screwed dentry refcounting
somewhere, combined with a race that managed to turn...
Oh, shit. No screwed refcounting is needed. Look:
BUG_ON(nd->flags & LOOKUP_RCU);
inode = d_backing_inode(path.dentry);
seq = 0; /* out of RCU mode, so the value doesn't matter */
if (unlikely(d_is_negative(path.dentry))) {
path_to_nameidata(&path, nd);
return -ENOENT;
}
Suppose we come here with negative path.dentry. We are holding a reference,
all right, and for a _postive_ dentry that would've been enough to keep
it positive. Not so for a negative one, though - symlink(2) on another
CPU doint d_instantiate() just before the d_is_negative() check and we
are fucked - inode is stale and we sail through all the checks, all the
way into should_follow_link().
We also have the same kind of crap in walk_component() -
err = lookup_slow(nd, &path);
if (err < 0)
return err;
inode = d_backing_inode(path.dentry);
seq = 0; /* we are already out of RCU mode */
err = -ENOENT;
if (d_is_negative(path.dentry))
goto out_path_put;
There it's much harder to hit, though - we need it not just d_instantiate()
overlapping those lines; we need the racing syscall to get from locking
the parent to d_instantiate() between the point where lookup_slow() has
unlocked the parent and d_is_negative(). And lookup_slow() couldn't have
gone into mountpoint crossing, so it's really hard to hit - you pretty
much have to get preempted just after fetching inode.
OK, the next delta to try, and there definitely are several commits in
that pile. It still does not explain the traces with GPF at 0x50, though -
for that we need not just a NULL getting to should_follow_link() but
something non-NULL with NULL at offset 40 from it (offset of ->i_sb in
struct inode). That something *can't* be a valid struct inode or had been
one in recent past - ->i_sb is assigned in new_inode(), value is always
non-NULL and never modified all the way until RCU-delayed freeing of struct
inode. So that has to be something entirely different... Anyway, the
patch so far follows:
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index c6d7d3d..86f81e3 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -323,6 +323,7 @@ static struct dentry *autofs4_mountpoint_changed(struct path *path)
struct dentry *new = d_lookup(parent, &dentry->d_name);
if (!new)
return NULL;
+ WARN_ON(d_is_negative(new));
ino = autofs4_dentry_ino(new);
ino->last_used = jiffies;
dput(path->dentry);
diff --git a/fs/namei.c b/fs/namei.c
index f624d13..a5bcf63 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1209,6 +1209,7 @@ static int follow_managed(struct path *path, struct nameidata *nd)
/* Handle an automount point */
if (managed & DCACHE_NEED_AUTOMOUNT) {
ret = follow_automount(path, nd, &need_mntput);
+ WARN_ON(d_is_negative(path->dentry));
if (ret < 0)
break;
continue;
@@ -1613,8 +1614,10 @@ unlazy:
path->mnt = mnt;
path->dentry = dentry;
err = follow_managed(path, nd);
- if (likely(!err))
+ if (likely(!err)) {
*inode = d_backing_inode(path->dentry);
+ WARN_ON(!inode);
+ }
return err;
need_lookup:
@@ -1712,6 +1715,17 @@ static inline int should_follow_link(struct nameidata *nd, struct path *link,
return 0;
if (!follow)
return 0;
+ /* make sure that d_is_symlink above matches inode */
+ if (nd->flags & LOOKUP_RCU) {
+ if (read_seqcount_retry(&link->dentry->d_seq, seq)) {
+ WARN_ON(1); // just as way to report hitting
+ // that path; it's OK to get
+ // here, in the final variant
+ // WARN_ON will disappear.
+ return -ECHILD;
+ }
+ }
+ WARN_ON(!inode); // now, _that_ should not happen.
return pick_link(nd, link, inode, seq);
}
@@ -1743,11 +1757,11 @@ static int walk_component(struct nameidata *nd, int flags)
if (err < 0)
return err;
- inode = d_backing_inode(path.dentry);
seq = 0; /* we are already out of RCU mode */
err = -ENOENT;
if (d_is_negative(path.dentry))
goto out_path_put;
+ inode = d_backing_inode(path.dentry);
}
if (flags & WALK_PUT)
@@ -3192,12 +3206,12 @@ retry_lookup:
return error;
BUG_ON(nd->flags & LOOKUP_RCU);
- inode = d_backing_inode(path.dentry);
seq = 0; /* out of RCU mode, so the value doesn't matter */
if (unlikely(d_is_negative(path.dentry))) {
path_to_nameidata(&path, nd);
return -ENOENT;
}
+ inode = d_backing_inode(path.dentry);
finish_lookup:
if (nd->depth)
put_link(nd);
@@ -3206,11 +3220,6 @@ finish_lookup:
if (unlikely(error))
return error;
- if (unlikely(d_is_symlink(path.dentry)) && !(open_flag & O_PATH)) {
- path_to_nameidata(&path, nd);
- return -ELOOP;
- }
-
if ((nd->flags & LOOKUP_RCU) || nd->path.mnt != path.mnt) {
path_to_nameidata(&path, nd);
} else {
@@ -3229,6 +3238,10 @@ finish_open:
return error;
}
audit_inode(nd->name, nd->path.dentry, 0);
+ if (unlikely(d_is_symlink(nd->path.dentry)) && !(open_flag & O_PATH)) {
+ error = -ELOOP;
+ goto out;
+ }
error = -EISDIR;
if ((open_flag & O_CREAT) && d_is_dir(nd->path.dentry))
goto out;
@@ -3273,6 +3286,10 @@ opened:
goto exit_fput;
}
out:
+ if (unlikely(error > 0)) {
+ WARN_ON(1);
+ error = -EINVAL;
+ }
if (got_write)
mnt_drop_write(nd->path.mnt);
path_put(&save_parent);
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2016-02-24 11:10 +0100 |
| Message-ID | <r5Eym-2WS-13@gated-at.bofh.it> |
| In reply to | #1341443 |
On Wed, Feb 24, 2016 at 5:46 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Feb 24, 2016 at 11:12:13AM +0800, Ian Kent wrote:
>> On Wed, 2016-02-17 at 00:40 +0100, Mickaël Salaün wrote:
>> > Hi,
>> >
>> > Actually I found the same bug (without fuzzing) and I can reproduce it
>> > in a deterministic way (e.g. by creating a LSM that return 1 for the
>> > security_file_open hook). At least, from v4.2.8 I can easily trigger
>> > traces like this :
>>
>> Reading through this thread I wonder if this is a new problem.
>>
>> Is there a previous kernel it can't be reproduced on?
>> Perhaps a bisect will shed some light on what's happening.
>
> There are several things in the mix. What Mickaël has found is that a bunch
> of places where _positive_ number returned instead of expected 0 or -E... can
> get propagated all way back to caller of do_last(), confusing the hell out
> of it. That's not what Dmitry has triggered, though. WARN_ON() in the
> end of do_last() would've triggered, and IMO this one, along with mitigation
> (map that "error value" to -EINVAL) should go into mainline and all -stable.
> Bogus ->open() returning a positive number had always been bad news; in the
> best case it would be returned to userland, leading to "open(2) has failed
> and returned a positive number". Hell knows if we ever had such instances
> (or have them right now), but I wouldn't bet on their absense. Rare
> failure exits returning bogus values in an ->open() instance in some driver
> can easily stay unnoticed for a long time. Starting from at least 3.6
> (circa the atomic_open support) it got more unpleasant than simple "confuse
> the hell out of userland". ->open() isn't the only vector for injection of
> such crap - ->permission() would also serve, same for several LSM turds, etc.
>
> Again, that's a separate problem. What Dmitry seems to be catching is getting
> crap values fed to should_follow_link() as inode. I see one bug in that
> area that does need fixing (fix present in the last patch I've posted, with
> WARN_ON() to indicate that this thing has triggered; _that_ WARN_ON() should
> be gone from the final variant, since this can trigger without driver bugs,
> etc.) In RCU mode after we'd checked that dentry is a symlink one, we need
> to verify that it hadn't been changed since before we'd fetched the inode.
> It might have been e.g. a regular file, which got unlinked with symlink
> created in its place. Then we'd go into get_link() with non-symlink inode
> and oops on attempt to call its ->i_op->get_link(). That race is real, very
> hard to hit (you need both the unlink(2) and symlink(2) to happen between
> lookup_fast() and should_follow_link() and unless you have PREEMPT_RCU you
> can't lose the timeslice there) and would've manifested differently.
>
> But that leaves other two kinds of bugs getting triggered: inode of some
> non-symlink is possible, but what we saw included NULL inode when we'd
> reached finish_open: in do_last(). Should be flat-out impossible - we either
> get lookup_fast(..., &inode, ...) return 0 and store NULL in inode, or
> get NULL inode from pinned d_is_symlink() dentry after having grabbed
> a reference and left RCU mode. Neither should be possible without either
> something weird happening to lookup_fast() (and we would've seen oopsen in
> link_path_walk() if that could happen, BTW) or screwed dentry refcounting
> somewhere, combined with a race that managed to turn...
>
> Oh, shit. No screwed refcounting is needed. Look:
> BUG_ON(nd->flags & LOOKUP_RCU);
> inode = d_backing_inode(path.dentry);
> seq = 0; /* out of RCU mode, so the value doesn't matter */
> if (unlikely(d_is_negative(path.dentry))) {
> path_to_nameidata(&path, nd);
> return -ENOENT;
> }
> Suppose we come here with negative path.dentry. We are holding a reference,
> all right, and for a _postive_ dentry that would've been enough to keep
> it positive. Not so for a negative one, though - symlink(2) on another
> CPU doint d_instantiate() just before the d_is_negative() check and we
> are fucked - inode is stale and we sail through all the checks, all the
> way into should_follow_link().
>
> We also have the same kind of crap in walk_component() -
> err = lookup_slow(nd, &path);
> if (err < 0)
> return err;
> inode = d_backing_inode(path.dentry);
> seq = 0; /* we are already out of RCU mode */
> err = -ENOENT;
> if (d_is_negative(path.dentry))
> goto out_path_put;
> There it's much harder to hit, though - we need it not just d_instantiate()
> overlapping those lines; we need the racing syscall to get from locking
> the parent to d_instantiate() between the point where lookup_slow() has
> unlocked the parent and d_is_negative(). And lookup_slow() couldn't have
> gone into mountpoint crossing, so it's really hard to hit - you pretty
> much have to get preempted just after fetching inode.
>
> OK, the next delta to try, and there definitely are several commits in
> that pile. It still does not explain the traces with GPF at 0x50, though -
> for that we need not just a NULL getting to should_follow_link() but
> something non-NULL with NULL at offset 40 from it (offset of ->i_sb in
> struct inode). That something *can't* be a valid struct inode or had been
> one in recent past - ->i_sb is assigned in new_inode(), value is always
> non-NULL and never modified all the way until RCU-delayed freeing of struct
> inode. So that has to be something entirely different... Anyway, the
> patch so far follows:
Restarted testing with this patch (dropped previous patches because
they conflict).
These "unlikely" scenarios can be more likely inside of VMs where
effective preemption can happen at random points. Also NMIs probably
can increase probability of such races.
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2016-02-24 11:20 +0100 |
| Message-ID | <r5EI3-313-41@gated-at.bofh.it> |
| In reply to | #1341711 |
On Wed, Feb 24, 2016 at 11:03 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Feb 24, 2016 at 5:46 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Wed, Feb 24, 2016 at 11:12:13AM +0800, Ian Kent wrote:
>>> On Wed, 2016-02-17 at 00:40 +0100, Mickaël Salaün wrote:
>>> > Hi,
>>> >
>>> > Actually I found the same bug (without fuzzing) and I can reproduce it
>>> > in a deterministic way (e.g. by creating a LSM that return 1 for the
>>> > security_file_open hook). At least, from v4.2.8 I can easily trigger
>>> > traces like this :
>>>
>>> Reading through this thread I wonder if this is a new problem.
>>>
>>> Is there a previous kernel it can't be reproduced on?
>>> Perhaps a bisect will shed some light on what's happening.
>>
>> There are several things in the mix. What Mickaël has found is that a bunch
>> of places where _positive_ number returned instead of expected 0 or -E... can
>> get propagated all way back to caller of do_last(), confusing the hell out
>> of it. That's not what Dmitry has triggered, though. WARN_ON() in the
>> end of do_last() would've triggered, and IMO this one, along with mitigation
>> (map that "error value" to -EINVAL) should go into mainline and all -stable.
>> Bogus ->open() returning a positive number had always been bad news; in the
>> best case it would be returned to userland, leading to "open(2) has failed
>> and returned a positive number". Hell knows if we ever had such instances
>> (or have them right now), but I wouldn't bet on their absense. Rare
>> failure exits returning bogus values in an ->open() instance in some driver
>> can easily stay unnoticed for a long time. Starting from at least 3.6
>> (circa the atomic_open support) it got more unpleasant than simple "confuse
>> the hell out of userland". ->open() isn't the only vector for injection of
>> such crap - ->permission() would also serve, same for several LSM turds, etc.
>>
>> Again, that's a separate problem. What Dmitry seems to be catching is getting
>> crap values fed to should_follow_link() as inode. I see one bug in that
>> area that does need fixing (fix present in the last patch I've posted, with
>> WARN_ON() to indicate that this thing has triggered; _that_ WARN_ON() should
>> be gone from the final variant, since this can trigger without driver bugs,
>> etc.) In RCU mode after we'd checked that dentry is a symlink one, we need
>> to verify that it hadn't been changed since before we'd fetched the inode.
>> It might have been e.g. a regular file, which got unlinked with symlink
>> created in its place. Then we'd go into get_link() with non-symlink inode
>> and oops on attempt to call its ->i_op->get_link(). That race is real, very
>> hard to hit (you need both the unlink(2) and symlink(2) to happen between
>> lookup_fast() and should_follow_link() and unless you have PREEMPT_RCU you
>> can't lose the timeslice there) and would've manifested differently.
>>
>> But that leaves other two kinds of bugs getting triggered: inode of some
>> non-symlink is possible, but what we saw included NULL inode when we'd
>> reached finish_open: in do_last(). Should be flat-out impossible - we either
>> get lookup_fast(..., &inode, ...) return 0 and store NULL in inode, or
>> get NULL inode from pinned d_is_symlink() dentry after having grabbed
>> a reference and left RCU mode. Neither should be possible without either
>> something weird happening to lookup_fast() (and we would've seen oopsen in
>> link_path_walk() if that could happen, BTW) or screwed dentry refcounting
>> somewhere, combined with a race that managed to turn...
>>
>> Oh, shit. No screwed refcounting is needed. Look:
>> BUG_ON(nd->flags & LOOKUP_RCU);
>> inode = d_backing_inode(path.dentry);
>> seq = 0; /* out of RCU mode, so the value doesn't matter */
>> if (unlikely(d_is_negative(path.dentry))) {
>> path_to_nameidata(&path, nd);
>> return -ENOENT;
>> }
>> Suppose we come here with negative path.dentry. We are holding a reference,
>> all right, and for a _postive_ dentry that would've been enough to keep
>> it positive. Not so for a negative one, though - symlink(2) on another
>> CPU doint d_instantiate() just before the d_is_negative() check and we
>> are fucked - inode is stale and we sail through all the checks, all the
>> way into should_follow_link().
>>
>> We also have the same kind of crap in walk_component() -
>> err = lookup_slow(nd, &path);
>> if (err < 0)
>> return err;
>> inode = d_backing_inode(path.dentry);
>> seq = 0; /* we are already out of RCU mode */
>> err = -ENOENT;
>> if (d_is_negative(path.dentry))
>> goto out_path_put;
>> There it's much harder to hit, though - we need it not just d_instantiate()
>> overlapping those lines; we need the racing syscall to get from locking
>> the parent to d_instantiate() between the point where lookup_slow() has
>> unlocked the parent and d_is_negative(). And lookup_slow() couldn't have
>> gone into mountpoint crossing, so it's really hard to hit - you pretty
>> much have to get preempted just after fetching inode.
>>
>> OK, the next delta to try, and there definitely are several commits in
>> that pile. It still does not explain the traces with GPF at 0x50, though -
>> for that we need not just a NULL getting to should_follow_link() but
>> something non-NULL with NULL at offset 40 from it (offset of ->i_sb in
>> struct inode). That something *can't* be a valid struct inode or had been
>> one in recent past - ->i_sb is assigned in new_inode(), value is always
>> non-NULL and never modified all the way until RCU-delayed freeing of struct
>> inode. So that has to be something entirely different... Anyway, the
>> patch so far follows:
>
>
> Restarted testing with this patch (dropped previous patches because
> they conflict).
>
> These "unlikely" scenarios can be more likely inside of VMs where
> effective preemption can happen at random points. Also NMIs probably
> can increase probability of such races.
For now I can only say that I am hitting this one (3 times in 20 mins):
if (read_seqcount_retry(&link->dentry->d_seq, seq)) {
WARN_ON(1); // just as way to report hitting
// that path; it's OK to get
// here, in the final variant
// WARN_ON will disappear.
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2016-02-24 14:40 +0100 |
| Message-ID | <r5HPA-5eb-31@gated-at.bofh.it> |
| In reply to | #1341749 |
On Wed, Feb 24, 2016 at 11:15 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Feb 24, 2016 at 11:03 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Wed, Feb 24, 2016 at 5:46 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>> On Wed, Feb 24, 2016 at 11:12:13AM +0800, Ian Kent wrote:
>>>> On Wed, 2016-02-17 at 00:40 +0100, Mickaël Salaün wrote:
>>>> > Hi,
>>>> >
>>>> > Actually I found the same bug (without fuzzing) and I can reproduce it
>>>> > in a deterministic way (e.g. by creating a LSM that return 1 for the
>>>> > security_file_open hook). At least, from v4.2.8 I can easily trigger
>>>> > traces like this :
>>>>
>>>> Reading through this thread I wonder if this is a new problem.
>>>>
>>>> Is there a previous kernel it can't be reproduced on?
>>>> Perhaps a bisect will shed some light on what's happening.
>>>
>>> There are several things in the mix. What Mickaël has found is that a bunch
>>> of places where _positive_ number returned instead of expected 0 or -E... can
>>> get propagated all way back to caller of do_last(), confusing the hell out
>>> of it. That's not what Dmitry has triggered, though. WARN_ON() in the
>>> end of do_last() would've triggered, and IMO this one, along with mitigation
>>> (map that "error value" to -EINVAL) should go into mainline and all -stable.
>>> Bogus ->open() returning a positive number had always been bad news; in the
>>> best case it would be returned to userland, leading to "open(2) has failed
>>> and returned a positive number". Hell knows if we ever had such instances
>>> (or have them right now), but I wouldn't bet on their absense. Rare
>>> failure exits returning bogus values in an ->open() instance in some driver
>>> can easily stay unnoticed for a long time. Starting from at least 3.6
>>> (circa the atomic_open support) it got more unpleasant than simple "confuse
>>> the hell out of userland". ->open() isn't the only vector for injection of
>>> such crap - ->permission() would also serve, same for several LSM turds, etc.
>>>
>>> Again, that's a separate problem. What Dmitry seems to be catching is getting
>>> crap values fed to should_follow_link() as inode. I see one bug in that
>>> area that does need fixing (fix present in the last patch I've posted, with
>>> WARN_ON() to indicate that this thing has triggered; _that_ WARN_ON() should
>>> be gone from the final variant, since this can trigger without driver bugs,
>>> etc.) In RCU mode after we'd checked that dentry is a symlink one, we need
>>> to verify that it hadn't been changed since before we'd fetched the inode.
>>> It might have been e.g. a regular file, which got unlinked with symlink
>>> created in its place. Then we'd go into get_link() with non-symlink inode
>>> and oops on attempt to call its ->i_op->get_link(). That race is real, very
>>> hard to hit (you need both the unlink(2) and symlink(2) to happen between
>>> lookup_fast() and should_follow_link() and unless you have PREEMPT_RCU you
>>> can't lose the timeslice there) and would've manifested differently.
>>>
>>> But that leaves other two kinds of bugs getting triggered: inode of some
>>> non-symlink is possible, but what we saw included NULL inode when we'd
>>> reached finish_open: in do_last(). Should be flat-out impossible - we either
>>> get lookup_fast(..., &inode, ...) return 0 and store NULL in inode, or
>>> get NULL inode from pinned d_is_symlink() dentry after having grabbed
>>> a reference and left RCU mode. Neither should be possible without either
>>> something weird happening to lookup_fast() (and we would've seen oopsen in
>>> link_path_walk() if that could happen, BTW) or screwed dentry refcounting
>>> somewhere, combined with a race that managed to turn...
>>>
>>> Oh, shit. No screwed refcounting is needed. Look:
>>> BUG_ON(nd->flags & LOOKUP_RCU);
>>> inode = d_backing_inode(path.dentry);
>>> seq = 0; /* out of RCU mode, so the value doesn't matter */
>>> if (unlikely(d_is_negative(path.dentry))) {
>>> path_to_nameidata(&path, nd);
>>> return -ENOENT;
>>> }
>>> Suppose we come here with negative path.dentry. We are holding a reference,
>>> all right, and for a _postive_ dentry that would've been enough to keep
>>> it positive. Not so for a negative one, though - symlink(2) on another
>>> CPU doint d_instantiate() just before the d_is_negative() check and we
>>> are fucked - inode is stale and we sail through all the checks, all the
>>> way into should_follow_link().
>>>
>>> We also have the same kind of crap in walk_component() -
>>> err = lookup_slow(nd, &path);
>>> if (err < 0)
>>> return err;
>>> inode = d_backing_inode(path.dentry);
>>> seq = 0; /* we are already out of RCU mode */
>>> err = -ENOENT;
>>> if (d_is_negative(path.dentry))
>>> goto out_path_put;
>>> There it's much harder to hit, though - we need it not just d_instantiate()
>>> overlapping those lines; we need the racing syscall to get from locking
>>> the parent to d_instantiate() between the point where lookup_slow() has
>>> unlocked the parent and d_is_negative(). And lookup_slow() couldn't have
>>> gone into mountpoint crossing, so it's really hard to hit - you pretty
>>> much have to get preempted just after fetching inode.
>>>
>>> OK, the next delta to try, and there definitely are several commits in
>>> that pile. It still does not explain the traces with GPF at 0x50, though -
>>> for that we need not just a NULL getting to should_follow_link() but
>>> something non-NULL with NULL at offset 40 from it (offset of ->i_sb in
>>> struct inode). That something *can't* be a valid struct inode or had been
>>> one in recent past - ->i_sb is assigned in new_inode(), value is always
>>> non-NULL and never modified all the way until RCU-delayed freeing of struct
>>> inode. So that has to be something entirely different... Anyway, the
>>> patch so far follows:
Fired after some time:
[ 3491.147607] ------------[ cut here ]------------
[ 3491.147986] WARNING: CPU: 1 PID: 17000 at fs/namei.c:1728
path_openat+0x14e5/0x1560()
[ 3491.148576] Modules linked in:
[ 3491.148818] CPU: 1 PID: 17000 Comm: syz-executor Tainted: G
W 4.5.0-rc5+ #72
[ 3491.149530] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Bochs 01/01/2011
[ 3491.149530] 0000000000000000 ffff88002d6ebcb8 ffffffff8194ad79
0000000000000000
[ 3491.152103] ffffffff8334501c ffff88002d6ebcf0 ffffffff81172291
0000000000000000
[ 3491.152103] ffff88002d6ebd98 ffff88002d6ebde0 0000000000048000
ffff88002d6ebefc
[ 3491.155310] Call Trace:
[ 3491.155310] [<ffffffff8194ad79>] dump_stack+0x99/0xd0
[ 3491.155310] [<ffffffff81172291>] warn_slowpath_common+0x81/0xc0
[ 3491.157483] [<ffffffff81172385>] warn_slowpath_null+0x15/0x20
[ 3491.157483] [<ffffffff81318495>] path_openat+0x14e5/0x1560
[ 3491.157483] [<ffffffff81319549>] do_filp_open+0x79/0xd0
[ 3491.157483] [<ffffffff82b11d92>] ? _raw_spin_unlock+0x22/0x30
[ 3491.157483] [<ffffffff81328f08>] ? __alloc_fd+0xf8/0x200
[ 3491.157483] [<ffffffff81306eb0>] do_sys_open+0x110/0x1f0
[ 3491.157483] [<ffffffff81306fbf>] SyS_openat+0xf/0x20
[ 3491.157483] [<ffffffff82b12836>] entry_SYSCALL_64_fastpath+0x16/0x7a
[ 3491.161521] ---[ end trace c65af9be7536efae ]---
[ 3491.161838] BUG: unable to handle kernel NULL pointer dereference
at 000000000000000c
[ 3491.162339] IP: [<ffffffff81327429>] atime_needs_update+0x9/0xc0
[ 3491.162577] PGD 2d626067 PUD 2c4cc067 PMD 0
[ 3491.163036] Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
[ 3491.163039] Modules linked in:
[ 3491.163039] CPU: 1 PID: 17000 Comm: syz-executor Tainted: G
W 4.5.0-rc5+ #72
[ 3491.163039] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Bochs 01/01/2011
[ 3491.164386] task: ffff88002c9196c0 ti: ffff88002d6e8000 task.ti:
ffff88002d6e8000
[ 3491.164386] RIP: 0010:[<ffffffff81327429>] [<ffffffff81327429>]
atime_needs_update+0x9/0xc0
[ 3491.164386] RSP: 0018:ffff88002d6ebcb0 EFLAGS: 00010282
[ 3491.165591] RAX: 0000000000000030 RBX: ffff88002d6ebde0 RCX: 0000000000000002
[ 3491.165591] RDX: ffff88002d6ebe38 RSI: 0000000000000000 RDI: ffff88002d6ebe38
[ 3491.165591] RBP: ffff88002d6ebcc0 R08: ffff88002d6ebe38 R09: 0000000000000001
[ 3491.167202] R10: 0000000000000001 R11: 0000000000001816 R12: 0000000000000000
[ 3491.167368] R13: ffff88002d6ebe38 R14: ffff880072508980 R15: ffff88002d6ebefc
[ 3491.167368] FS: 00007f02454e5700(0000) GS:ffff88003ed00000(0000)
knlGS:0000000000000000
[ 3491.167368] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 3491.167368] CR2: 000000000000000c CR3: 000000002cb5f000 CR4: 00000000000006e0
[ 3491.167368] Stack:
[ 3491.169740] 0000000000000000 ffff88002d6ebde0 ffff88002d6ebd00
ffffffff81314922
[ 3491.169740] 0000000000000001 0000000000000000 ffff88002d6ebd98
ffff88002d6ebde0
[ 3491.169740] 0000000000048000 ffff88002d6ebefc ffff88002d6ebdd0
ffffffff8131729d
[ 3491.169740] Call Trace:
[ 3491.169740] [<ffffffff81314922>] trailing_symlink+0x62/0x260
[ 3491.169740] [<ffffffff8131729d>] path_openat+0x2ed/0x1560
[ 3491.169740] [<ffffffff81319549>] do_filp_open+0x79/0xd0
[ 3491.169740] [<ffffffff82b11d92>] ? _raw_spin_unlock+0x22/0x30
[ 3491.169740] [<ffffffff81328f08>] ? __alloc_fd+0xf8/0x200
[ 3491.169740] [<ffffffff81306eb0>] do_sys_open+0x110/0x1f0
[ 3491.169740] [<ffffffff81306fbf>] SyS_openat+0xf/0x20
[ 3491.169740] [<ffffffff82b12836>] entry_SYSCALL_64_fastpath+0x16/0x7a
[ 3491.169740] Code: ff ff ff 48 85 c0 48 89 c3 74 08 48 89 c7 e8 ef
dc ff ff 48 89 d8 5b 5d c3 66 0f 1f 84 00 00 00 00 00 55 48 89 e5 53
48 83 ec 08 <f6> 46 0c 02 48 8b 1f 75 6b 48 8b 7e 28 48 8b 47 50 a9 01
04 00
[ 3491.169740] RIP [<ffffffff81327429>] atime_needs_update+0x9/0xc0
[ 3491.169740] RSP <ffff88002d6ebcb0>
[ 3491.169740] CR2: 000000000000000c
[ 3491.195051] ---[ end trace c65af9be7536efaf ]---
[ 3491.195051] BUG: sleeping function called from invalid context at
include/linux/sched.h:2795
[ 3491.195051] in_atomic(): 1, irqs_disabled(): 1, pid: 17000, name:
syz-executor
[ 3491.195051] INFO: lockdep is turned off.
[ 3491.195051] irq event stamp: 622
[ 3491.195051] hardirqs last enabled at (621): [<ffffffff811d9926>]
vprintk_emit+0x2d6/0x5f0
[ 3491.195051] hardirqs last disabled at (622): [<ffffffff82b14d49>]
error_entry+0x69/0xc0
[ 3491.195051] softirqs last enabled at (618): [<ffffffff81178172>]
__do_softirq+0x222/0x4a0
[ 3491.195051] softirqs last disabled at (595): [<ffffffff81178767>]
irq_exit+0xa7/0xc0
[ 3491.195051] CPU: 1 PID: 17000 Comm: syz-executor Tainted: G D
W 4.5.0-rc5+ #72
[ 3491.195051] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Bochs 01/01/2011
[ 3491.195051] 0000000000000000 ffff88002d6eb9a8 ffffffff8194ad79
ffff88002c9196c0
[ 3491.195051] 0000000000004268 ffff88002d6eb9d0 ffffffff811a0659
ffffffff8329b7fb
[ 3491.195051] 0000000000000aeb 0000000000000000 ffff88002d6eb9f8
ffffffff811a0764
[ 3491.195051] Call Trace:
[ 3491.195051] [<ffffffff8194ad79>] dump_stack+0x99/0xd0
[ 3491.195051] [<ffffffff811a0659>] ___might_sleep+0x179/0x240
[ 3491.195051] [<ffffffff811a0764>] __might_sleep+0x44/0x80
[ 3491.195051] [<ffffffff811842cf>] exit_signals+0x1f/0x130
[ 3491.195051] [<ffffffff811756ef>] do_exit+0xbf/0xd10
[ 3491.195051] [<ffffffff811da524>] ? kmsg_dump+0x104/0x180
[ 3491.195051] [<ffffffff8108549f>] oops_end+0x9f/0xe0
[ 3491.195051] [<ffffffff810ce6e8>] no_context+0x108/0x390
[ 3491.195051] [<ffffffff810cea8d>] __bad_area_nosemaphore+0x11d/0x220
[ 3491.195051] [<ffffffff810ceb9e>] bad_area_nosemaphore+0xe/0x10
[ 3491.195051] [<ffffffff810cf2c4>] __do_page_fault+0x84/0x470
[ 3491.195051] [<ffffffff810cf764>] trace_do_page_fault+0x74/0x2c0
[ 3491.195051] [<ffffffff810c9ba4>] do_async_page_fault+0x14/0x90
[ 3491.195051] [<ffffffff82b14b78>] async_page_fault+0x28/0x30
[ 3491.195051] [<ffffffff81327429>] ? atime_needs_update+0x9/0xc0
[ 3491.195051] [<ffffffff81314922>] trailing_symlink+0x62/0x260
[ 3491.195051] [<ffffffff8131729d>] path_openat+0x2ed/0x1560
[ 3491.195051] [<ffffffff81319549>] do_filp_open+0x79/0xd0
[ 3491.195051] [<ffffffff82b11d92>] ? _raw_spin_unlock+0x22/0x30
[ 3491.195051] [<ffffffff81328f08>] ? __alloc_fd+0xf8/0x200
[ 3491.195051] [<ffffffff81306eb0>] do_sys_open+0x110/0x1f0
[ 3491.195051] [<ffffffff81306fbf>] SyS_openat+0xf/0x20
[ 3491.195051] [<ffffffff82b12836>] entry_SYSCALL_64_fastpath+0x16/0x7a
[ 3491.284831] note: syz-executor[17000] exited with preempt_count 1
The warning is this one:
static inline int should_follow_link(struct nameidata *nd, struct path *link,
int follow,
struct inode *inode, unsigned seq)
{
....
WARN_ON(!inode); // now, _that_ should not happen.
return pick_link(nd, link, inode, seq);
}
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-02-24 16:20 +0100 |
| Message-ID | <r5Jom-6qY-7@gated-at.bofh.it> |
| In reply to | #1341983 |
On Wed, Feb 24, 2016 at 02:35:18PM +0100, Dmitry Vyukov wrote:
> The warning is this one:
>
> static inline int should_follow_link(struct nameidata *nd, struct path *link,
> int follow,
> struct inode *inode, unsigned seq)
> {
> ....
> WARN_ON(!inode); // now, _that_ should not happen.
> return pick_link(nd, link, inode, seq);
> }
Let's try it with less chatty should_follow_link() and better set of
tripwires on the way to it:
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index c6d7d3d..86f81e3 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -323,6 +323,7 @@ static struct dentry *autofs4_mountpoint_changed(struct path *path)
struct dentry *new = d_lookup(parent, &dentry->d_name);
if (!new)
return NULL;
+ WARN_ON(d_is_negative(new));
ino = autofs4_dentry_ino(new);
ino->last_used = jiffies;
dput(path->dentry);
diff --git a/fs/namei.c b/fs/namei.c
index f624d13..071a4ba 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1209,6 +1209,7 @@ static int follow_managed(struct path *path, struct nameidata *nd)
/* Handle an automount point */
if (managed & DCACHE_NEED_AUTOMOUNT) {
ret = follow_automount(path, nd, &need_mntput);
+ WARN_ON(d_is_negative(path->dentry));
if (ret < 0)
break;
continue;
@@ -1582,8 +1583,10 @@ static int lookup_fast(struct nameidata *nd,
return -ENOENT;
path->mnt = mnt;
path->dentry = dentry;
- if (likely(__follow_mount_rcu(nd, path, inode, seqp)))
+ if (likely(__follow_mount_rcu(nd, path, inode, seqp))) {
+ WARN_ON(!*inode);
return 0;
+ }
unlazy:
if (unlazy_walk(nd, dentry, seq))
return -ECHILD;
@@ -1613,8 +1616,10 @@ unlazy:
path->mnt = mnt;
path->dentry = dentry;
err = follow_managed(path, nd);
- if (likely(!err))
+ if (likely(!err)) {
*inode = d_backing_inode(path->dentry);
+ WARN_ON(!*inode);
+ }
return err;
need_lookup:
@@ -1712,6 +1717,12 @@ static inline int should_follow_link(struct nameidata *nd, struct path *link,
return 0;
if (!follow)
return 0;
+ /* make sure that d_is_symlink above matches inode */
+ if (nd->flags & LOOKUP_RCU) {
+ if (read_seqcount_retry(&link->dentry->d_seq, seq))
+ return -ECHILD;
+ }
+ WARN_ON(!inode); // now, _that_ should not happen.
return pick_link(nd, link, inode, seq);
}
@@ -1743,11 +1754,11 @@ static int walk_component(struct nameidata *nd, int flags)
if (err < 0)
return err;
- inode = d_backing_inode(path.dentry);
seq = 0; /* we are already out of RCU mode */
err = -ENOENT;
if (d_is_negative(path.dentry))
goto out_path_put;
+ inode = d_backing_inode(path.dentry);
}
if (flags & WALK_PUT)
@@ -3106,8 +3117,10 @@ static int do_last(struct nameidata *nd,
nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
/* we _can_ be in RCU mode here */
error = lookup_fast(nd, &path, &inode, &seq);
- if (likely(!error))
+ if (likely(!error)) {
+ WARN_ON(!inode);
goto finish_lookup;
+ }
if (error < 0)
return error;
@@ -3192,12 +3205,13 @@ retry_lookup:
return error;
BUG_ON(nd->flags & LOOKUP_RCU);
- inode = d_backing_inode(path.dentry);
seq = 0; /* out of RCU mode, so the value doesn't matter */
if (unlikely(d_is_negative(path.dentry))) {
path_to_nameidata(&path, nd);
return -ENOENT;
}
+ inode = d_backing_inode(path.dentry);
+ WARN_ON(!inode);
finish_lookup:
if (nd->depth)
put_link(nd);
@@ -3206,11 +3220,6 @@ finish_lookup:
if (unlikely(error))
return error;
- if (unlikely(d_is_symlink(path.dentry)) && !(open_flag & O_PATH)) {
- path_to_nameidata(&path, nd);
- return -ELOOP;
- }
-
if ((nd->flags & LOOKUP_RCU) || nd->path.mnt != path.mnt) {
path_to_nameidata(&path, nd);
} else {
@@ -3229,6 +3238,10 @@ finish_open:
return error;
}
audit_inode(nd->name, nd->path.dentry, 0);
+ if (unlikely(d_is_symlink(nd->path.dentry)) && !(open_flag & O_PATH)) {
+ error = -ELOOP;
+ goto out;
+ }
error = -EISDIR;
if ((open_flag & O_CREAT) && d_is_dir(nd->path.dentry))
goto out;
@@ -3273,6 +3286,10 @@ opened:
goto exit_fput;
}
out:
+ if (unlikely(error > 0)) {
+ WARN_ON(1);
+ error = -EINVAL;
+ }
if (got_write)
mnt_drop_write(nd->path.mnt);
path_put(&save_parent);
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2016-02-25 09:30 +0100 |
| Message-ID | <r5Zt8-120-13@gated-at.bofh.it> |
| In reply to | #1342104 |
On Wed, Feb 24, 2016 at 4:15 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Feb 24, 2016 at 02:35:18PM +0100, Dmitry Vyukov wrote:
>
>> The warning is this one:
>>
>> static inline int should_follow_link(struct nameidata *nd, struct path *link,
>> int follow,
>> struct inode *inode, unsigned seq)
>> {
>> ....
>> WARN_ON(!inode); // now, _that_ should not happen.
>> return pick_link(nd, link, inode, seq);
>> }
>
> Let's try it with less chatty should_follow_link() and better set of
> tripwires on the way to it:
Humm... I've left it running over night but no GPFs happened...
Usually they happened within two hours or so. I would think that your
patch fixes it and I did not actually apply it last time (or did not
rebuild kernel). But I saw the new warnings that the patch adds, so I
should have been rebuilt it...
What I saw is a dozen of pairs of warnings like the one below.
Is it possible the warning printing introduces enough delay to close
the inconsistency window?....
[46839.557154] ------------[ cut here ]------------
[46839.557663] WARNING: CPU: 0 PID: 10503 at fs/namei.c:1587
lookup_fast+0x2bf/0x420()
[46839.558239] Modules linked in:
[46839.558466] CPU: 0 PID: 10503 Comm: syz-executor Tainted: G
W 4.5.0-rc5+ #73
[46839.558980] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Bochs 01/01/2011
[46839.559174] 0000000000000000 ffff88002b6ebc48 ffffffff8194acd9
0000000000000000
[46839.559564] ffffffff83344ffc ffff88002b6ebc80 ffffffff81172291
ffff88002b6ebde0
[46839.559564] ffff8800314cc720 ffff88002b6ebd90 ffff88002b6ebd98
ffff88002b6ebd8c
[46839.560614] Call Trace:
[46839.560614] [<ffffffff8194acd9>] dump_stack+0x99/0xd0
[46839.561294] [<ffffffff81172291>] warn_slowpath_common+0x81/0xc0
[46839.561712] [<ffffffff81172385>] warn_slowpath_null+0x15/0x20
[46839.561712] [<ffffffff81314ddf>] lookup_fast+0x2bf/0x420
[46839.561712] [<ffffffff81315568>] ? link_path_walk+0x68/0x4e0
[46839.561712] [<ffffffff813172b5>] path_openat+0x375/0x1520
[46839.563137] [<ffffffff81319499>] do_filp_open+0x79/0xd0
[46839.563137] [<ffffffff82b11cf2>] ? _raw_spin_unlock+0x22/0x30
[46839.563137] [<ffffffff81328e58>] ? __alloc_fd+0xf8/0x200
[46839.564353] [<ffffffff81306eb0>] do_sys_open+0x110/0x1f0
[46839.564353] [<ffffffff81306fbf>] SyS_openat+0xf/0x20
[46839.564353] [<ffffffff82b12776>] entry_SYSCALL_64_fastpath+0x16/0x7a
[46839.565558] ---[ end trace 3c3bc0f927bf4e90 ]---
[46839.565858] ------------[ cut here ]------------
[46839.566203] WARNING: CPU: 0 PID: 10503 at fs/namei.c:3121
path_openat+0x12bc/0x1520()
[46839.566700] Modules linked in:
[46839.566914] CPU: 0 PID: 10503 Comm: syz-executor Tainted: G
W 4.5.0-rc5+ #73
[46839.567154] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Bochs 01/01/2011
[46839.567154] 0000000000000000 ffff88002b6ebcb8 ffffffff8194acd9
0000000000000000
[46839.567154] ffffffff83344ffc ffff88002b6ebcf0 ffffffff81172291
0000000000000005
[46839.567154] ffff88002b6ebd98 0000000000048000 ffff88002b6ebde0
ffff88002b6ebefc
[46839.567154] Call Trace:
[46839.567154] [<ffffffff8194acd9>] dump_stack+0x99/0xd0
[46839.567154] [<ffffffff81172291>] warn_slowpath_common+0x81/0xc0
[46839.567154] [<ffffffff81172385>] warn_slowpath_null+0x15/0x20
[46839.567154] [<ffffffff813181fc>] path_openat+0x12bc/0x1520
[46839.567154] [<ffffffff81319499>] do_filp_open+0x79/0xd0
[46839.567154] [<ffffffff82b11cf2>] ? _raw_spin_unlock+0x22/0x30
[46839.567154] [<ffffffff81328e58>] ? __alloc_fd+0xf8/0x200
[46839.567154] [<ffffffff81306eb0>] do_sys_open+0x110/0x1f0
[46839.567154] [<ffffffff81306fbf>] SyS_openat+0xf/0x20
[46839.567154] [<ffffffff82b12776>] entry_SYSCALL_64_fastpath+0x16/0x7a
[46839.573763] ---[ end trace 3c3bc0f927bf4e91 ]---
[toc] | [prev] | [next] | [standalone]
Page 1 of 3 [1] 2 3 Next page →
Back to top | Article view | linux.kernel
csiph-web