All of lore.kernel.org
 help / color / mirror / Atom feed
* GPF in keyring_destroy
@ 2015-10-12  8:40 Dmitry Vyukov
  2015-10-15 15:29 ` David Howells
  2015-10-15 19:21 ` David Howells
  0 siblings, 2 replies; 10+ messages in thread
From: Dmitry Vyukov @ 2015-10-12  8:40 UTC (permalink / raw)
  To: dhowells, james.l.morris, serge, keyrings, linux-security-module, LKML
  Cc: syzkaller, Kostya Serebryany, Alexander Potapenko,
	Andrey Konovalov, Sasha Levin, Julien Tinnes, Kees Cook

Hello,

The following program crashes kernel:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <syscall.h>
#include <string.h>
#include <stdint.h>

int main()
{
        long r0 = syscall(SYS_mmap, 0x20001000ul, 0x1000ul, 0x3ul,
0x32ul, 0xfffffffffffffffful, 0x0ul);
        long r1 = syscall(SYS_mmap, 0x20000000ul, 0x1000ul, 0x3ul,
0x32ul, 0xfffffffffffffffful, 0x0ul);
        *(uint64_t*)0x20000131 = 0x947;
        *(uint64_t*)0x20000139 = 0xa9be;
        *(uint64_t*)0x20000141 = 0x4;
        *(uint64_t*)0x20000149 = 0xfffffffffffffff8;
        *(uint64_t*)0x20000151 = 0x3;
        *(uint64_t*)0x20000159 = 0x1;
        *(uint64_t*)0x20000161 = 0x7;
        *(uint64_t*)0x20000169 = 0x3b;
        long r11 = syscall(SYS_mmap, 0x20002000ul, 0x1000ul, 0x3ul,
0x32ul, 0xfffffffffffffffful, 0x0ul);
        memcpy((void*)0x20002000,
"\x5d\x27\x7b\x65\x63\x72\x79\x70\x74\x66\x73\x5c\x73\x65\x6c\x69\x6e\x75\x78\x6e\x66\x73\x00",
23);
        memcpy((void*)0x20001ff4,
"\x23\x73\x65\x6c\x69\x6e\x75\x78\x2c\x62\x64\x65\x76\x72\x61\x6d\x66\x73\xe6\x68\x66\x73\x70\x6c\x75\x73\x7b\x2c\x28\x72\x6f\x6f\x74\x66\x73\x00",
36);
        memcpy((void*)0x20002000, "\x6b\x65\x79\x72\x69\x6e\x67\x00", 8);
        long r16 = syscall(SYS_request_key, 0x20002000ul,
0x20001ff4ul, 0x20002000ul, 0xfffffffffffffffbul, 0, 0);
        memcpy((void*)0x20002000,
"\x5d\x27\x7b\x65\x63\x72\x79\x70\x74\x66\x73\x5c\x73\x65\x6c\x69\x6e\x75\x78\x6e\x66\x73\x00",
23);
        memcpy((void*)0x20001ff4,
"\x23\x73\x65\x6c\x69\x6e\x75\x78\x2c\x62\x64\x65\x76\x72\x61\x6d\x66\x73\xe6\x68\x66\x73\x70\x6c\x75\x73\x7b\x2c\x28\x72\x6f\x6f\x74\x66\x73\x00",
36);
        memcpy((void*)0x20002000, "\x6b\x65\x79\x72\x69\x6e\x67\x00", 8);
        long r20 = syscall(SYS_request_key, 0x20002000ul,
0x20001ff4ul, 0x20002000ul, 0xfffffffffffffffbul, 0, 0);
        memcpy((void*)0x20002000,
"\x5d\x27\x7b\x65\x63\x72\x79\x70\x74\x66\x73\x5c\x73\x65\x6c\x69\x6e\x75\x78\x6e\x66\x73\x00",
23);
        memcpy((void*)0x20001ff4,
"\x23\x73\x65\x6c\x69\x6e\x75\x78\x2c\x62\x64\x65\x76\x72\x61\x6d\x66\x73\xe6\x68\x66\x73\x70\x6c\x75\x73\x7b\x2c\x28\x72\x6f\x6f\x74\x66\x73\x00",
36);
        memcpy((void*)0x20002000, "\x6b\x65\x79\x72\x69\x6e\x67\x00", 8);
        long r24 = syscall(SYS_request_key, 0x20002000ul,
0x20001ff4ul, 0x20002000ul, 0xfffffffffffffffbul, 0, 0);
        return 0;
}

