next-20170510 refcount_inc() on zero / use-after-free in key_lookup()

next-20170510 refcount_inc() on zero / use-after-free in key_lookup()

[Mark Rutland]

Hi,

While fuzzing next-20170510 on arm64 with Syzkaller, I hit a
refcount_inc() on a zero count in __key_get(), called from key_lookup().
The tree was dirty as I was testing with some new hardened string
functions, but AFAICT these are not to blame.

>From a quick look at key_lookup(), the following looks very suspicious:

found:
        /* pretend it doesn't exist if it is awaiting deletion */
        if (refcount_read(&key->usage) == 0)
                goto not_found;

        /* this races with key_put(), but that doesn't matter since key_put()
         * doesn't actually change the key
         */
        __key_get(key);

... as if we can race with key_put(), we can see a zero refcount here,
and the race *does* matter.

Full splat (and Syzkaller reproducer) below. See [1] for how to build
Syzkaller and run the reproducer.

I had to stick a udelay(10) between the refcount_read() and __key_get()
in order to reproduce this reliably on my machine, due to the small
window in key_lookup(). YMMV.

Thanks,
Mark.

[1] https://github.com/google/syzkaller/wiki/How-to-execute-syzkaller-programs

------------[ cut here ]------------
WARNING: CPU: 0 PID: 3438 at lib/refcount.c:150 refcount_inc+0x78/0xa0 lib/refcount.c:150
Kernel panic - not syncing: panic_on_warn set ...

CPU: 0 PID: 3438 Comm: syz-executor0 Not tainted 4.11.0-next-20170510-00001-g67f1150-dirty #6
Hardware name: linux,dummy-virt (DT)
Call trace:
[<ffff200008094bb8>] dump_backtrace+0x0/0x538 arch/arm64/kernel/traps.c:73
[<ffff200008095390>] show_stack+0x20/0x30 arch/arm64/kernel/traps.c:228
[<ffff200008c1bc68>] __dump_stack lib/dump_stack.c:16 [inline]
[<ffff200008c1bc68>] dump_stack+0x120/0x188 lib/dump_stack.c:52
[<ffff20000840bdd8>] panic+0x288/0x554 kernel/panic.c:180
[<ffff200008150f14>] __warn+0x25c/0x2b8 kernel/panic.c:541
[<ffff200008c1a54c>] report_bug+0x234/0x2e0 lib/bug.c:183
[<ffff2000080957d8>] bug_handler.part.1+0x40/0x110 arch/arm64/kernel/traps.c:725
[<ffff2000080958f4>] bug_handler+0x4c/0x88 arch/arm64/kernel/traps.c:722
[<ffff200008086bf8>] call_break_hook arch/arm64/kernel/debug-monitors.c:303 [inline]
[<ffff200008086bf8>] brk_handler+0x1b8/0x360 arch/arm64/kernel/debug-monitors.c:318
[<ffff200008081ad4>] do_debug_exception+0xfc/0x360 arch/arm64/mm/fault.c:683
Exception stack(0xffff80005eab7a40 to 0xffff80005eab7b70)
7a40: 0000000000000000 0001000000000000 ffff80005eab7c90 ffff200008c77808
7a60: 0000000080000145 000000000000003d ffff20000b878000 ffff20000b878e60
7a80: ffff20000b878da0 ffff8000655bb180 ffff80005eab7ba0 000000002000d000
7aa0: 0000000041b58ab3 ffff20000a90b450 ffff2000080819d8 ffff20000a2e70c0
7ac0: ffff800061f95e80 ffff20000b878e00 ffff20000b878000 ffff20000b878e60
7ae0: ffff80005eab7c90 ffff80005eab7c90 ffff80005eab7c50 00000000ffffffc8
7b00: 0000000041b58ab3 ffff20000a91dc28 ffff20000828c908 1fffe40001be8b32
7b20: 1ffff0000cab772d 1ffff0000cab772d ffff20000d705f80 ffff8000655bb948
7b40: ffff8000655bb948 1ffff0000cab7728 1ffff0000bd56f72 ffff8000655bb940
7b60: 000000000000002b ffff10000bd56f60
[<ffff200008083ea4>] el1_dbg+0x18/0x74
[<ffff200008acdef0>] __key_get include/linux/key.h:253 [inline]
[<ffff200008acdef0>] key_lookup+0x158/0x1c8 security/keys/key.c:670
[<ffff200008adb6c0>] lookup_user_key+0x128/0xdf8 security/keys/process_keys.c:680
[<ffff200008ad4f5c>] keyctl_update_key+0xf4/0x1b0 security/keys/keyctl.c:340
[<ffff200008ad965c>] SYSC_keyctl security/keys/keyctl.c:1647 [inline]
[<ffff200008ad965c>] SyS_keyctl+0x2ac/0x2e8 security/keys/keyctl.c:1635
[<ffff200008084770>] el0_svc_naked+0x24/0x28
SMP: stopping secondary CPUs
Kernel Offset: disabled
Memory Limit: none
Rebooting in 86400 seconds..


