* Re: general protection fault in find_entry
2018-01-05 21:31 ` general protection fault in find_entry Luis R. Rodriguez
@ 2018-01-30 23:29 ` Eric Biggers
0 siblings, 0 replies; 2+ messages in thread
From: Eric Biggers @ 2018-01-30 23:29 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: syzbot, keescook, linux-fsdevel, linux-kernel, syzkaller-bugs
On Fri, Jan 05, 2018 at 10:31:04PM +0100, Luis R. Rodriguez wrote:
> On Fri, Dec 22, 2017 at 11:33:02PM -0800, syzbot wrote:
> > Hello,
> >
> > syzkaller hit the following crash on
> > 6084b576dca2e898f5c101baef151f7bfdbb606d
> > git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/master
> > compiler: gcc (GCC) 7.1.1 20170620
> > .config is attached
> > Raw console output is attached.
> >
> > Unfortunately, I don't have any reproducer for this bug yet.
> >
> >
> > netlink: 13 bytes leftover after parsing attributes in process
> > `syz-executor2'.
> > general protection fault: 0000 [#1] SMP
> > Dumping ftrace buffer:
> > (ftrace buffer empty)
> > Modules linked in:
> > CPU: 1 PID: 26639 Comm: syz-executor5 Not tainted 4.15.0-rc3-next-20171214+
> > #67
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > Google 01/01/2011
> > RIP: 0010:find_entry.isra.14+0x45/0xc0 fs/proc/proc_sysctl.c:119
> > RSP: 0018:ffffc90000dcba60 EFLAGS: 00010282
> > RAX: 0000000000010000 RBX: ffff1003f68e0980 RCX: ffffffff814becad
> > RDX: 000000000000674d RSI: ffffc90000fe1000 RDI: ffffc90000dcbaa8
> > RBP: ffffc90000dcba98 R08: 0000000000000001 R09: 0000000000000004
> > R10: ffffc90000dcba48 R11: 0000000000000004 R12: ffff8801fb4704d0
> > R13: ffff8801fb470400 R14: ffff8801fc7ecc00 R15: ffffc90000dcbb7d
> > FS: 00007f125b750700(0000) GS:ffff88021fd00000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000000001680cd8 CR3: 00000001fd3ac001 CR4: 00000000001606e0
> > DR0: 0000000020001008 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
> > Call Trace:
> > find_subdir+0x35/0x80 fs/proc/proc_sysctl.c:923
> > get_subdir fs/proc/proc_sysctl.c:977 [inline]
> > __register_sysctl_table+0x392/0x7c0 fs/proc/proc_sysctl.c:1327
>
> find_subdir() accepts as part of its seconds argument a struct ctl_dir *dir,
> which we then find_subdir() then dereferences using find_entry():
>
> struct rb_node *node = dir->root.rb_node;
>
> The RIP however is on find_entry.isra.14+0x45/0xc0 fs/proc/proc_sysctl.c:119
> which as of linux-next is:
>
> entry = &head->ctl_table[ctl_node - head->node];
>
> So a few questions to review.
>
> 1) Is it sensible to assume we can always reference the dir?
> 2) Is the entry computation above really safe always?
>
> Let's try:
>
> 1) Is it sensible to assume we can always reference the dir?
>
> 1a) When registering a sysctl table register_sysctl() calls:
>
> return __register_sysctl_table(&sysctl_table_root.default_set,
> path, table);
>
> So the sysctl_table_root.default_set set is used. When registering
> then, the first dir used is this one. init_header() sets the
> set on the header to this one, and we use the same set for
> the first dir:
>
> dir = &set->dir;
>
> Whether or not this is safe will depend on the life cycle of this root entry's
> dir pointer, and at init sysctl_table_root.default_set.dir is just a pointer
> in static memory. It cannot be freed. So on the first iteration
> the dereference should always be safe.
>
> On the loop, since strchr() is used we iterate over the path given
> for the target proc directory entry by first looking at the first
> pattern with '/'.
>
> > register_net_sysctl+0x29/0x30 net/sysctl_net.c:120
> > neigh_sysctl_register+0x150/0x220 net/core/neighbour.c:3235
>
> In this particular case of addrconf_sysctl_register()
> the path is something along the lines of: "net/%s/neigh/%s"
>
> So if we had path as net/foo/neigh/bar we'd have a few iterations as
> follows:
>
> Path: net/foo/neigh/bar
> ------------------------------------
> name: net/foo/neigh/bar
> nextname: foo/neigh/bar
> ------------------------------------
> name: foo/neigh/bar
> nextname: neigh/bar
> ------------------------------------
> name: neigh/bar
> nextname: bar
> ------------------------------------
> name: bar
> nextname: (null)
>
> And for each one we'd call get_subdir() to ensure that
> the dir exists:
>
> dir = get_subdir(dir, name, namelen);
> if (IS_ERR(dir))
> goto fail;
>
> Its in these lookups where we hit an issue. Note that
> get_subdir() calls find_subdir() twice, the one we hit the
> issue with is the first call.
>
> The loop will keep setting up dir to traverse the entire path
> ensuring we have a dir entry up to the end of the path. Each
> sub directory must have an entry in order for this
> to work.
>
> The safety of relying on dereferencing dir will then depend on our locking
> sanity and management of the dirs here. I'm not so sure about the sanity
> on locking here yet.
>
> 2) Is the entry computation above really safe always?
> 2A) The find_entry() call and entry lookup we are concerned with which we see
> on RIP happens off of the get_subdir() calls on the loop inspected above. The
> trace above shows its on the get_subdir() call, and this trace:
>
> get_subdir fs/proc/proc_sysctl.c:977 [inline]
>
> Tells us its the first find_subdir() call within get_subdir() where the issue
> was hit. find_subdir() looks at the registered ctl table and looks for a
> matching directory, and if found returns it it, a struct ctl_dir pointer.
> From what I can gather this is safe so long as our locking is sane.
>
> Not sure yet what could be wrong.. being able to reproduce would help.
> I'd recommend trying to expand on lib/test_sysctl.c with more elaborate
> entanglements and trying to register/deregister as a tests a series of
> subdirectories which some depend entries depend on.
>
Hi Luis, this crash has only occurred once, and it was actually reported while
KASAN was accidentally disabled in the syzbot kconfig due to a change to the
kconfig menus in linux-next -- so the crash was possibly caused by slab
corruption elsewhere.
I will just invalidate the bug so that syzbot can report crashes with the same
signature again, but feel free to look into it more if you think there is still
a real problem somewhere.
#syz invalid
Thanks,
Eric
^ permalink raw reply [flat|nested] 2+ messages in thread