[GIT PULL] keys bugfix

[James Morris]

This is a resend of just the first (critical) fix.

Please pull.


The following changes since commit 8db7b3c54401d83a4dc370a59b8692854000ea03:

  Merge branch 'parisc-4.4-4' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux (2015-12-25 13:19:50 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git for-linus2

David Howells (1):
      KEYS: Fix race between read and revoke

 security/keys/keyctl.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

---

commit b4a1b4f5047e4f54e194681125c74c0aa64d637d
Author: David Howells <dhowells@xxxxxxxxxx>
Date:   Fri Dec 18 01:34:26 2015 +0000

    KEYS: Fix race between read and revoke
    
    This fixes CVE-2015-7550.
    
    There's a race between keyctl_read() and keyctl_revoke().  If the revoke
    happens between keyctl_read() checking the validity of a key and the key's
    semaphore being taken, then the key type read method will see a revoked key.
    
    This causes a problem for the user-defined key type because it assumes in
    its read method that there will always be a payload in a non-revoked key
    and doesn't check for a NULL pointer.
    
    Fix this by making keyctl_read() check the validity of a key after taking
    semaphore instead of before.
    
    I think the bug was introduced with the original keyrings code.
    
    This was discovered by a multithreaded test program generated by syzkaller
    (http://github.com/google/syzkaller).  Here's a cleaned up version:
    
    	#include <sys/types.h>
    	#include <keyutils.h>
    	#include <pthread.h>
    	void *thr0(void *arg)
    	{
    		key_serial_t key = (unsigned long)arg;
    		keyctl_revoke(key);
    		return 0;
    	}
    	void *thr1(void *arg)
    	{
    		key_serial_t key = (unsigned long)arg;
    		char buffer[16];
    		keyctl_read(key, buffer, 16);
    		return 0;
    	}
    	int main()
    	{
    		key_serial_t key = add_key("user", "%", "foo", 3, KEY_SPEC_USER_KEYRING);
    		pthread_t th[5];
    		pthread_create(&th[0], 0, thr0, (void *)(unsigned long)key);
    		pthread_create(&th[1], 0, thr1, (void *)(unsigned long)key);
    		pthread_create(&th[2], 0, thr0, (void *)(unsigned long)key);
    		pthread_create(&th[3], 0, thr1, (void *)(unsigned long)key);
    		pthread_join(th[0], 0);
    		pthread_join(th[1], 0);
    		pthread_join(th[2], 0);
    		pthread_join(th[3], 0);
    		return 0;
    	}
    
    Build as:
    
    	cc -o keyctl-race keyctl-race.c -lkeyutils -lpthread
    
    Run as:
    
    	while keyctl-race; do :; done
    
    as it may need several iterations to crash the kernel.  The crash can be
    summarised as:
    
    	BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
    	IP: [<ffffffff81279b08>] user_read+0x56/0xa3
    	...
    	Call Trace:
    	 [<ffffffff81276aa9>] keyctl_read_key+0xb6/0xd7
    	 [<ffffffff81277815>] SyS_keyctl+0x83/0xe0
    	 [<ffffffff815dbb97>] entry_SYSCALL_64_fastpath+0x12/0x6f
    
    Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
    Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
    Tested-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
    Cc: stable@xxxxxxxxxxxxxxx
    Signed-off-by: James Morris <james.l.morris@xxxxxxxxxx>

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index fb111ea..1c3872a 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -751,16 +751,16 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 
 	/* the key is probably readable - now try to read it */
 can_read_key:
-	ret = key_validate(key);
-	if (ret == 0) {
-		ret = -EOPNOTSUPP;
-		if (key->type->read) {
-			/* read the data with the semaphore held (since we
-			 * might sleep) */
-			down_read(&key->sem);
+	ret = -EOPNOTSUPP;
+	if (key->type->read) {
+		/* Read the data with the semaphore held (since we might sleep)
+		 * to protect against the key being updated or revoked.
+		 */
+		down_read(&key->sem);
+		ret = key_validate(key);
+		if (ret == 0)
 			ret = key->type->read(key, buffer, buflen);
-			up_read(&key->sem);
-		}
+		up_read(&key->sem);
 	}
 
 error2:

-- 
James Morris
<jmorris@xxxxxxxxx>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/