Syzkaller reproducer:
# {Threaded:true Collide:true Repeat:true Procs:1 Sandbox:setuid Repro:false}
mmap(&(0x7f0000000000/0xb000)=nil, (0xb000), 0x3, 0x32, 0xffffffffffffffff, 0x0)
r0 = add_key(&(0x7f0000003000-0x8)="6b657972696e6700", &(0x7f0000005000-0x5)={0x73, 0x79, 0x7a, 0x0, 0x0}, &(0x7f0000006000)="", 0x0, 0xffffffffffffffff)
r1 = request_key(&(0x7f0000006000-0x5)="6465616400", &(0x7f0000007000-0x5)={0x73, 0x79, 0x7a, 0x2, 0x0}, &(0x7f0000006000)="7472757374656465746831706f7369785f61636c5f6163636573736b657972696e675b47504c6264657600", 0xfffffffffffffffb)
keyctl$unlink(0x9, r0, r1)
socketpair(0x19, 0xa, 0x7, &(0x7f000000b000)={0xffffffffffffffff, 0xffffffffffffffff})
connect(0xffffffffffffffff, &(0x7f0000005000-0xc)=@nl={0x10, 0x0, 0x7, 0x1}, 0xc)
mmap(&(0x7f000000c000/0x1000)=nil, (0x1000), 0x3, 0x32, 0xffffffffffffffff, 0x0)
keyctl$read(0xb, r0, &(0x7f000000c000)="000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", 0x3c)
gettid()
mmap(&(0x7f000000d000/0x1000)=nil, (0x1000), 0x3, 0x32, 0xffffffffffffffff, 0x0)
getuid()
mmap(&(0x7f0000005000/0x4000)=nil, (0x4000), 0x3, 0x110, 0xffffffffffffffff, 0x0)
getresuid(&(0x7f000000d000)=0x0, &(0x7f000000d000)=0x0, &(0x7f000000e000-0x4)=0x0)
mmap(&(0x7f000000e000/0x1000)=nil, (0x1000), 0x3, 0x32, 0xffffffffffffffff, 0x0)
mmap(&(0x7f000000e000/0x1000)=nil, (0x1000), 0x3, 0x32, 0xffffffffffffffff, 0x0)
mmap(&(0x7f000000e000/0x1000)=nil, (0x1000), 0x3, 0x32, 0xffffffffffffffff, 0x0)
mmap(&(0x7f000000e000/0x1000)=nil, (0x1000), 0x3, 0x32, 0xffffffffffffffff, 0x0)
mmap(&(0x7f000000e000/0x1000)=nil, (0x1000), 0x3, 0x32, 0xffffffffffffffff, 0x0)
mmap(&(0x7f000000e000/0x1000)=nil, (0x1000), 0x3, 0x32, 0xffffffffffffffff, 0x0)
KEYCTl$update(0x2, r0, &(0x7f000000c000)="012c27e9d9b724cd535660d5c1a8618218e464709bb9f1f10488601c5b26ac08f9ea4223cb59259a83d344a32abe11dae8aa62a385044890f9eaf5", 0x3b)

Re: next-20170510 refcount_inc() on zero / use-after-free in key_lookup()

[David Howells]

Mark Rutland <mark.rutland@arm.com> wrote:

> From a quick look at key_lookup(), the following looks very suspicious:
> 
> found:
>         /* pretend it doesn't exist if it is awaiting deletion */
>         if (refcount_read(&key->usage) == 0)
>                 goto not_found;
> 
>         /* this races with key_put(), but that doesn't matter since key_put()
>          * doesn't actually change the key
>          */
>         __key_get(key);
> 
> ... as if we can race with key_put(), we can see a zero refcount here,
> and the race *does* matter.

No, it doesn't.

If key_put() reduces a refcount to 0, it doesn't do anything other than poke
the gc thread:

	void key_put(struct key *key)
	{
		if (key) {
			key_check(key);

			if (refcount_dec_and_test(&key->usage))
				schedule_work(&key_gc_work);
		}
	}

in particular, no indication of the reduced key is passed.

The gc thread scans the entire key serial tree under the key_serial_lock
looking for keys that are no longer ref'd.  No one else is allowed to remove
keys from the tree.  This means that the gc thread can safely leave a cursor
pointing into the midst of the tree with no locks held whilst it yields to the
scheduler.

The code you quoted above in key_lookup() is inside the key_serial_lock, so it
prevents the gc thread from culling a key when it resurrects it.

So the problem isn't the key code, it's the refcount code.

As I've said before, the refcount code needs an increment op that permits
inc-from-0.  In this case, it's perfectly okay.

David

Re: next-20170510 refcount_inc() on zero / use-after-free in key_lookup()

[Mark Rutland]

On Fri, May 12, 2017 at 04:29:39PM +0100, David Howells wrote:
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > From a quick look at key_lookup(), the following looks very suspicious:
> > 
> > found:
> >         /* pretend it doesn't exist if it is awaiting deletion */
> >         if (refcount_read(&key->usage) == 0)
> >                 goto not_found;
> > 
> >         /* this races with key_put(), but that doesn't matter since key_put()
> >          * doesn't actually change the key
> >          */
> >         __key_get(key);
> > 
> > ... as if we can race with key_put(), we can see a zero refcount here,
> > and the race *does* matter.
> 
> No, it doesn't.
> 
> If key_put() reduces a refcount to 0, it doesn't do anything other than poke
> the gc thread:
> 
> 	void key_put(struct key *key)
> 	{
> 		if (key) {
> 			key_check(key);
> 
> 			if (refcount_dec_and_test(&key->usage))
> 				schedule_work(&key_gc_work);
> 		}
> 	}
> 
> in particular, no indication of the reduced key is passed.
> 
> The gc thread scans the entire key serial tree under the key_serial_lock
> looking for keys that are no longer ref'd.  No one else is allowed to remove
> keys from the tree.  This means that the gc thread can safely leave a cursor
> pointing into the midst of the tree with no locks held whilst it yields to the
> scheduler.
> 
> The code you quoted above in key_lookup() is inside the key_serial_lock, so it
> prevents the gc thread from culling a key when it resurrects it.
> 
> So the problem isn't the key code, it's the refcount code.

Sure, there's no actual use-after-free here.

Sorry for the misleading title.

> As I've said before, the refcount code needs an increment op that permits
> inc-from-0.  In this case, it's perfectly okay.

Given that there's currently an attempt to bail out on a zero refcount courtesy
of the refcount_read(), can't we do something like the below?

Thanks,
Mark.

diff --git a/include/linux/key.h b/include/linux/key.h
index 78e25aa..1e68ae2 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -248,6 +248,14 @@ extern struct key *key_alloc(struct key_type *type,
 extern void key_invalidate(struct key *key);
 extern void key_put(struct key *key);
 
+static inline struct key *__key_get_notzero(struct key *key)
+{
+       if (refcount_inc_not_zero(&key->usage))
+               return key;
+
+       return NULL;
+}
+
 static inline struct key *__key_get(struct key *key)
 {
        refcount_inc(&key->usage);
diff --git a/security/keys/key.c b/security/keys/key.c
index 455c04d..f375cc6 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -661,14 +661,9 @@ struct key *key_lookup(key_serial_t id)
 
 found:
        /* pretend it doesn't exist if it is awaiting deletion */
-       if (refcount_read(&key->usage) == 0)
+       if (!__key_get_notzero(key))
                goto not_found;
 
-       /* this races with key_put(), but that doesn't matter since key_put()
-        * doesn't actually change the key
-        */
-       __key_get(key);
-
 error:
        spin_unlock(&key_serial_lock);
        return key;