Re: general protection fault in find_entry

[Luis R. Rodriguez]

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.

  Luis