From 6e753e515f9ee87879a0630ba71366580dd0195f Mon Sep 17 00:00:00 2001 From: David Howells Date: Tue, 12 Mar 2013 16:44:31 +1100 Subject: keys: fix race with concurrent install_user_keyrings() commit 0da9dfdd2cd9889201bc6f6f43580c99165cd087 upstream. 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 Signed-off-by: Andrew Morton Signed-off-by: James Morris Signed-off-by: Greg Kroah-Hartman --- security/keys/process_keys.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c index a3063eb..44a5100 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -54,7 +54,7 @@ int install_user_keyrings(void) kenter("%p{%u}", user, user->uid); - if (user->uid_keyring) { + if (user->uid_keyring && user->session_keyring) { kleave(" = 0 [exist]"); return 0; } -- cgit v1.1 From 5e44eef46e81bf6e159b44fa893cf7dd47a88f53 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Sat, 16 Mar 2013 12:48:11 +0300 Subject: selinux: use GFP_ATOMIC under spin_lock commit 4502403dcf8f5c76abd4dbab8726c8e4ecb5cd34 upstream. The call tree here is: sk_clone_lock() <- takes bh_lock_sock(newsk); xfrm_sk_clone_policy() __xfrm_sk_clone_policy() clone_policy() <- uses GFP_ATOMIC for allocations security_xfrm_policy_clone() security_ops->xfrm_policy_clone_security() selinux_xfrm_policy_clone() Signed-off-by: Dan Carpenter Signed-off-by: James Morris Signed-off-by: Greg Kroah-Hartman --- security/selinux/xfrm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c index 68178b7..1df278c 100644 --- a/security/selinux/xfrm.c +++ b/security/selinux/xfrm.c @@ -310,7 +310,7 @@ int selinux_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx, if (old_ctx) { new_ctx = kmalloc(sizeof(*old_ctx) + old_ctx->ctx_len, - GFP_KERNEL); + GFP_ATOMIC); if (!new_ctx) return -ENOMEM; -- cgit v1.1 From 8fc39830ce6dc2a93f29872afce83b180a6d3deb Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Fri, 28 Sep 2012 12:20:02 +0100 Subject: key: Fix resource leak commit a84a921978b7d56e0e4b87ffaca6367429b4d8ff upstream. On an error iov may still have been reallocated and need freeing Signed-off-by: Alan Cox Signed-off-by: David Howells Signed-off-by: Greg Kroah-Hartman --- security/keys/keyctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index eca5191..9f9cc3a 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -1067,12 +1067,12 @@ long keyctl_instantiate_key_iov(key_serial_t id, ret = rw_copy_check_uvector(WRITE, _payload_iov, ioc, ARRAY_SIZE(iovstack), iovstack, &iov); if (ret < 0) - return ret; + goto err; if (ret == 0) goto no_payload_free; ret = keyctl_instantiate_key_common(id, iov, ioc, ret, ringid); - +err: if (iov != iovstack) kfree(iov); return ret; -- cgit v1.1 From c18508394610b47964ef6c2d4d71b85873ce10fe Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Mon, 25 Feb 2013 10:20:36 -0500 Subject: Fix: compat_rw_copy_check_uvector() misuse in aio, readv, writev, and security keys commit 8aec0f5d4137532de14e6554fd5dd201ff3a3c49 upstream. 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 Signed-off-by: Greg Kroah-Hartman --- security/keys/compat.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/keys/compat.c b/security/keys/compat.c index 338b510..10a6e4c 100644 --- a/security/keys/compat.c +++ b/security/keys/compat.c @@ -40,12 +40,12 @@ long compat_keyctl_instantiate_key_iov( ARRAY_SIZE(iovstack), iovstack, &iov); if (ret < 0) - return ret; + goto err; if (ret == 0) goto no_payload_free; ret = keyctl_instantiate_key_common(id, iov, ioc, ret, ringid); - +err: if (iov != iovstack) kfree(iov); return ret; -- cgit v1.1