To reproduce run the program several times, then wait a minute, run
again, repeat until crashes (the crash happens in a background thread
during deferred garbage collection).

Found with syzkaller fuzzer.

On commit c6fa8e6de3dc420cba092bf155b2ed25bcd537f7
(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)


BUG: unable to handle kernel paging request at 00000000ffffff8a
IP: [<     inline     >] list_del include/linux/list.h:107
IP: [<ffffffff81291add>] keyring_destroy+0x3d/0x80 security/keys/keyring.c:392
PGD 0
Oops: 0002 [#1] SMP KASAN
Modules linked in:
CPU: 2 PID: 505 Comm: kworker/2:1 Not tainted 4.3.0-rc4+ #47
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: events key_garbage_collector
task: ffff88006dd93400 ti: ffff88006dd68000 task.ti: ffff88006dd68000
RIP: 0010:[<ffffffff81291add>]  [<ffffffff81291add>] keyring_destroy+0x3d/0x80
RSP: 0018:ffff88006dd6fda8  EFLAGS: 00010213
RAX: 00000000ffffff82 RBX: ffff88006d73bc00 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff88006dd93400 RDI: ffffffff821077a0
RBP: ffff88006dd6fdb0 R08: ffff88006dd68000 R09: ffff88006dd93d40
R10: 000000000000072c R11: 0000000000000001 R12: ffff88006d73bc00
R13: 7fffffffffffffff R14: ffff88006dd70000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff88006e800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00000000ffffff8a CR3: 000000006cda6000 CR4: 00000000000006e0
Stack:
 ffff88006d73bc08 ffff88006dd6fdd0 ffffffff81290131 0000000000000000
 0000000056177c32 ffff88006dd6fe18 ffffffff81290391 ffff88006e626d28
 0000000000000000 ffffffff81e812a0 ffff88006e746080 ffff88006e81d8c0
Call Trace:
 [<ffffffff81290131>] key_gc_unused_keys.constprop.1+0xa1/0xf0
security/keys/gc.c:139
 [<ffffffff81290391>] key_garbage_collector+0x1a1/0x360 security/keys/gc.c:294
 [<ffffffff8106630e>] process_one_work+0x13e/0x3c0 kernel/workqueue.c:2030
 [<ffffffff810666a5>] worker_thread+0x115/0x450 kernel/workqueue.c:2162
 [<     inline     >] ? context_switch kernel/sched/core.c:2651
 [<ffffffff81838e21>] ? __schedule+0x311/0x7e0 kernel/sched/core.c:3115
 [<ffffffff81066590>] ? process_one_work+0x3c0/0x3c0
kernel/workqueue.c:1973 (discriminator 1)
 [<ffffffff8106b4d4>] kthread+0xc4/0xe0 kernel/kthread.c:209
 [<ffffffff8106b410>] ? kthread_park+0x50/0x50 kernel/kthread.c:448
 [<ffffffff8183c9af>] ret_from_fork+0x3f/0x70 arch/x86/entry/entry_64.S:472
 [<ffffffff8106b410>] ? kthread_park+0x50/0x50 kernel/kthread.c:448
Code: 48 c7 c7 a0 77 10 82 e8 f2 a8 5a 00 48 8b 83 98 00 00 00 48 85
c0 74 36 48 8d 93 98 00 00 00 48 39 d0 74 2a 48 8b 93 a0 00 00 00 <48>
89 50 08 48 89 02 48 b8 00 01 00 00 00 00 ad de 48 89 83 98
RIP  [<     inline     >] list_del include/linux/list.h:107
RIP  [<ffffffff81291add>] keyring_destroy+0x3d/0x80 security/keys/keyring.c:392
 RSP <ffff88006dd6fda8>
CR2: 00000000ffffff8a
---[ end trace 92e3ff9d26bc918d ]---
BUG: unable to handle kernel paging request at ffffffffffffffd8
IP: [<ffffffff8106bbcb>] kthread_data+0xb/0x20 kernel/kthread.c:136
PGD 1e11067 PUD 1e13067 PMD 0
Oops: 0000 [#2] SMP KASAN
Modules linked in:
CPU: 2 PID: 505 Comm: kworker/2:1 Tainted: G      D         4.3.0-rc4+ #47
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: ffff88006dd93400 ti: ffff88006dd68000 task.ti: ffff88006dd68000
RIP: 0010:[<ffffffff8106bbcb>]  [<ffffffff8106bbcb>] kthread_data+0xb/0x20
RSP: 0018:ffff88006dd6fac8  EFLAGS: 00010002
RAX: 0000000000000000 RBX: 0000000000000002 RCX: 0000000000000002
RDX: ffff88003e810000 RSI: 0000000000000002 RDI: ffff88006dd93400
RBP: ffff88006dd6fac8 R08: 0000001d99efcfc6 R09: 0000000000000001
R10: ffff88003ded5f80 R11: 000000000000001a R12: ffff88006dd93400
R13: 000000000001e0c0 R14: 0000000000000002 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff88006e800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000028 CR3: 000000006cda6000 CR4: 00000000000006e0
Stack:
 ffff88006dd6fae0 ffffffff81066e8c ffff88006e81e0c0 ffff88006dd6fb28
 ffffffff81839041 ffff88006d6b97b0 ffff88006dd93400 ffff88006dd70000
 ffff88006dd93820 0000000000000046 ffff88006dd93400 ffff88003eaf0000
Call Trace:
 [<ffffffff81066e8c>] wq_worker_sleeping+0xc/0x80 kernel/workqueue.c:847
 [<ffffffff81839041>] __schedule+0x531/0x7e0 kernel/sched/core.c:3094
 [<ffffffff8183931e>] schedule+0x2e/0x70 kernel/sched/core.c:3144
(discriminator 1)
 [<ffffffff810529b3>] do_exit+0x693/0xb50 kernel/exit.c:824
 [<ffffffff81007564>] oops_end+0x84/0xc0 arch/x86/kernel/dumpstack.c:250
 [<ffffffff810443c4>] no_context+0x104/0x350 arch/x86/mm/fault.c:728
 [<     inline     >] ? context_switch kernel/sched/core.c:2651
 [<ffffffff81838e21>] ? __schedule+0x311/0x7e0 kernel/sched/core.c:3115
 [<ffffffff8104470e>] __bad_area_nosemaphore+0xfe/0x200 arch/x86/mm/fault.c:808
 [<ffffffff8104481e>] bad_area_nosemaphore+0xe/0x10 arch/x86/mm/fault.c:815
 [<ffffffff8104489c>] __do_page_fault+0x7c/0x3e0 arch/x86/mm/fault.c:1180
 [<ffffffff81085480>] ? __wake_up_common+0x50/0x80 kernel/sched/wait.c:73
 [<ffffffff81044c0c>] do_page_fault+0xc/0x10 arch/x86/mm/fault.c:1301
 [<ffffffff8183e422>] page_fault+0x22/0x30 arch/x86/entry/entry_64.S:979
 [<     inline     >] ? list_del include/linux/list.h:107
 [<ffffffff81291add>] ? keyring_destroy+0x3d/0x80 security/keys/keyring.c:392
 [<ffffffff81291abe>] ? keyring_destroy+0x1e/0x80 security/keys/keyring.c:388
 [<ffffffff81290131>] key_gc_unused_keys.constprop.1+0xa1/0xf0
security/keys/gc.c:139
 [<ffffffff81290391>] key_garbage_collector+0x1a1/0x360 security/keys/gc.c:294
 [<ffffffff8106630e>] process_one_work+0x13e/0x3c0 kernel/workqueue.c:2030
 [<ffffffff810666a5>] worker_thread+0x115/0x450 kernel/workqueue.c:2162
 [<     inline     >] ? context_switch kernel/sched/core.c:2651
 [<ffffffff81838e21>] ? __schedule+0x311/0x7e0 kernel/sched/core.c:3115
 [<ffffffff81066590>] ? process_one_work+0x3c0/0x3c0
kernel/workqueue.c:1973 (discriminator 1)
 [<ffffffff8106b4d4>] kthread+0xc4/0xe0 kernel/kthread.c:209
 [<ffffffff8106b410>] ? kthread_park+0x50/0x50 kernel/kthread.c:448
 [<ffffffff8183c9af>] ret_from_fork+0x3f/0x70 arch/x86/entry/entry_64.S:472
 [<ffffffff8106b410>] ? kthread_park+0x50/0x50 kernel/kthread.c:448
Code: 00 00 48 8d 04 c5 48 86 a0 81 48 89 e5 48 29 f0 48 89 c6 e8 48
fa ff ff 5d c3 66 0f 1f 44 00 00 55 48 8b 87 d0 04 00 00 48 89 e5 <48>
8b 40 d8 5d c3 66 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00
RIP  [<ffffffff8106bbcb>] kthread_data+0xb/0x20 kernel/kthread.c:136
 RSP <ffff88006dd6fac8>
CR2: ffffffffffffffd8
---[ end trace 92e3ff9d26bc918e ]---
Fixing recursive fault but reboot is needed!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: GPF in keyring_destroy
  2015-10-12  8:40 GPF in keyring_destroy Dmitry Vyukov
@ 2015-10-15 15:29 ` David Howells
  2015-10-15 19:21 ` David Howells
  1 sibling, 0 replies; 10+ messages in thread
From: David Howells @ 2015-10-15 15:29 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: dhowells, james.l.morris, serge, keyrings, linux-security-module,
	LKML, syzkaller, Kostya Serebryany, Alexander Potapenko,
	Andrey Konovalov, Sasha Levin, Julien Tinnes, Kees Cook

Dmitry Vyukov <dvyukov@google.com> wrote:

> RAX: 00000000ffffff82

This is the value that matters.  It would appear to be -ENOKEY and would be in
key->type_data.reject_error, I think.

David

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: GPF in keyring_destroy
  2015-10-12  8:40 GPF in keyring_destroy Dmitry Vyukov
  2015-10-15 15:29 ` David Howells
@ 2015-10-15 19:21 ` David Howells
  2015-10-19  8:26   ` Dmitry Vyukov
                     ` (2 more replies)
  1 sibling, 3 replies; 10+ messages in thread
From: David Howells @ 2015-10-15 19:21 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: dhowells, james.l.morris, serge, keyrings, linux-security-module,
	LKML, syzkaller, Kostya Serebryany, Alexander Potapenko,
	Andrey Konovalov, Sasha Levin, Julien Tinnes, Kees Cook

Does the attached patch fix it for you?

David
---
commit a7609e0bb3973d6ee3c9f1ecd0b6a382d99d6248
Author: David Howells <dhowells@redhat.com>
Date:   Thu Oct 15 17:21:37 2015 +0100

    KEYS: Fix crash when attempt to garbage collect an uninstantiated keyring
    
    The following sequence of commands:
    
        i=`keyctl add user a a @s`
        keyctl request2 keyring foo bar @t
        keyctl unlink $i @s
    
    tries to invoke an upcall to instantiate a keyring if one doesn't already
    exist by that name within the user's keyring set.  However, if the upcall
    fails, the code sets keyring->type_data.reject_error to -ENOKEY or some
    other error code.  When the key is garbage collected, the key destroy
    function is called unconditionally and keyring_destroy() uses list_empty()
    on keyring->type_data.link - which is in a union with reject_error.
    Subsequently, the kernel tries to unlink the keyring from the keyring names
    list - which oopses like this:
    
    	BUG: unable to handle kernel paging request at 00000000ffffff8a
    	IP: [<ffffffff8126e051>] keyring_destroy+0x3d/0x88
    	...
    	Workqueue: events key_garbage_collector
    	...
    	RIP: 0010:[<ffffffff8126e051>] keyring_destroy+0x3d/0x88
    	RSP: 0018:ffff88003e2f3d30  EFLAGS: 00010203
    	RAX: 00000000ffffff82 RBX: ffff88003bf1a900 RCX: 0000000000000000
    	RDX: 0000000000000000 RSI: 000000003bfc6901 RDI: ffffffff81a73a40
    	RBP: ffff88003e2f3d38 R08: 0000000000000152 R09: 0000000000000000
    	R10: ffff88003e2f3c18 R11: 000000000000865b R12: ffff88003bf1a900
    	R13: 0000000000000000 R14: ffff88003bf1a908 R15: ffff88003e2f4000
    	...
    	CR2: 00000000ffffff8a CR3: 000000003e3ec000 CR4: 00000000000006f0
    	...
    	Call Trace:
    	 [<ffffffff8126c756>] key_gc_unused_keys.constprop.1+0x5d/0x10f
    	 [<ffffffff8126ca71>] key_garbage_collector+0x1fa/0x351
    	 [<ffffffff8105ec9b>] process_one_work+0x28e/0x547
    	 [<ffffffff8105fd17>] worker_thread+0x26e/0x361
    	 [<ffffffff8105faa9>] ? rescuer_thread+0x2a8/0x2a8
    	 [<ffffffff810648ad>] kthread+0xf3/0xfb
    	 [<ffffffff810647ba>] ? kthread_create_on_node+0x1c2/0x1c2
    	 [<ffffffff815f2ccf>] ret_from_fork+0x3f/0x70
    	 [<ffffffff810647ba>] ? kthread_create_on_node+0x1c2/0x1c2
    
    Note the value in RAX.  This is a 32-bit representation of -ENOKEY.
    
    The solution is to only call ->destroy() if the key was successfully
    instantiated.
    
    Reported-by: Dmitry Vyukov <dvyukov@google.com>
    Signed-off-by: David Howells <dhowells@redhat.com>

diff --git a/security/keys/gc.c b/security/keys/gc.c
index 39eac1fd5706..addf060399e0 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -134,8 +134,10 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 		kdebug("- %u", key->serial);
 		key_check(key);
 
-		/* Throw away the key data */
-		if (key->type->destroy)
+		/* Throw away the key data if the key is instantiated */
+		if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
+		    !test_bit(KEY_FLAG_NEGATIVE, &key->flags) &&
+		    key->type->destroy)
 			key->type->destroy(key);
 
 		security_key_free(key);

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: GPF in keyring_destroy
  2015-10-15 19:21 ` David Howells
@ 2015-10-19  8:26   ` Dmitry Vyukov
  2015-10-19  9:30   ` David Howells
  2015-10-19 10:33   ` David Howells
  2 siblings, 0 replies; 10+ messages in thread
From: Dmitry Vyukov @ 2015-10-19  8:26 UTC (permalink / raw)
  To: David Howells
  Cc: james.l.morris, serge, keyrings, linux-security-module, LKML,
	syzkaller, Kostya Serebryany, Alexander Potapenko,
	Andrey Konovalov, Sasha Levin, Julien Tinnes, Kees Cook

On Thu, Oct 15, 2015 at 9:21 PM, David Howells <dhowells@redhat.com> wrote:
> Does the attached patch fix it for you?

Yes, it fixes the crash for me.


> David
> ---
> commit a7609e0bb3973d6ee3c9f1ecd0b6a382d99d6248
> Author: David Howells <dhowells@redhat.com>
> Date:   Thu Oct 15 17:21:37 2015 +0100
>
>     KEYS: Fix crash when attempt to garbage collect an uninstantiated keyring
>
>     The following sequence of commands:
>
>         i=`keyctl add user a a @s`
>         keyctl request2 keyring foo bar @t
>         keyctl unlink $i @s
>
>     tries to invoke an upcall to instantiate a keyring if one doesn't already
>     exist by that name within the user's keyring set.  However, if the upcall
>     fails, the code sets keyring->type_data.reject_error to -ENOKEY or some
>     other error code.  When the key is garbage collected, the key destroy
>     function is called unconditionally and keyring_destroy() uses list_empty()
>     on keyring->type_data.link - which is in a union with reject_error.
>     Subsequently, the kernel tries to unlink the keyring from the keyring names
>     list - which oopses like this:
>
>         BUG: unable to handle kernel paging request at 00000000ffffff8a
>         IP: [<ffffffff8126e051>] keyring_destroy+0x3d/0x88
>         ...
>         Workqueue: events key_garbage_collector
>         ...
>         RIP: 0010:[<ffffffff8126e051>] keyring_destroy+0x3d/0x88
>         RSP: 0018:ffff88003e2f3d30  EFLAGS: 00010203
>         RAX: 00000000ffffff82 RBX: ffff88003bf1a900 RCX: 0000000000000000
>         RDX: 0000000000000000 RSI: 000000003bfc6901 RDI: ffffffff81a73a40
>         RBP: ffff88003e2f3d38 R08: 0000000000000152 R09: 0000000000000000
>         R10: ffff88003e2f3c18 R11: 000000000000865b R12: ffff88003bf1a900
>         R13: 0000000000000000 R14: ffff88003bf1a908 R15: ffff88003e2f4000
>         ...
>         CR2: 00000000ffffff8a CR3: 000000003e3ec000 CR4: 00000000000006f0
>         ...
>         Call Trace:
>          [<ffffffff8126c756>] key_gc_unused_keys.constprop.1+0x5d/0x10f
>          [<ffffffff8126ca71>] key_garbage_collector+0x1fa/0x351
>          [<ffffffff8105ec9b>] process_one_work+0x28e/0x547
>          [<ffffffff8105fd17>] worker_thread+0x26e/0x361
>          [<ffffffff8105faa9>] ? rescuer_thread+0x2a8/0x2a8
>          [<ffffffff810648ad>] kthread+0xf3/0xfb
>          [<ffffffff810647ba>] ? kthread_create_on_node+0x1c2/0x1c2
>          [<ffffffff815f2ccf>] ret_from_fork+0x3f/0x70
>          [<ffffffff810647ba>] ? kthread_create_on_node+0x1c2/0x1c2
>
>     Note the value in RAX.  This is a 32-bit representation of -ENOKEY.
>
>     The solution is to only call ->destroy() if the key was successfully
>     instantiated.
>
>     Reported-by: Dmitry Vyukov <dvyukov@google.com>
>     Signed-off-by: David Howells <dhowells@redhat.com>
>
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 39eac1fd5706..addf060399e0 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -134,8 +134,10 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
>                 kdebug("- %u", key->serial);
>                 key_check(key);
>
> -               /* Throw away the key data */
> -               if (key->type->destroy)
> +               /* Throw away the key data if the key is instantiated */
> +               if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
> +                   !test_bit(KEY_FLAG_NEGATIVE, &key->flags) &&
> +                   key->type->destroy)
>                         key->type->destroy(key);
>
>                 security_key_free(key);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: GPF in keyring_destroy
  2015-10-15 19:21 ` David Howells
  2015-10-19  8:26   ` Dmitry Vyukov
@ 2015-10-19  9:30   ` David Howells
  2015-10-19  9:31     ` Dmitry Vyukov
  2015-10-19 10:25     ` David Howells
  2015-10-19 10:33   ` David Howells
  2 siblings, 2 replies; 10+ messages in thread
From: David Howells @ 2015-10-19  9:30 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: dhowells, james.l.morris, serge, keyrings, linux-security-module,
	LKML, syzkaller, Kostya Serebryany, Alexander Potapenko,
	Andrey Konovalov, Sasha Levin, Julien Tinnes, Kees Cook

Dmitry Vyukov <dvyukov@google.com> wrote:

> > Does the attached patch fix it for you?
> 
> Yes, it fixes the crash for me.

Can I put you down as a Tested-by?

David

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: GPF in keyring_destroy
  2015-10-19  9:30   ` David Howells
@ 2015-10-19  9:31     ` Dmitry Vyukov
  2015-10-19 10:25     ` David Howells
  1 sibling, 0 replies; 10+ messages in thread
From: Dmitry Vyukov @ 2015-10-19  9:31 UTC (permalink / raw)
  To: syzkaller
  Cc: David Howells, james.l.morris, serge, keyrings,
	linux-security-module, LKML, Kostya Serebryany,
	Alexander Potapenko, Andrey Konovalov, Sasha Levin,
	Julien Tinnes, Kees Cook

On Mon, Oct 19, 2015 at 11:30 AM, David Howells <dhowells@redhat.com> wrote:
> Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> > Does the attached patch fix it for you?
>>
>> Yes, it fixes the crash for me.
>
> Can I put you down as a Tested-by?
>
> David


Yes, sure. Do I need to say something like:

Tested-by: Dmitry Vyukov <dvyukov@google.com>

in future?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: GPF in keyring_destroy
  2015-10-19  9:30   ` David Howells
  2015-10-19  9:31     ` Dmitry Vyukov
@ 2015-10-19 10:25     ` David Howells
  1 sibling, 0 replies; 10+ messages in thread
From: David Howells @ 2015-10-19 10:25 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: dhowells, syzkaller, james.l.morris, serge, keyrings,
	linux-security-module, LKML, Kostya Serebryany,
	Alexander Potapenko, Andrey Konovalov, Sasha Levin,
	Julien Tinnes, Kees Cook

Dmitry Vyukov <dvyukov@google.com> wrote:

> Yes, sure. Do I need to say something like:
> 
> Tested-by: Dmitry Vyukov <dvyukov@google.com>
> 
> in future?

That helps:-)

David

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: GPF in keyring_destroy
  2015-10-15 19:21 ` David Howells
  2015-10-19  8:26   ` Dmitry Vyukov
  2015-10-19  9:30   ` David Howells
@ 2015-10-19 10:33   ` David Howells
  2015-10-19 10:41     ` Dmitry Vyukov
  2015-10-19 10:55     ` David Howells
  2 siblings, 2 replies; 10+ messages in thread
From: David Howells @ 2015-10-19 10:33 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: dhowells, james.l.morris, serge, keyrings, linux-security-module,
	LKML, syzkaller, Kostya Serebryany, Alexander Potapenko,
	Andrey Konovalov, Sasha Levin, Julien Tinnes, Kees Cook

Dmitry Vyukov <dvyukov@google.com> wrote:

> > Does the attached patch fix it for you?
> 
> Yes, it fixes the crash for me.

I have an additional patch to prevent keyrings from being constructed by
request_key() at all (though it can still search for them).  Could you give
this a spin in addition to the previous one also?

Thanks,
David
---
commit 27874345bb8d2c39f3d493607a86ecbfcb100405
Author: David Howells <dhowells@redhat.com>
Date:   Mon Oct 19 11:20:28 2015 +0100

    KEYS: Don't permit request_key() to construct a new keyring
    
    If request_key() is used to find a keyring, only do the search part - don't
    do the construction part if the keyring was not found by the search.  We
    don't really want keyrings in the negative instantiated state since the
    rejected/negative instantiation error value in the payload is unioned with
    keyring metadata.
    
    Signed-off-by: David Howells <dhowells@redhat.com>

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 486ef6fa393b..0d6253124278 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -440,6 +440,9 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
 
 	kenter("");
 
+	if (ctx->index_key.type == &key_type_keyring)
+		return ERR_PTR(-EPERM);
+	
 	user = key_user_lookup(current_fsuid());
 	if (!user)
 		return ERR_PTR(-ENOMEM);

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: GPF in keyring_destroy
  2015-10-19 10:33   ` David Howells
@ 2015-10-19 10:41     ` Dmitry Vyukov
  2015-10-19 10:55     ` David Howells
  1 sibling, 0 replies; 10+ messages in thread
From: Dmitry Vyukov @ 2015-10-19 10:41 UTC (permalink / raw)
  To: syzkaller
  Cc: David Howells, james.l.morris, serge, keyrings,
	linux-security-module, LKML, Kostya Serebryany,
	Alexander Potapenko, Andrey Konovalov, Sasha Levin,
	Julien Tinnes, Kees Cook

On Mon, Oct 19, 2015 at 12:33 PM, David Howells <dhowells@redhat.com> wrote:
> Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> > Does the attached patch fix it for you?
>>
>> Yes, it fixes the crash for me.
>
> I have an additional patch to prevent keyrings from being constructed by
> request_key() at all (though it can still search for them).  Could you give
> this a spin in addition to the previous one also?

Do you mean in addition or instead of the previous one? From your
description, it sounds like it alone should prevent the crash.


> Thanks,
> David
> ---
> commit 27874345bb8d2c39f3d493607a86ecbfcb100405
> Author: David Howells <dhowells@redhat.com>
> Date:   Mon Oct 19 11:20:28 2015 +0100
>
>     KEYS: Don't permit request_key() to construct a new keyring
>
>     If request_key() is used to find a keyring, only do the search part - don't
>     do the construction part if the keyring was not found by the search.  We
>     don't really want keyrings in the negative instantiated state since the
>     rejected/negative instantiation error value in the payload is unioned with
>     keyring metadata.
>
>     Signed-off-by: David Howells <dhowells@redhat.com>
>
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index 486ef6fa393b..0d6253124278 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -440,6 +440,9 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
>
>         kenter("");
>
> +       if (ctx->index_key.type == &key_type_keyring)
> +               return ERR_PTR(-EPERM);
> +
>         user = key_user_lookup(current_fsuid());
>         if (!user)
>                 return ERR_PTR(-ENOMEM);
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> To post to this group, send email to syzkaller@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/17443.1445250818%40warthog.procyon.org.uk.
> For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: GPF in keyring_destroy
  2015-10-19 10:33   ` David Howells
  2015-10-19 10:41     ` Dmitry Vyukov
@ 2015-10-19 10:55     ` David Howells
  1 sibling, 0 replies; 10+ messages in thread
From: David Howells @ 2015-10-19 10:55 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: dhowells, syzkaller, james.l.morris, serge, keyrings,
	linux-security-module, LKML, Kostya Serebryany,
	Alexander Potapenko, Andrey Konovalov, Sasha Levin,
	Julien Tinnes, Kees Cook

Dmitry Vyukov <dvyukov@google.com> wrote:

> Do you mean in addition or instead of the previous one? From your
> description, it sounds like it alone should prevent the crash.

I'm going to submit them both, so if you could test them together.  You're
right, though, I think this should also prevent the crash.

David

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-10-19 10:55 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-12  8:40 GPF in keyring_destroy Dmitry Vyukov
2015-10-15 15:29 ` David Howells
2015-10-15 19:21 ` David Howells
2015-10-19  8:26   ` Dmitry Vyukov
2015-10-19  9:30   ` David Howells
2015-10-19  9:31     ` Dmitry Vyukov
2015-10-19 10:25     ` David Howells
2015-10-19 10:33   ` David Howells
2015-10-19 10:41     ` Dmitry Vyukov
2015-10-19 10:55     ` David Howells

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.