[Devel] [PATCH RHEL7 COMMIT] ms/keys: Fix race between read and revoke

[Konstantin Khorenko]

The commit is pushed to "branch-rh7-3.10.0-327.3.1-vz7.10.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
after rh7-3.10.0-327.3.1.vz7.10.9
------>
commit ae7683f77d40269530da4424bb66df9822f3ed19
Author: Vasily Averin <vvs at virtuozzo.com>
Date:   Wed Feb 3 19:18:29 2016 +0400

    ms/keys: Fix race between read and revoke
    
    Author: David Howells <dhowells at redhat.com>
    
    ms commit: b4a1b4f5047e4f54e194681125c74c0aa64d637d
    
        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 at google.com>
        Signed-off-by: David Howells <dhowells at redhat.com>
        Tested-by: Dmitry Vyukov <dvyukov at google.com>
        Cc: stable at vger.kernel.org
        Signed-off-by: James Morris <james.l.morris at oracle.com>
    
    https://jira.sw.ru/browse/PSBM-43800
    
    Signed-off-by:	Vasily Averin <vvs at virtuozzo.com>
    
    khorenko@: RedHat has decided not to fix this security issue for some reason.
---
 security/keys/keyctl.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 762cb3d..f4f904d 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -761,16 +761,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: