linux-stable.git/security/keys, branch v3.12.52

KEYS: Fix crash when attempt to garbage collect an uninstantiated keyring

2015-11-18T09:27:26+00:00

commit f05819df10d7b09f6d1eb6f8534a8f68e5a4fe61 upstream.

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: [] keyring_destroy+0x3d/0x88
	...
	Workqueue: events key_garbage_collector
	...
	RIP: 0010:[] 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:
	 [] key_gc_unused_keys.constprop.1+0x5d/0x10f
	 [] key_garbage_collector+0x1fa/0x351
	 [] process_one_work+0x28e/0x547
	 [] worker_thread+0x26e/0x361
	 [] ? rescuer_thread+0x2a8/0x2a8
	 [] kthread+0xf3/0xfb
	 [] ? kthread_create_on_node+0x1c2/0x1c2
	 [] ret_from_fork+0x3f/0x70
	 [] ? 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 
Signed-off-by: David Howells 
Tested-by: Dmitry Vyukov 
Signed-off-by: Jiri Slaby

KEYS: Fix race between key destruction and finding a keyring by name

2015-11-18T09:27:20+00:00

commit 94c4554ba07adbdde396748ee7ae01e86cf2d8d7 upstream.

There appears to be a race between:

 (1) key_gc_unused_keys() which frees key->security and then calls
     keyring_destroy() to unlink the name from the name list

 (2) find_keyring_by_name() which calls key_permission(), thus accessing
     key->security, on a key before checking to see whether the key usage is 0
     (ie. the key is dead and might be cleaned up).

Fix this by calling ->destroy() before cleaning up the core key data -
including key->security.

Reported-by: Petr Matousek 
Signed-off-by: David Howells 
Signed-off-by: Jiri Slaby

Don't leak a key reference if request_key() tries to use a revoked keyring

2015-04-09T11:14:24+00:00

commit d0709f1e66e8066c4ac6a54620ec116aa41937c0 upstream.

If a request_key() call to allocate and fill out a key attempts to insert the
key structure into a revoked keyring, the key will leak, using memory and part
of the user's key quota until the system reboots. This is from a failure of
construct_alloc_key() to decrement the key's reference count after the attempt
to insert into the requested keyring is rejected.

key_put() needs to be called in the link_prealloc_failed callpath to ensure
the unused key is released.

Signed-off-by: David Jeffery 
Signed-off-by: David Howells 
Signed-off-by: James Morris 
Signed-off-by: Jiri Slaby

KEYS: close race between key lookup and freeing

2015-01-29T14:45:09+00:00

commit a3a8784454692dd72e5d5d34dcdab17b4420e74c upstream.

When a key is being garbage collected, it's key->user would get put before
the ->destroy() callback is called, where the key is removed from it's
respective tracking structures.

This leaves a key hanging in a semi-invalid state which leaves a window open
for a different task to try an access key->user. An example is
find_keyring_by_name() which would dereference key->user for a key that is
in the process of being garbage collected (where key->user was freed but
->destroy() wasn't called yet - so it's still present in the linked list).

This would cause either a panic, or corrupt memory.

Fixes CVE-2014-9529.

Signed-off-by: Sasha Levin 
Signed-off-by: David Howells 
Signed-off-by: Jiri Slaby

KEYS: Fix stale key registration at error path

2015-01-07T16:55:06+00:00

commit b26bdde5bb27f3f900e25a95e33a0c476c8c2c48 upstream.

When loading encrypted-keys module, if the last check of
aes_get_sizes() in init_encrypted() fails, the driver just returns an
error without unregistering its key type.  This results in the stale
entry in the list.  In addition to memory leaks, this leads to a kernel
crash when registering a new key type later.

This patch fixes the problem by swapping the calls of aes_get_sizes()
and register_key_type(), and releasing resources properly at the error
paths.

Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=908163
Signed-off-by: Takashi Iwai 
Signed-off-by: Mimi Zohar 
Signed-off-by: Jiri Slaby

aio: don't include aio.h in sched.h

2013-05-08T03:16:25+00:00

Faster kernel compiles by way of fewer unnecessary includes.

[akpm@linux-foundation.org: fix fallout]
[akpm@linux-foundation.org: fix build]
Signed-off-by: Kent Overstreet 
Cc: Zach Brown 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: Mark Fasheh 
Cc: Joel Becker 
Cc: Rusty Russell 
Cc: Jens Axboe 
Cc: Asai Thambi S P 
Cc: Selvan Mani 
Cc: Sam Bradshaw 
Cc: Jeff Moyer 
Cc: Al Viro 
Cc: Benjamin LaHaise 
Reviewed-by: "Theodore Ts'o" 
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds

KEYS: split call to call_usermodehelper_fns()

2013-05-01T00:04:06+00:00

Use call_usermodehelper_setup() + call_usermodehelper_exec() instead of
calling call_usermodehelper_fns().  In case there's an OOM in this last
function the cleanup function may not be called - in this case we would
miss a call to key_put().

Signed-off-by: Lucas De Marchi 
Cc: Oleg Nesterov 
Acked-by: David Howells 
Acked-by: James Morris 
Cc: Al Viro 
Cc: Tejun Heo 
Cc: "Rafael J. Wysocki" 
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds

Fix: compat_rw_copy_check_uvector() misuse in aio, readv, writev, and security keys

2013-03-12T18:05:45+00:00

Looking at mm/process_vm_access.c:process_vm_rw() and comparing it to
compat_process_vm_rw() shows that the compatibility code requires an
explicit "access_ok()" check before calling
compat_rw_copy_check_uvector(). The same difference seems to appear when
we compare fs/read_write.c:do_readv_writev() to
fs/compat.c:compat_do_readv_writev().

This subtle difference between the compat and non-compat requirements
should probably be debated, as it seems to be error-prone. In fact,
there are two others sites that use this function in the Linux kernel,
and they both seem to get it wrong:

Now shifting our attention to fs/aio.c, we see that aio_setup_iocb()
also ends up calling compat_rw_copy_check_uvector() through
aio_setup_vectored_rw(). Unfortunately, the access_ok() check appears to
be missing. Same situation for
security/keys/compat.c:compat_keyctl_instantiate_key_iov().

I propose that we add the access_ok() check directly into
compat_rw_copy_check_uvector(), so callers don't have to worry about it,
and it therefore makes the compat call code similar to its non-compat
counterpart. Place the access_ok() check in the same location where
copy_from_user() can trigger a -EFAULT error in the non-compat code, so
the ABI behaviors are alike on both compat and non-compat.

While we are here, fix compat_do_readv_writev() so it checks for
compat_rw_copy_check_uvector() negative return values.

And also, fix a memory leak in compat_keyctl_instantiate_key_iov() error
handling.

Acked-by: Linus Torvalds 
Acked-by: Al Viro 
Signed-off-by: Mathieu Desnoyers 
Signed-off-by: Linus Torvalds

keys: fix race with concurrent install_user_keyrings()

2013-03-12T05:44:31+00:00

This fixes CVE-2013-1792.

There is a race in install_user_keyrings() that can cause a NULL pointer
dereference when called concurrently for the same user if the uid and
uid-session keyrings are not yet created.  It might be possible for an
unprivileged user to trigger this by calling keyctl() from userspace in
parallel immediately after logging in.

Assume that we have two threads both executing lookup_user_key(), both
looking for KEY_SPEC_USER_SESSION_KEYRING.

	THREAD A			THREAD B
	===============================	===============================
					==>call install_user_keyrings();
	if (!cred->user->session_keyring)
	==>call install_user_keyrings()
					...
					user->uid_keyring = uid_keyring;
	if (user->uid_keyring)
		return 0;
	<==
	key = cred->user->session_keyring [== NULL]
					user->session_keyring = session_keyring;
	atomic_inc(&key->usage); [oops]

At the point thread A dereferences cred->user->session_keyring, thread B
hasn't updated user->session_keyring yet, but thread A assumes it is
populated because install_user_keyrings() returned ok.

The race window is really small but can be exploited if, for example,
thread B is interrupted or preempted after initializing uid_keyring, but
before doing setting session_keyring.

This couldn't be reproduced on a stock kernel.  However, after placing
systemtap probe on 'user->session_keyring = session_keyring;' that
introduced some delay, the kernel could be crashed reliably.

Fix this by checking both pointers before deciding whether to return.
Alternatively, the test could be done away with entirely as it is checked
inside the mutex - but since the mutex is global, that may not be the best
way.

Signed-off-by: David Howells 
Reported-by: Mateusz Guzik 
Cc: 
Signed-off-by: Andrew Morton 
Signed-off-by: James Morris

userns: Stop oopsing in key_change_session_keyring

2013-03-04T03:35:38+00:00

Dave Jones  writes:
> Just hit this on Linus' current tree.
>
> [   89.621770] BUG: unable to handle kernel NULL pointer dereference at 00000000000000c8
> [   89.623111] IP: [] commit_creds+0x250/0x2f0
> [   89.624062] PGD 122bfd067 PUD 122bfe067 PMD 0
> [   89.624901] Oops: 0000 [#1] PREEMPT SMP
> [   89.625678] Modules linked in: caif_socket caif netrom bridge hidp 8021q garp stp mrp rose llc2 af_rxrpc phonet af_key binfmt_misc bnep l2tp_ppp can_bcm l2tp_core pppoe pppox can_raw scsi_transport_iscsi ppp_generic slhc nfnetlink can ipt_ULOG ax25 decnet irda nfc rds x25 crc_ccitt appletalk atm ipx p8023 psnap p8022 llc lockd sunrpc ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_conntrack nf_conntrack ip6table_filter ip6_tables btusb bluetooth snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_pcm vhost_net snd_page_alloc snd_timer tun macvtap usb_debug snd rfkill microcode macvlan edac_core pcspkr serio_raw kvm_amd soundcore kvm r8169 mii
> [   89.637846] CPU 2
> [   89.638175] Pid: 782, comm: trinity-main Not tainted 3.8.0+ #63 Gigabyte Technology Co., Ltd. GA-MA78GM-S2H/GA-MA78GM-S2H
> [   89.639850] RIP: 0010:[]  [] commit_creds+0x250/0x2f0
> [   89.641161] RSP: 0018:ffff880115657eb8  EFLAGS: 00010207
> [   89.641984] RAX: 00000000000003e8 RBX: ffff88012688b000 RCX: 0000000000000000
> [   89.643069] RDX: 0000000000000000 RSI: ffffffff81c32960 RDI: ffff880105839600
> [   89.644167] RBP: ffff880115657ed8 R08: 0000000000000000 R09: 0000000000000000
> [   89.645254] R10: 0000000000000001 R11: 0000000000000246 R12: ffff880105839600
> [   89.646340] R13: ffff88011beea490 R14: ffff88011beea490 R15: 0000000000000000
> [   89.647431] FS:  00007f3ac063b740(0000) GS:ffff88012b200000(0000) knlGS:0000000000000000
> [   89.648660] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [   89.649548] CR2: 00000000000000c8 CR3: 0000000122bfc000 CR4: 00000000000007e0
> [   89.650635] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   89.651723] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [   89.652812] Process trinity-main (pid: 782, threadinfo ffff880115656000, task ffff88011beea490)
> [   89.654128] Stack:
> [   89.654433]  0000000000000000 ffff8801058396a0 ffff880105839600 ffff88011beeaa78
> [   89.655769]  ffff880115657ef8 ffffffff812c7d9b ffffffff82079be0 0000000000000000
> [   89.657073]  ffff880115657f28 ffffffff8106c665 0000000000000002 ffff880115657f58
> [   89.658399] Call Trace:
> [   89.658822]  [] key_change_session_keyring+0xfb/0x140
> [   89.659845]  [] task_work_run+0xa5/0xd0
> [   89.660698]  [] do_notify_resume+0x71/0xb0
> [   89.661581]  [] int_signal+0x12/0x17
> [   89.662385] Code: 24 90 00 00 00 48 8b b3 90 00 00 00 49 8b 4c 24 40 48 39 f2 75 08 e9 83 00 00 00 48 89 ca 48 81 fa 60 29 c3 81 0f 84 41 fe ff ff <48> 8b 8a c8 00 00 00 48 39 ce 75 e4 3b 82 d0 00 00 00 0f 84 4b
> [   89.667778] RIP  [] commit_creds+0x250/0x2f0
> [   89.668733]  RSP 
> [   89.669301] CR2: 00000000000000c8
>
> My fastest trinity induced oops yet!
>
>
> Appears to be..
>
>                 if ((set_ns == subset_ns->parent)  &&
>      850:       48 8b 8a c8 00 00 00    mov    0xc8(%rdx),%rcx
>
> from the inlined cred_cap_issubset

By historical accident we have been reading trying to set new->user_ns
from new->user_ns.  Which is totally silly as new->user_ns is NULL (as
is every other field in new except session_keyring at that point).

The intent is clearly to copy all of the fields from old to new so copy
old->user_ns into  into new->user_ns.

Cc: stable@vger.kernel.org
Reported-by: Dave Jones 
Tested-by: Dave Jones 
Acked-by: Serge Hallyn 
Signed-off-by: "Eric W. Biederman"