All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/6] KEYS: fix atomicity issues with key flags
@ 2017-09-26 20:10 ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:10 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

The first patch in this series fixes the race between updating and
finding a negative key, which could be used by an unprivileged user to
cause a kernel oops.  That patch is Cc'ed to stable.

The remaining patches fix some other, more theoretical atomicity issues
with accessing key->flags and key->expiry, then eliminate
KEY_FLAG_NEGATIVE, which becomes unnecessary after the first patch.

Eric Biggers (6):
  KEYS: fix race between updating and finding negative key
  KEYS: load key flags atomically in key_is_instantiated()
  KEYS: load key flags and expiry time atomically in key_validate()
  KEYS: load key flags and expiry time atomically in
    keyring_search_iterator()
  KEYS: load key flags and expiry time atomically in proc_keys_show()
  KEYS: remove KEY_FLAG_NEGATIVE

 include/linux/key.h                      | 37 +++++++++++++++++++++++---------
 security/keys/encrypted-keys/encrypted.c |  2 +-
 security/keys/gc.c                       |  4 +---
 security/keys/key.c                      | 24 +++++++++++++++------
 security/keys/keyctl.c                   |  5 ++++-
 security/keys/keyring.c                  | 12 ++++++-----
 security/keys/permission.c               |  7 +++---
 security/keys/proc.c                     | 28 +++++++++++++-----------
 security/keys/request_key.c              | 11 ++++++----
 security/keys/trusted.c                  |  2 +-
 security/keys/user_defined.c             |  2 +-
 11 files changed, 86 insertions(+), 48 deletions(-)

-- 
2.14.1.992.g2c7b836f3a-goog


^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v2 0/6] KEYS: fix atomicity issues with key flags
@ 2017-09-26 20:10 ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:10 UTC (permalink / raw)
  To: keyrings
  Cc: David Howells, Michael Halcrow, linux-security-module,
	linux-kernel, Eric Biggers

From: Eric Biggers <ebiggers@google.com>

The first patch in this series fixes the race between updating and
finding a negative key, which could be used by an unprivileged user to
cause a kernel oops.  That patch is Cc'ed to stable.

The remaining patches fix some other, more theoretical atomicity issues
with accessing key->flags and key->expiry, then eliminate
KEY_FLAG_NEGATIVE, which becomes unnecessary after the first patch.

Eric Biggers (6):
  KEYS: fix race between updating and finding negative key
  KEYS: load key flags atomically in key_is_instantiated()
  KEYS: load key flags and expiry time atomically in key_validate()
  KEYS: load key flags and expiry time atomically in
    keyring_search_iterator()
  KEYS: load key flags and expiry time atomically in proc_keys_show()
  KEYS: remove KEY_FLAG_NEGATIVE

 include/linux/key.h                      | 37 +++++++++++++++++++++++---------
 security/keys/encrypted-keys/encrypted.c |  2 +-
 security/keys/gc.c                       |  4 +---
 security/keys/key.c                      | 24 +++++++++++++++------
 security/keys/keyctl.c                   |  5 ++++-
 security/keys/keyring.c                  | 12 ++++++-----
 security/keys/permission.c               |  7 +++---
 security/keys/proc.c                     | 28 +++++++++++++-----------
 security/keys/request_key.c              | 11 ++++++----
 security/keys/trusted.c                  |  2 +-
 security/keys/user_defined.c             |  2 +-
 11 files changed, 86 insertions(+), 48 deletions(-)

-- 
2.14.1.992.g2c7b836f3a-goog

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v2 0/6] KEYS: fix atomicity issues with key flags
@ 2017-09-26 20:10 ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:10 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

The first patch in this series fixes the race between updating and
finding a negative key, which could be used by an unprivileged user to
cause a kernel oops.  That patch is Cc'ed to stable.

The remaining patches fix some other, more theoretical atomicity issues
with accessing key->flags and key->expiry, then eliminate
KEY_FLAG_NEGATIVE, which becomes unnecessary after the first patch.

Eric Biggers (6):
  KEYS: fix race between updating and finding negative key
  KEYS: load key flags atomically in key_is_instantiated()
  KEYS: load key flags and expiry time atomically in key_validate()
  KEYS: load key flags and expiry time atomically in
    keyring_search_iterator()
  KEYS: load key flags and expiry time atomically in proc_keys_show()
  KEYS: remove KEY_FLAG_NEGATIVE

 include/linux/key.h                      | 37 +++++++++++++++++++++++---------
 security/keys/encrypted-keys/encrypted.c |  2 +-
 security/keys/gc.c                       |  4 +---
 security/keys/key.c                      | 24 +++++++++++++++------
 security/keys/keyctl.c                   |  5 ++++-
 security/keys/keyring.c                  | 12 ++++++-----
 security/keys/permission.c               |  7 +++---
 security/keys/proc.c                     | 28 +++++++++++++-----------
 security/keys/request_key.c              | 11 ++++++----
 security/keys/trusted.c                  |  2 +-
 security/keys/user_defined.c             |  2 +-
 11 files changed, 86 insertions(+), 48 deletions(-)

-- 
2.14.1.992.g2c7b836f3a-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v2 1/6] KEYS: fix race between updating and finding negative key
  2017-09-26 20:10 ` Eric Biggers
  (?)
@ 2017-09-26 20:11   ` Eric Biggers
  -1 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

In keyring_search_iterator() and in wait_for_key_construction(), we
check whether the key has been negatively instantiated, and if so return
the key's ->reject_error.

However, no lock is held during this, and ->reject_error is in union
with ->payload.  And it's impossible for KEY_FLAG_NEGATIVE to be updated
atomically with respect to ->reject_error and ->payload.

Most problematically, when a negative key is positively instantiated via
__key_update() (via sys_add_key()), ->payload is initialized first, then
KEY_FLAG_NEGATIVE is cleared.  But that means that ->reject_error can be
observed to have a bogus value, having been overwritten with ->payload,
while the key still appears to be "negative".  Clearing
KEY_FLAG_NEGATIVE first wouldn't work either, since then anyone who
accesses the payload under rcu_read_lock() rather than the key semaphore
might observe an uninitialized ->payload.  Nor can we just always take
the key's semaphore when checking whether the key is negative, since
keyring searches happen under rcu_read_lock().

Therefore, fix the bug by moving ->reject_error into the high bits of
->flags so that we can read and write it atomically with respect to
KEY_FLAG_NEGATIVE and KEY_FLAG_INSTANTIATED.

This will also allow KEY_FLAG_NEGATIVE to be removed, since tests for
KEY_FLAG_NEGATIVE can be replaced with tests for nonzero reject_error.
But for ease of backporting this fix, that is left for a later patch.

This fixes a kernel crash caused by the following program:

    #include <stdlib.h>
    #include <unistd.h>
    #include <keyutils.h>

    int main(void)
    {
        int ringid = keyctl_join_session_keyring(NULL);

        if (fork()) {
            for (;;) {
                usleep(rand() % 4096);
                add_key("user", "desc", "x", 1, ringid);
                keyctl_clear(ringid);
            }
        } else {
            for (;;)
                request_key("user", "desc", "", ringid);
        }
    }

Here is the crash:

    BUG: unable to handle kernel paging request at fffffffffd39a6b0
    IP: __key_link_begin+0x0/0x100
    PGD 7a0a067 P4D 7a0a067 PUD 7a0c067 PMD 0
    Oops: 0000 [#1] SMP
    CPU: 1 PID: 165 Comm: keyctl_negate_r Not tainted 4.14.0-rc1 #377
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-20170228_101828-anatol 04/01/2014
    task: ffff9791fd809140 task.stack: ffffacba402bc000
    RIP: 0010:__key_link_begin+0x0/0x100
    RSP: 0018:ffffacba402bfdc8 EFLAGS: 00010282
    RAX: ffff9791fd809140 RBX: fffffffffd39a620 RCX: 0000000000000008
    RDX: ffffacba402bfdd0 RSI: fffffffffd39a6a0 RDI: ffff9791fd810600
    RBP: ffffacba402bfdf8 R08: 0000000000000063 R09: ffffffff94845620
    R10: 8080808080808080 R11: 0000000000000004 R12: ffff9791fd810600
    R13: ffff9791fd39a940 R14: fffffffffd39a6a0 R15: 0000000000000000
    FS:  00007fbf14a90740(0000) GS:ffff9791ffd00000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: fffffffffd39a6b0 CR3: 000000003b910003 CR4: 00000000003606e0
    Call Trace:
     ? key_link+0x28/0xb0
     ? search_process_keyrings+0x13/0x100
     request_key_and_link+0xcb/0x550
     ? keyring_instantiate+0x110/0x110
     ? key_default_cmp+0x20/0x20
     SyS_request_key+0xc0/0x160
     ? exit_to_usermode_loop+0x5e/0x80
     entry_SYSCALL_64_fastpath+0x1a/0xa5
    RIP: 0033:0x7fbf14190bb9
    RSP: 002b:00007ffd8e4fe6c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000f9
    RAX: ffffffffffffffda RBX: 0000000036cc28fb RCX: 00007fbf14190bb9
    RDX: 000055748b56ca4a RSI: 000055748b56ca46 RDI: 000055748b56ca4b
    RBP: 000055748b56ca4a R08: 0000000000000001 R09: 0000000000000001
    R10: 0000000036cc28fb R11: 0000000000000246 R12: 000055748b56c8b0
    R13: 00007ffd8e4fe7d0 R14: 0000000000000000 R15: 0000000000000000
    Code: c5 0f 85 69 ff ff ff 48 c7 c3 82 ff ff ff eb ab 45 31 ed e9 18 ff ff ff 85 c0 75 8d eb d2 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 <48> 83 7e 10 00 0f 84 c5 00 00 00 55 48 89 e5 41 57 41 56 41 55
    RIP: __key_link_begin+0x0/0x100 RSP: ffffacba402bfdc8
    CR2: fffffffffd39a6b0

Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
Cc: <stable@vger.kernel.org>	[v4.4+]
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/key.h         | 12 +++++++++++-
 security/keys/key.c         | 26 +++++++++++++++++++-------
 security/keys/keyctl.c      |  3 +++
 security/keys/keyring.c     |  4 ++--
 security/keys/request_key.c | 11 +++++++----
 5 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index e315e16b6ff8..b7b590d7c480 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -189,6 +189,17 @@ struct key {
 #define KEY_FLAG_KEEP		10	/* set if key should not be removed */
 #define KEY_FLAG_UID_KEYRING	11	/* set if key is a user or user session keyring */
 
+	/*
+	 * If the key is negatively instantiated, then bits 20-31 hold the error
+	 * code which should be returned when someone tries to use the key
+	 * (unless they allow negative keys).  The error code is stored as a
+	 * positive number, so it must be negated before being returned.
+	 *
+	 * Note that a key can go from negative to positive but not vice versa.
+	 */
+#define KEY_FLAGS_REJECT_ERROR_SHIFT	20
+#define KEY_FLAGS_REJECT_ERROR_MASK	0xFFF00000
+
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
 	 * - it should be a printable string
@@ -213,7 +224,6 @@ struct key {
 			struct list_head name_link;
 			struct assoc_array keys;
 		};
-		int reject_error;
 	};
 
 	/* This is set on a keyring to restrict the addition of a link to a key
diff --git a/security/keys/key.c b/security/keys/key.c
index eb914a838840..3ffb6829972f 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -401,6 +401,20 @@ int key_payload_reserve(struct key *key, size_t datalen)
 }
 EXPORT_SYMBOL(key_payload_reserve);
 
+static void mark_key_instantiated(struct key *key, unsigned int reject_error)
+{
+	unsigned long old, new;
+
+	do {
+		old = READ_ONCE(key->flags);
+		new = (old & ~(KEY_FLAG_NEGATIVE |
+			       KEY_FLAGS_REJECT_ERROR_MASK)) |
+		      KEY_FLAG_INSTANTIATED |
+		      (reject_error ? KEY_FLAG_NEGATIVE : 0) |
+		      (reject_error << KEY_FLAGS_REJECT_ERROR_SHIFT);
+	} while (cmpxchg_release(&key->flags, old, new) != old);
+}
+
 /*
  * Instantiate a key and link it into the target keyring atomically.  Must be
  * called with the target keyring's semaphore writelocked.  The target key's
@@ -431,7 +445,7 @@ static int __key_instantiate_and_link(struct key *key,
 		if (ret = 0) {
 			/* mark the key as being instantiated */
 			atomic_inc(&key->user->nikeys);
-			set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+			mark_key_instantiated(key, 0);
 
 			if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
 				awaken = 1;
@@ -580,10 +594,8 @@ int key_reject_and_link(struct key *key,
 	if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
 		/* mark the key as being negatively instantiated */
 		atomic_inc(&key->user->nikeys);
-		key->reject_error = -error;
-		smp_wmb();
-		set_bit(KEY_FLAG_NEGATIVE, &key->flags);
-		set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+		mark_key_instantiated(key, error);
+
 		now = current_kernel_time();
 		key->expiry = now.tv_sec + timeout;
 		key_schedule_gc(key->expiry + key_gc_delay);
@@ -753,7 +765,7 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
 	ret = key->type->update(key, prep);
 	if (ret = 0)
 		/* updating a negative key instantiates it */
-		clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
@@ -987,7 +999,7 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 	ret = key->type->update(key, &prep);
 	if (ret = 0)
 		/* updating a negative key instantiates it */
-		clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 365ff85d7e27..19a09e121089 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1223,6 +1223,9 @@ long keyctl_reject_key(key_serial_t id, unsigned timeout, unsigned error,
 	    error = ERESTART_RESTARTBLOCK)
 		return -EINVAL;
 
+	BUILD_BUG_ON(MAX_ERRNO > (KEY_FLAGS_REJECT_ERROR_MASK >>
+				  KEY_FLAGS_REJECT_ERROR_SHIFT));
+
 	/* the appropriate instantiation authorisation key must have been
 	 * assumed before calling this */
 	ret = -EPERM;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..7fc661f492d3 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -598,8 +598,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
 		/* we set a different error code if we pass a negative key */
 		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
-			smp_rmb();
-			ctx->result = ERR_PTR(key->reject_error);
+			ctx->result = ERR_PTR(-(int)(kflags >>
+						KEY_FLAGS_REJECT_ERROR_SHIFT));
 			kleave(" = %d [neg]", ctx->skipped_ret);
 			goto skipped;
 		}
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 63e63a42db3c..0aab68344837 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -590,15 +590,18 @@ struct key *request_key_and_link(struct key_type *type,
 int wait_for_key_construction(struct key *key, bool intr)
 {
 	int ret;
+	unsigned long flags;
 
 	ret = wait_on_bit(&key->flags, KEY_FLAG_USER_CONSTRUCT,
 			  intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
 	if (ret)
 		return -ERESTARTSYS;
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
-		smp_rmb();
-		return key->reject_error;
-	}
+
+	/* Pairs with RELEASE in mark_key_instantiated() */
+	flags = smp_load_acquire(&key->flags);
+	if (flags & (1 << KEY_FLAG_NEGATIVE))
+		return -(int)(flags >> KEY_FLAGS_REJECT_ERROR_SHIFT);
+
 	return key_validate(key);
 }
 EXPORT_SYMBOL(wait_for_key_construction);
-- 
2.14.1.992.g2c7b836f3a-goog


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 1/6] KEYS: fix race between updating and finding negative key
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: keyrings
  Cc: David Howells, Michael Halcrow, linux-security-module,
	linux-kernel, Eric Biggers, stable

From: Eric Biggers <ebiggers@google.com>

In keyring_search_iterator() and in wait_for_key_construction(), we
check whether the key has been negatively instantiated, and if so return
the key's ->reject_error.

However, no lock is held during this, and ->reject_error is in union
with ->payload.  And it's impossible for KEY_FLAG_NEGATIVE to be updated
atomically with respect to ->reject_error and ->payload.

Most problematically, when a negative key is positively instantiated via
__key_update() (via sys_add_key()), ->payload is initialized first, then
KEY_FLAG_NEGATIVE is cleared.  But that means that ->reject_error can be
observed to have a bogus value, having been overwritten with ->payload,
while the key still appears to be "negative".  Clearing
KEY_FLAG_NEGATIVE first wouldn't work either, since then anyone who
accesses the payload under rcu_read_lock() rather than the key semaphore
might observe an uninitialized ->payload.  Nor can we just always take
the key's semaphore when checking whether the key is negative, since
keyring searches happen under rcu_read_lock().

Therefore, fix the bug by moving ->reject_error into the high bits of
->flags so that we can read and write it atomically with respect to
KEY_FLAG_NEGATIVE and KEY_FLAG_INSTANTIATED.

This will also allow KEY_FLAG_NEGATIVE to be removed, since tests for
KEY_FLAG_NEGATIVE can be replaced with tests for nonzero reject_error.
But for ease of backporting this fix, that is left for a later patch.

This fixes a kernel crash caused by the following program:

    #include <stdlib.h>
    #include <unistd.h>
    #include <keyutils.h>

    int main(void)
    {
        int ringid = keyctl_join_session_keyring(NULL);

        if (fork()) {
            for (;;) {
                usleep(rand() % 4096);
                add_key("user", "desc", "x", 1, ringid);
                keyctl_clear(ringid);
            }
        } else {
            for (;;)
                request_key("user", "desc", "", ringid);
        }
    }

Here is the crash:

    BUG: unable to handle kernel paging request at fffffffffd39a6b0
    IP: __key_link_begin+0x0/0x100
    PGD 7a0a067 P4D 7a0a067 PUD 7a0c067 PMD 0
    Oops: 0000 [#1] SMP
    CPU: 1 PID: 165 Comm: keyctl_negate_r Not tainted 4.14.0-rc1 #377
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-20170228_101828-anatol 04/01/2014
    task: ffff9791fd809140 task.stack: ffffacba402bc000
    RIP: 0010:__key_link_begin+0x0/0x100
    RSP: 0018:ffffacba402bfdc8 EFLAGS: 00010282
    RAX: ffff9791fd809140 RBX: fffffffffd39a620 RCX: 0000000000000008
    RDX: ffffacba402bfdd0 RSI: fffffffffd39a6a0 RDI: ffff9791fd810600
    RBP: ffffacba402bfdf8 R08: 0000000000000063 R09: ffffffff94845620
    R10: 8080808080808080 R11: 0000000000000004 R12: ffff9791fd810600
    R13: ffff9791fd39a940 R14: fffffffffd39a6a0 R15: 0000000000000000
    FS:  00007fbf14a90740(0000) GS:ffff9791ffd00000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: fffffffffd39a6b0 CR3: 000000003b910003 CR4: 00000000003606e0
    Call Trace:
     ? key_link+0x28/0xb0
     ? search_process_keyrings+0x13/0x100
     request_key_and_link+0xcb/0x550
     ? keyring_instantiate+0x110/0x110
     ? key_default_cmp+0x20/0x20
     SyS_request_key+0xc0/0x160
     ? exit_to_usermode_loop+0x5e/0x80
     entry_SYSCALL_64_fastpath+0x1a/0xa5
    RIP: 0033:0x7fbf14190bb9
    RSP: 002b:00007ffd8e4fe6c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000f9
    RAX: ffffffffffffffda RBX: 0000000036cc28fb RCX: 00007fbf14190bb9
    RDX: 000055748b56ca4a RSI: 000055748b56ca46 RDI: 000055748b56ca4b
    RBP: 000055748b56ca4a R08: 0000000000000001 R09: 0000000000000001
    R10: 0000000036cc28fb R11: 0000000000000246 R12: 000055748b56c8b0
    R13: 00007ffd8e4fe7d0 R14: 0000000000000000 R15: 0000000000000000
    Code: c5 0f 85 69 ff ff ff 48 c7 c3 82 ff ff ff eb ab 45 31 ed e9 18 ff ff ff 85 c0 75 8d eb d2 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 <48> 83 7e 10 00 0f 84 c5 00 00 00 55 48 89 e5 41 57 41 56 41 55
    RIP: __key_link_begin+0x0/0x100 RSP: ffffacba402bfdc8
    CR2: fffffffffd39a6b0

Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
Cc: <stable@vger.kernel.org>	[v4.4+]
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/key.h         | 12 +++++++++++-
 security/keys/key.c         | 26 +++++++++++++++++++-------
 security/keys/keyctl.c      |  3 +++
 security/keys/keyring.c     |  4 ++--
 security/keys/request_key.c | 11 +++++++----
 5 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index e315e16b6ff8..b7b590d7c480 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -189,6 +189,17 @@ struct key {
 #define KEY_FLAG_KEEP		10	/* set if key should not be removed */
 #define KEY_FLAG_UID_KEYRING	11	/* set if key is a user or user session keyring */
 
+	/*
+	 * If the key is negatively instantiated, then bits 20-31 hold the error
+	 * code which should be returned when someone tries to use the key
+	 * (unless they allow negative keys).  The error code is stored as a
+	 * positive number, so it must be negated before being returned.
+	 *
+	 * Note that a key can go from negative to positive but not vice versa.
+	 */
+#define KEY_FLAGS_REJECT_ERROR_SHIFT	20
+#define KEY_FLAGS_REJECT_ERROR_MASK	0xFFF00000
+
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
 	 * - it should be a printable string
@@ -213,7 +224,6 @@ struct key {
 			struct list_head name_link;
 			struct assoc_array keys;
 		};
-		int reject_error;
 	};
 
 	/* This is set on a keyring to restrict the addition of a link to a key
diff --git a/security/keys/key.c b/security/keys/key.c
index eb914a838840..3ffb6829972f 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -401,6 +401,20 @@ int key_payload_reserve(struct key *key, size_t datalen)
 }
 EXPORT_SYMBOL(key_payload_reserve);
 
+static void mark_key_instantiated(struct key *key, unsigned int reject_error)
+{
+	unsigned long old, new;
+
+	do {
+		old = READ_ONCE(key->flags);
+		new = (old & ~(KEY_FLAG_NEGATIVE |
+			       KEY_FLAGS_REJECT_ERROR_MASK)) |
+		      KEY_FLAG_INSTANTIATED |
+		      (reject_error ? KEY_FLAG_NEGATIVE : 0) |
+		      (reject_error << KEY_FLAGS_REJECT_ERROR_SHIFT);
+	} while (cmpxchg_release(&key->flags, old, new) != old);
+}
+
 /*
  * Instantiate a key and link it into the target keyring atomically.  Must be
  * called with the target keyring's semaphore writelocked.  The target key's
@@ -431,7 +445,7 @@ static int __key_instantiate_and_link(struct key *key,
 		if (ret == 0) {
 			/* mark the key as being instantiated */
 			atomic_inc(&key->user->nikeys);
-			set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+			mark_key_instantiated(key, 0);
 
 			if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
 				awaken = 1;
@@ -580,10 +594,8 @@ int key_reject_and_link(struct key *key,
 	if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
 		/* mark the key as being negatively instantiated */
 		atomic_inc(&key->user->nikeys);
-		key->reject_error = -error;
-		smp_wmb();
-		set_bit(KEY_FLAG_NEGATIVE, &key->flags);
-		set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+		mark_key_instantiated(key, error);
+
 		now = current_kernel_time();
 		key->expiry = now.tv_sec + timeout;
 		key_schedule_gc(key->expiry + key_gc_delay);
@@ -753,7 +765,7 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
 	ret = key->type->update(key, prep);
 	if (ret == 0)
 		/* updating a negative key instantiates it */
-		clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
@@ -987,7 +999,7 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 	ret = key->type->update(key, &prep);
 	if (ret == 0)
 		/* updating a negative key instantiates it */
-		clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 365ff85d7e27..19a09e121089 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1223,6 +1223,9 @@ long keyctl_reject_key(key_serial_t id, unsigned timeout, unsigned error,
 	    error == ERESTART_RESTARTBLOCK)
 		return -EINVAL;
 
+	BUILD_BUG_ON(MAX_ERRNO > (KEY_FLAGS_REJECT_ERROR_MASK >>
+				  KEY_FLAGS_REJECT_ERROR_SHIFT));
+
 	/* the appropriate instantiation authorisation key must have been
 	 * assumed before calling this */
 	ret = -EPERM;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..7fc661f492d3 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -598,8 +598,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
 		/* we set a different error code if we pass a negative key */
 		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
-			smp_rmb();
-			ctx->result = ERR_PTR(key->reject_error);
+			ctx->result = ERR_PTR(-(int)(kflags >>
+						KEY_FLAGS_REJECT_ERROR_SHIFT));
 			kleave(" = %d [neg]", ctx->skipped_ret);
 			goto skipped;
 		}
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 63e63a42db3c..0aab68344837 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -590,15 +590,18 @@ struct key *request_key_and_link(struct key_type *type,
 int wait_for_key_construction(struct key *key, bool intr)
 {
 	int ret;
+	unsigned long flags;
 
 	ret = wait_on_bit(&key->flags, KEY_FLAG_USER_CONSTRUCT,
 			  intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
 	if (ret)
 		return -ERESTARTSYS;
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
-		smp_rmb();
-		return key->reject_error;
-	}
+
+	/* Pairs with RELEASE in mark_key_instantiated() */
+	flags = smp_load_acquire(&key->flags);
+	if (flags & (1 << KEY_FLAG_NEGATIVE))
+		return -(int)(flags >> KEY_FLAGS_REJECT_ERROR_SHIFT);
+
 	return key_validate(key);
 }
 EXPORT_SYMBOL(wait_for_key_construction);
-- 
2.14.1.992.g2c7b836f3a-goog

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 1/6] KEYS: fix race between updating and finding negative key
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

In keyring_search_iterator() and in wait_for_key_construction(), we
check whether the key has been negatively instantiated, and if so return
the key's ->reject_error.

However, no lock is held during this, and ->reject_error is in union
with ->payload.  And it's impossible for KEY_FLAG_NEGATIVE to be updated
atomically with respect to ->reject_error and ->payload.

Most problematically, when a negative key is positively instantiated via
__key_update() (via sys_add_key()), ->payload is initialized first, then
KEY_FLAG_NEGATIVE is cleared.  But that means that ->reject_error can be
observed to have a bogus value, having been overwritten with ->payload,
while the key still appears to be "negative".  Clearing
KEY_FLAG_NEGATIVE first wouldn't work either, since then anyone who
accesses the payload under rcu_read_lock() rather than the key semaphore
might observe an uninitialized ->payload.  Nor can we just always take
the key's semaphore when checking whether the key is negative, since
keyring searches happen under rcu_read_lock().

Therefore, fix the bug by moving ->reject_error into the high bits of
->flags so that we can read and write it atomically with respect to
KEY_FLAG_NEGATIVE and KEY_FLAG_INSTANTIATED.

This will also allow KEY_FLAG_NEGATIVE to be removed, since tests for
KEY_FLAG_NEGATIVE can be replaced with tests for nonzero reject_error.
But for ease of backporting this fix, that is left for a later patch.

This fixes a kernel crash caused by the following program:

    #include <stdlib.h>
    #include <unistd.h>
    #include <keyutils.h>

    int main(void)
    {
        int ringid = keyctl_join_session_keyring(NULL);

        if (fork()) {
            for (;;) {
                usleep(rand() % 4096);
                add_key("user", "desc", "x", 1, ringid);
                keyctl_clear(ringid);
            }
        } else {
            for (;;)
                request_key("user", "desc", "", ringid);
        }
    }

Here is the crash:

    BUG: unable to handle kernel paging request at fffffffffd39a6b0
    IP: __key_link_begin+0x0/0x100
    PGD 7a0a067 P4D 7a0a067 PUD 7a0c067 PMD 0
    Oops: 0000 [#1] SMP
    CPU: 1 PID: 165 Comm: keyctl_negate_r Not tainted 4.14.0-rc1 #377
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-20170228_101828-anatol 04/01/2014
    task: ffff9791fd809140 task.stack: ffffacba402bc000
    RIP: 0010:__key_link_begin+0x0/0x100
    RSP: 0018:ffffacba402bfdc8 EFLAGS: 00010282
    RAX: ffff9791fd809140 RBX: fffffffffd39a620 RCX: 0000000000000008
    RDX: ffffacba402bfdd0 RSI: fffffffffd39a6a0 RDI: ffff9791fd810600
    RBP: ffffacba402bfdf8 R08: 0000000000000063 R09: ffffffff94845620
    R10: 8080808080808080 R11: 0000000000000004 R12: ffff9791fd810600
    R13: ffff9791fd39a940 R14: fffffffffd39a6a0 R15: 0000000000000000
    FS:  00007fbf14a90740(0000) GS:ffff9791ffd00000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: fffffffffd39a6b0 CR3: 000000003b910003 CR4: 00000000003606e0
    Call Trace:
     ? key_link+0x28/0xb0
     ? search_process_keyrings+0x13/0x100
     request_key_and_link+0xcb/0x550
     ? keyring_instantiate+0x110/0x110
     ? key_default_cmp+0x20/0x20
     SyS_request_key+0xc0/0x160
     ? exit_to_usermode_loop+0x5e/0x80
     entry_SYSCALL_64_fastpath+0x1a/0xa5
    RIP: 0033:0x7fbf14190bb9
    RSP: 002b:00007ffd8e4fe6c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000f9
    RAX: ffffffffffffffda RBX: 0000000036cc28fb RCX: 00007fbf14190bb9
    RDX: 000055748b56ca4a RSI: 000055748b56ca46 RDI: 000055748b56ca4b
    RBP: 000055748b56ca4a R08: 0000000000000001 R09: 0000000000000001
    R10: 0000000036cc28fb R11: 0000000000000246 R12: 000055748b56c8b0
    R13: 00007ffd8e4fe7d0 R14: 0000000000000000 R15: 0000000000000000
    Code: c5 0f 85 69 ff ff ff 48 c7 c3 82 ff ff ff eb ab 45 31 ed e9 18 ff ff ff 85 c0 75 8d eb d2 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 <48> 83 7e 10 00 0f 84 c5 00 00 00 55 48 89 e5 41 57 41 56 41 55
    RIP: __key_link_begin+0x0/0x100 RSP: ffffacba402bfdc8
    CR2: fffffffffd39a6b0

Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
Cc: <stable@vger.kernel.org>	[v4.4+]
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/key.h         | 12 +++++++++++-
 security/keys/key.c         | 26 +++++++++++++++++++-------
 security/keys/keyctl.c      |  3 +++
 security/keys/keyring.c     |  4 ++--
 security/keys/request_key.c | 11 +++++++----
 5 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index e315e16b6ff8..b7b590d7c480 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -189,6 +189,17 @@ struct key {
 #define KEY_FLAG_KEEP		10	/* set if key should not be removed */
 #define KEY_FLAG_UID_KEYRING	11	/* set if key is a user or user session keyring */
 
+	/*
+	 * If the key is negatively instantiated, then bits 20-31 hold the error
+	 * code which should be returned when someone tries to use the key
+	 * (unless they allow negative keys).  The error code is stored as a
+	 * positive number, so it must be negated before being returned.
+	 *
+	 * Note that a key can go from negative to positive but not vice versa.
+	 */
+#define KEY_FLAGS_REJECT_ERROR_SHIFT	20
+#define KEY_FLAGS_REJECT_ERROR_MASK	0xFFF00000
+
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
 	 * - it should be a printable string
@@ -213,7 +224,6 @@ struct key {
 			struct list_head name_link;
 			struct assoc_array keys;
 		};
-		int reject_error;
 	};
 
 	/* This is set on a keyring to restrict the addition of a link to a key
diff --git a/security/keys/key.c b/security/keys/key.c
index eb914a838840..3ffb6829972f 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -401,6 +401,20 @@ int key_payload_reserve(struct key *key, size_t datalen)
 }
 EXPORT_SYMBOL(key_payload_reserve);
 
+static void mark_key_instantiated(struct key *key, unsigned int reject_error)
+{
+	unsigned long old, new;
+
+	do {
+		old = READ_ONCE(key->flags);
+		new = (old & ~(KEY_FLAG_NEGATIVE |
+			       KEY_FLAGS_REJECT_ERROR_MASK)) |
+		      KEY_FLAG_INSTANTIATED |
+		      (reject_error ? KEY_FLAG_NEGATIVE : 0) |
+		      (reject_error << KEY_FLAGS_REJECT_ERROR_SHIFT);
+	} while (cmpxchg_release(&key->flags, old, new) != old);
+}
+
 /*
  * Instantiate a key and link it into the target keyring atomically.  Must be
  * called with the target keyring's semaphore writelocked.  The target key's
@@ -431,7 +445,7 @@ static int __key_instantiate_and_link(struct key *key,
 		if (ret == 0) {
 			/* mark the key as being instantiated */
 			atomic_inc(&key->user->nikeys);
-			set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+			mark_key_instantiated(key, 0);
 
 			if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
 				awaken = 1;
@@ -580,10 +594,8 @@ int key_reject_and_link(struct key *key,
 	if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
 		/* mark the key as being negatively instantiated */
 		atomic_inc(&key->user->nikeys);
-		key->reject_error = -error;
-		smp_wmb();
-		set_bit(KEY_FLAG_NEGATIVE, &key->flags);
-		set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+		mark_key_instantiated(key, error);
+
 		now = current_kernel_time();
 		key->expiry = now.tv_sec + timeout;
 		key_schedule_gc(key->expiry + key_gc_delay);
@@ -753,7 +765,7 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
 	ret = key->type->update(key, prep);
 	if (ret == 0)
 		/* updating a negative key instantiates it */
-		clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
@@ -987,7 +999,7 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 	ret = key->type->update(key, &prep);
 	if (ret == 0)
 		/* updating a negative key instantiates it */
-		clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 365ff85d7e27..19a09e121089 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1223,6 +1223,9 @@ long keyctl_reject_key(key_serial_t id, unsigned timeout, unsigned error,
 	    error == ERESTART_RESTARTBLOCK)
 		return -EINVAL;
 
+	BUILD_BUG_ON(MAX_ERRNO > (KEY_FLAGS_REJECT_ERROR_MASK >>
+				  KEY_FLAGS_REJECT_ERROR_SHIFT));
+
 	/* the appropriate instantiation authorisation key must have been
 	 * assumed before calling this */
 	ret = -EPERM;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..7fc661f492d3 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -598,8 +598,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
 		/* we set a different error code if we pass a negative key */
 		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
-			smp_rmb();
-			ctx->result = ERR_PTR(key->reject_error);
+			ctx->result = ERR_PTR(-(int)(kflags >>
+						KEY_FLAGS_REJECT_ERROR_SHIFT));
 			kleave(" = %d [neg]", ctx->skipped_ret);
 			goto skipped;
 		}
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 63e63a42db3c..0aab68344837 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -590,15 +590,18 @@ struct key *request_key_and_link(struct key_type *type,
 int wait_for_key_construction(struct key *key, bool intr)
 {
 	int ret;
+	unsigned long flags;
 
 	ret = wait_on_bit(&key->flags, KEY_FLAG_USER_CONSTRUCT,
 			  intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
 	if (ret)
 		return -ERESTARTSYS;
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
-		smp_rmb();
-		return key->reject_error;
-	}
+
+	/* Pairs with RELEASE in mark_key_instantiated() */
+	flags = smp_load_acquire(&key->flags);
+	if (flags & (1 << KEY_FLAG_NEGATIVE))
+		return -(int)(flags >> KEY_FLAGS_REJECT_ERROR_SHIFT);
+
 	return key_validate(key);
 }
 EXPORT_SYMBOL(wait_for_key_construction);
-- 
2.14.1.992.g2c7b836f3a-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 2/6] KEYS: load key flags atomically in key_is_instantiated()
  2017-09-26 20:10 ` Eric Biggers
  (?)
@ 2017-09-26 20:11   ` Eric Biggers
  -1 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

In key_is_instantiated(), we check for KEY_FLAG_INSTANTIATED set and
KEY_FLAG_NEGATIVE unset.  But this was done as two separate bit tests
which were not atomic with respect to each other, and had no memory
barrier providing ordering.  Therefore, it was theoretically possible
for the function to incorrectly return true if called while the key was
being negatively instantiated.

There also needs to be a memory barrier before anything which is only
meaningful for positively instantiated keys, e.g. ->payload and
->datalen, can be read --- which some of the ->describe() methods do.

Fix both these problems by loading the flags using smp_load_acquire().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/key.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index b7b590d7c480..fcb79eedbdb5 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -372,8 +372,11 @@ extern void key_set_timeout(struct key *, unsigned);
  */
 static inline bool key_is_instantiated(const struct key *key)
 {
-	return test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
-		!test_bit(KEY_FLAG_NEGATIVE, &key->flags);
+	/* Pairs with RELEASE in mark_key_instantiated() */
+	unsigned long flags = smp_load_acquire(&key->flags);
+
+	return (flags & KEY_FLAG_INSTANTIATED) &&
+		!(flags & KEY_FLAG_NEGATIVE);
 }
 
 #define dereference_key_rcu(KEY)					\
-- 
2.14.1.992.g2c7b836f3a-goog


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 2/6] KEYS: load key flags atomically in key_is_instantiated()
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: keyrings
  Cc: David Howells, Michael Halcrow, linux-security-module,
	linux-kernel, Eric Biggers

From: Eric Biggers <ebiggers@google.com>

In key_is_instantiated(), we check for KEY_FLAG_INSTANTIATED set and
KEY_FLAG_NEGATIVE unset.  But this was done as two separate bit tests
which were not atomic with respect to each other, and had no memory
barrier providing ordering.  Therefore, it was theoretically possible
for the function to incorrectly return true if called while the key was
being negatively instantiated.

There also needs to be a memory barrier before anything which is only
meaningful for positively instantiated keys, e.g. ->payload and
->datalen, can be read --- which some of the ->describe() methods do.

Fix both these problems by loading the flags using smp_load_acquire().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/key.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index b7b590d7c480..fcb79eedbdb5 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -372,8 +372,11 @@ extern void key_set_timeout(struct key *, unsigned);
  */
 static inline bool key_is_instantiated(const struct key *key)
 {
-	return test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
-		!test_bit(KEY_FLAG_NEGATIVE, &key->flags);
+	/* Pairs with RELEASE in mark_key_instantiated() */
+	unsigned long flags = smp_load_acquire(&key->flags);
+
+	return (flags & KEY_FLAG_INSTANTIATED) &&
+		!(flags & KEY_FLAG_NEGATIVE);
 }
 
 #define dereference_key_rcu(KEY)					\
-- 
2.14.1.992.g2c7b836f3a-goog

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 2/6] KEYS: load key flags atomically in key_is_instantiated()
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

In key_is_instantiated(), we check for KEY_FLAG_INSTANTIATED set and
KEY_FLAG_NEGATIVE unset.  But this was done as two separate bit tests
which were not atomic with respect to each other, and had no memory
barrier providing ordering.  Therefore, it was theoretically possible
for the function to incorrectly return true if called while the key was
being negatively instantiated.

There also needs to be a memory barrier before anything which is only
meaningful for positively instantiated keys, e.g. ->payload and
->datalen, can be read --- which some of the ->describe() methods do.

Fix both these problems by loading the flags using smp_load_acquire().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/key.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index b7b590d7c480..fcb79eedbdb5 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -372,8 +372,11 @@ extern void key_set_timeout(struct key *, unsigned);
  */
 static inline bool key_is_instantiated(const struct key *key)
 {
-	return test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
-		!test_bit(KEY_FLAG_NEGATIVE, &key->flags);
+	/* Pairs with RELEASE in mark_key_instantiated() */
+	unsigned long flags = smp_load_acquire(&key->flags);
+
+	return (flags & KEY_FLAG_INSTANTIATED) &&
+		!(flags & KEY_FLAG_NEGATIVE);
 }
 
 #define dereference_key_rcu(KEY)					\
-- 
2.14.1.992.g2c7b836f3a-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 3/6] KEYS: load key flags and expiry time atomically in key_validate()
  2017-09-26 20:10 ` Eric Biggers
  (?)
@ 2017-09-26 20:11   ` Eric Biggers
  -1 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

In key_validate(), load the flags and expiry time once atomically, since
these can change concurrently if key_validate() is called without the
key semaphore held.  And we don't want to get inconsistent results if a
variable is referenced multiple times.  For example, key->expiry was
referenced in both 'if (key->expiry)' and in 'if (now.tv_sec >key->expiry)', making it theoretically possible to see a spurious
EKEYEXPIRED while the expiration time was being removed, i.e. set to 0.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/permission.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/security/keys/permission.c b/security/keys/permission.c
index 732cc0beffdf..a72b4dd70c8a 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -88,7 +88,8 @@ EXPORT_SYMBOL(key_task_permission);
  */
 int key_validate(const struct key *key)
 {
-	unsigned long flags = key->flags;
+	unsigned long flags = READ_ONCE(key->flags);
+	time_t expiry = READ_ONCE(key->expiry);
 
 	if (flags & (1 << KEY_FLAG_INVALIDATED))
 		return -ENOKEY;
@@ -99,9 +100,9 @@ int key_validate(const struct key *key)
 		return -EKEYREVOKED;
 
 	/* check it hasn't expired */
-	if (key->expiry) {
+	if (expiry) {
 		struct timespec now = current_kernel_time();
-		if (now.tv_sec >= key->expiry)
+		if (now.tv_sec >= expiry)
 			return -EKEYEXPIRED;
 	}
 
-- 
2.14.1.992.g2c7b836f3a-goog


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 3/6] KEYS: load key flags and expiry time atomically in key_validate()
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: keyrings
  Cc: David Howells, Michael Halcrow, linux-security-module,
	linux-kernel, Eric Biggers

From: Eric Biggers <ebiggers@google.com>

In key_validate(), load the flags and expiry time once atomically, since
these can change concurrently if key_validate() is called without the
key semaphore held.  And we don't want to get inconsistent results if a
variable is referenced multiple times.  For example, key->expiry was
referenced in both 'if (key->expiry)' and in 'if (now.tv_sec >=
key->expiry)', making it theoretically possible to see a spurious
EKEYEXPIRED while the expiration time was being removed, i.e. set to 0.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/permission.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/security/keys/permission.c b/security/keys/permission.c
index 732cc0beffdf..a72b4dd70c8a 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -88,7 +88,8 @@ EXPORT_SYMBOL(key_task_permission);
  */
 int key_validate(const struct key *key)
 {
-	unsigned long flags = key->flags;
+	unsigned long flags = READ_ONCE(key->flags);
+	time_t expiry = READ_ONCE(key->expiry);
 
 	if (flags & (1 << KEY_FLAG_INVALIDATED))
 		return -ENOKEY;
@@ -99,9 +100,9 @@ int key_validate(const struct key *key)
 		return -EKEYREVOKED;
 
 	/* check it hasn't expired */
-	if (key->expiry) {
+	if (expiry) {
 		struct timespec now = current_kernel_time();
-		if (now.tv_sec >= key->expiry)
+		if (now.tv_sec >= expiry)
 			return -EKEYEXPIRED;
 	}
 
-- 
2.14.1.992.g2c7b836f3a-goog

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 3/6] KEYS: load key flags and expiry time atomically in key_validate()
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

In key_validate(), load the flags and expiry time once atomically, since
these can change concurrently if key_validate() is called without the
key semaphore held.  And we don't want to get inconsistent results if a
variable is referenced multiple times.  For example, key->expiry was
referenced in both 'if (key->expiry)' and in 'if (now.tv_sec >=
key->expiry)', making it theoretically possible to see a spurious
EKEYEXPIRED while the expiration time was being removed, i.e. set to 0.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/permission.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/security/keys/permission.c b/security/keys/permission.c
index 732cc0beffdf..a72b4dd70c8a 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -88,7 +88,8 @@ EXPORT_SYMBOL(key_task_permission);
  */
 int key_validate(const struct key *key)
 {
-	unsigned long flags = key->flags;
+	unsigned long flags = READ_ONCE(key->flags);
+	time_t expiry = READ_ONCE(key->expiry);
 
 	if (flags & (1 << KEY_FLAG_INVALIDATED))
 		return -ENOKEY;
@@ -99,9 +100,9 @@ int key_validate(const struct key *key)
 		return -EKEYREVOKED;
 
 	/* check it hasn't expired */
-	if (key->expiry) {
+	if (expiry) {
 		struct timespec now = current_kernel_time();
-		if (now.tv_sec >= key->expiry)
+		if (now.tv_sec >= expiry)
 			return -EKEYEXPIRED;
 	}
 
-- 
2.14.1.992.g2c7b836f3a-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 4/6] KEYS: load key flags and expiry time atomically in keyring_search_iterator()
  2017-09-26 20:10 ` Eric Biggers
  (?)
@ 2017-09-26 20:11   ` Eric Biggers
  -1 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

Similar to the case for key_validate(), we should load the key ->flags
and ->expiry once atomically in keyring_search_iterator(), since they
can be changed concurrently whenever the key semaphore isn't held.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/keyring.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 7fc661f492d3..1dfff0eac474 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -553,7 +553,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 {
 	struct keyring_search_context *ctx = iterator_data;
 	const struct key *key = keyring_ptr_to_key(object);
-	unsigned long kflags = key->flags;
+	unsigned long kflags = READ_ONCE(key->flags);
 
 	kenter("{%d}", key->serial);
 
@@ -565,6 +565,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 
 	/* skip invalidated, revoked and expired keys */
 	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
+		time_t expiry = READ_ONCE(key->expiry);
+
 		if (kflags & ((1 << KEY_FLAG_INVALIDATED) |
 			      (1 << KEY_FLAG_REVOKED))) {
 			ctx->result = ERR_PTR(-EKEYREVOKED);
@@ -572,7 +574,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 			goto skipped;
 		}
 
-		if (key->expiry && ctx->now.tv_sec >= key->expiry) {
+		if (expiry && ctx->now.tv_sec >= expiry) {
 			if (!(ctx->flags & KEYRING_SEARCH_SKIP_EXPIRED))
 				ctx->result = ERR_PTR(-EKEYEXPIRED);
 			kleave(" = %d [expire]", ctx->skipped_ret);
-- 
2.14.1.992.g2c7b836f3a-goog


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 4/6] KEYS: load key flags and expiry time atomically in keyring_search_iterator()
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: keyrings
  Cc: David Howells, Michael Halcrow, linux-security-module,
	linux-kernel, Eric Biggers

From: Eric Biggers <ebiggers@google.com>

Similar to the case for key_validate(), we should load the key ->flags
and ->expiry once atomically in keyring_search_iterator(), since they
can be changed concurrently whenever the key semaphore isn't held.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/keyring.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 7fc661f492d3..1dfff0eac474 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -553,7 +553,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 {
 	struct keyring_search_context *ctx = iterator_data;
 	const struct key *key = keyring_ptr_to_key(object);
-	unsigned long kflags = key->flags;
+	unsigned long kflags = READ_ONCE(key->flags);
 
 	kenter("{%d}", key->serial);
 
@@ -565,6 +565,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 
 	/* skip invalidated, revoked and expired keys */
 	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
+		time_t expiry = READ_ONCE(key->expiry);
+
 		if (kflags & ((1 << KEY_FLAG_INVALIDATED) |
 			      (1 << KEY_FLAG_REVOKED))) {
 			ctx->result = ERR_PTR(-EKEYREVOKED);
@@ -572,7 +574,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 			goto skipped;
 		}
 
-		if (key->expiry && ctx->now.tv_sec >= key->expiry) {
+		if (expiry && ctx->now.tv_sec >= expiry) {
 			if (!(ctx->flags & KEYRING_SEARCH_SKIP_EXPIRED))
 				ctx->result = ERR_PTR(-EKEYEXPIRED);
 			kleave(" = %d [expire]", ctx->skipped_ret);
-- 
2.14.1.992.g2c7b836f3a-goog

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 4/6] KEYS: load key flags and expiry time atomically in keyring_search_iterator()
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

Similar to the case for key_validate(), we should load the key ->flags
and ->expiry once atomically in keyring_search_iterator(), since they
can be changed concurrently whenever the key semaphore isn't held.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/keyring.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 7fc661f492d3..1dfff0eac474 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -553,7 +553,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 {
 	struct keyring_search_context *ctx = iterator_data;
 	const struct key *key = keyring_ptr_to_key(object);
-	unsigned long kflags = key->flags;
+	unsigned long kflags = READ_ONCE(key->flags);
 
 	kenter("{%d}", key->serial);
 
@@ -565,6 +565,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 
 	/* skip invalidated, revoked and expired keys */
 	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
+		time_t expiry = READ_ONCE(key->expiry);
+
 		if (kflags & ((1 << KEY_FLAG_INVALIDATED) |
 			      (1 << KEY_FLAG_REVOKED))) {
 			ctx->result = ERR_PTR(-EKEYREVOKED);
@@ -572,7 +574,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 			goto skipped;
 		}
 
-		if (key->expiry && ctx->now.tv_sec >= key->expiry) {
+		if (expiry && ctx->now.tv_sec >= expiry) {
 			if (!(ctx->flags & KEYRING_SEARCH_SKIP_EXPIRED))
 				ctx->result = ERR_PTR(-EKEYEXPIRED);
 			kleave(" = %d [expire]", ctx->skipped_ret);
-- 
2.14.1.992.g2c7b836f3a-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 5/6] KEYS: load key flags and expiry time atomically in proc_keys_show()
  2017-09-26 20:10 ` Eric Biggers
  (?)
@ 2017-09-26 20:11   ` Eric Biggers
  -1 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

In proc_keys_show(), the key semaphore is not held, so the key ->flags
and ->expiry can be changed concurrently.  We therefore should read them
atomically just once.  Otherwise /proc/keys may show an inconsistent
state, such a key that is negative ('N') but not instantiated ('I').

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/proc.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/security/keys/proc.c b/security/keys/proc.c
index de834309d100..a038069ac46a 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -179,7 +179,9 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	struct rb_node *_p = v;
 	struct key *key = rb_entry(_p, struct key, serial_node);
 	struct timespec now;
+	time_t expiry;
 	unsigned long timo;
+	unsigned long flags;
 	key_ref_t key_ref, skey_ref;
 	char xbuf[16];
 	int rc;
@@ -217,12 +219,13 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	rcu_read_lock();
 
 	/* come up with a suitable timeout value */
-	if (key->expiry = 0) {
+	expiry = READ_ONCE(key->expiry);
+	if (expiry = 0) {
 		memcpy(xbuf, "perm", 5);
-	} else if (now.tv_sec >= key->expiry) {
+	} else if (now.tv_sec >= expiry) {
 		memcpy(xbuf, "expd", 5);
 	} else {
-		timo = key->expiry - now.tv_sec;
+		timo = expiry - now.tv_sec;
 
 		if (timo < 60)
 			sprintf(xbuf, "%lus", timo);
@@ -236,18 +239,19 @@ static int proc_keys_show(struct seq_file *m, void *v)
 			sprintf(xbuf, "%luw", timo / (60*60*24*7));
 	}
 
-#define showflag(KEY, LETTER, FLAG) \
-	(test_bit(FLAG,	&(KEY)->flags) ? LETTER : '-')
+#define showflag(FLAGS, LETTER, FLAG) \
+	((FLAGS & (1 << FLAG)) ? LETTER : '-')
 
+	flags = READ_ONCE(key->flags);
 	seq_printf(m, "%08x %c%c%c%c%c%c%c %5d %4s %08x %5d %5d %-9.9s ",
 		   key->serial,
-		   showflag(key, 'I', KEY_FLAG_INSTANTIATED),
-		   showflag(key, 'R', KEY_FLAG_REVOKED),
-		   showflag(key, 'D', KEY_FLAG_DEAD),
-		   showflag(key, 'Q', KEY_FLAG_IN_QUOTA),
-		   showflag(key, 'U', KEY_FLAG_USER_CONSTRUCT),
-		   showflag(key, 'N', KEY_FLAG_NEGATIVE),
-		   showflag(key, 'i', KEY_FLAG_INVALIDATED),
+		   showflag(flags, 'I', KEY_FLAG_INSTANTIATED),
+		   showflag(flags, 'R', KEY_FLAG_REVOKED),
+		   showflag(flags, 'D', KEY_FLAG_DEAD),
+		   showflag(flags, 'Q', KEY_FLAG_IN_QUOTA),
+		   showflag(flags, 'U', KEY_FLAG_USER_CONSTRUCT),
+		   showflag(flags, 'N', KEY_FLAG_NEGATIVE),
+		   showflag(flags, 'i', KEY_FLAG_INVALIDATED),
 		   refcount_read(&key->usage),
 		   xbuf,
 		   key->perm,
-- 
2.14.1.992.g2c7b836f3a-goog


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 5/6] KEYS: load key flags and expiry time atomically in proc_keys_show()
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: keyrings
  Cc: David Howells, Michael Halcrow, linux-security-module,
	linux-kernel, Eric Biggers

From: Eric Biggers <ebiggers@google.com>

In proc_keys_show(), the key semaphore is not held, so the key ->flags
and ->expiry can be changed concurrently.  We therefore should read them
atomically just once.  Otherwise /proc/keys may show an inconsistent
state, such a key that is negative ('N') but not instantiated ('I').

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/proc.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/security/keys/proc.c b/security/keys/proc.c
index de834309d100..a038069ac46a 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -179,7 +179,9 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	struct rb_node *_p = v;
 	struct key *key = rb_entry(_p, struct key, serial_node);
 	struct timespec now;
+	time_t expiry;
 	unsigned long timo;
+	unsigned long flags;
 	key_ref_t key_ref, skey_ref;
 	char xbuf[16];
 	int rc;
@@ -217,12 +219,13 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	rcu_read_lock();
 
 	/* come up with a suitable timeout value */
-	if (key->expiry == 0) {
+	expiry = READ_ONCE(key->expiry);
+	if (expiry == 0) {
 		memcpy(xbuf, "perm", 5);
-	} else if (now.tv_sec >= key->expiry) {
+	} else if (now.tv_sec >= expiry) {
 		memcpy(xbuf, "expd", 5);
 	} else {
-		timo = key->expiry - now.tv_sec;
+		timo = expiry - now.tv_sec;
 
 		if (timo < 60)
 			sprintf(xbuf, "%lus", timo);
@@ -236,18 +239,19 @@ static int proc_keys_show(struct seq_file *m, void *v)
 			sprintf(xbuf, "%luw", timo / (60*60*24*7));
 	}
 
-#define showflag(KEY, LETTER, FLAG) \
-	(test_bit(FLAG,	&(KEY)->flags) ? LETTER : '-')
+#define showflag(FLAGS, LETTER, FLAG) \
+	((FLAGS & (1 << FLAG)) ? LETTER : '-')
 
+	flags = READ_ONCE(key->flags);
 	seq_printf(m, "%08x %c%c%c%c%c%c%c %5d %4s %08x %5d %5d %-9.9s ",
 		   key->serial,
-		   showflag(key, 'I', KEY_FLAG_INSTANTIATED),
-		   showflag(key, 'R', KEY_FLAG_REVOKED),
-		   showflag(key, 'D', KEY_FLAG_DEAD),
-		   showflag(key, 'Q', KEY_FLAG_IN_QUOTA),
-		   showflag(key, 'U', KEY_FLAG_USER_CONSTRUCT),
-		   showflag(key, 'N', KEY_FLAG_NEGATIVE),
-		   showflag(key, 'i', KEY_FLAG_INVALIDATED),
+		   showflag(flags, 'I', KEY_FLAG_INSTANTIATED),
+		   showflag(flags, 'R', KEY_FLAG_REVOKED),
+		   showflag(flags, 'D', KEY_FLAG_DEAD),
+		   showflag(flags, 'Q', KEY_FLAG_IN_QUOTA),
+		   showflag(flags, 'U', KEY_FLAG_USER_CONSTRUCT),
+		   showflag(flags, 'N', KEY_FLAG_NEGATIVE),
+		   showflag(flags, 'i', KEY_FLAG_INVALIDATED),
 		   refcount_read(&key->usage),
 		   xbuf,
 		   key->perm,
-- 
2.14.1.992.g2c7b836f3a-goog

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 5/6] KEYS: load key flags and expiry time atomically in proc_keys_show()
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

In proc_keys_show(), the key semaphore is not held, so the key ->flags
and ->expiry can be changed concurrently.  We therefore should read them
atomically just once.  Otherwise /proc/keys may show an inconsistent
state, such a key that is negative ('N') but not instantiated ('I').

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/proc.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/security/keys/proc.c b/security/keys/proc.c
index de834309d100..a038069ac46a 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -179,7 +179,9 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	struct rb_node *_p = v;
 	struct key *key = rb_entry(_p, struct key, serial_node);
 	struct timespec now;
+	time_t expiry;
 	unsigned long timo;
+	unsigned long flags;
 	key_ref_t key_ref, skey_ref;
 	char xbuf[16];
 	int rc;
@@ -217,12 +219,13 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	rcu_read_lock();
 
 	/* come up with a suitable timeout value */
-	if (key->expiry == 0) {
+	expiry = READ_ONCE(key->expiry);
+	if (expiry == 0) {
 		memcpy(xbuf, "perm", 5);
-	} else if (now.tv_sec >= key->expiry) {
+	} else if (now.tv_sec >= expiry) {
 		memcpy(xbuf, "expd", 5);
 	} else {
-		timo = key->expiry - now.tv_sec;
+		timo = expiry - now.tv_sec;
 
 		if (timo < 60)
 			sprintf(xbuf, "%lus", timo);
@@ -236,18 +239,19 @@ static int proc_keys_show(struct seq_file *m, void *v)
 			sprintf(xbuf, "%luw", timo / (60*60*24*7));
 	}
 
-#define showflag(KEY, LETTER, FLAG) \
-	(test_bit(FLAG,	&(KEY)->flags) ? LETTER : '-')
+#define showflag(FLAGS, LETTER, FLAG) \
+	((FLAGS & (1 << FLAG)) ? LETTER : '-')
 
+	flags = READ_ONCE(key->flags);
 	seq_printf(m, "%08x %c%c%c%c%c%c%c %5d %4s %08x %5d %5d %-9.9s ",
 		   key->serial,
-		   showflag(key, 'I', KEY_FLAG_INSTANTIATED),
-		   showflag(key, 'R', KEY_FLAG_REVOKED),
-		   showflag(key, 'D', KEY_FLAG_DEAD),
-		   showflag(key, 'Q', KEY_FLAG_IN_QUOTA),
-		   showflag(key, 'U', KEY_FLAG_USER_CONSTRUCT),
-		   showflag(key, 'N', KEY_FLAG_NEGATIVE),
-		   showflag(key, 'i', KEY_FLAG_INVALIDATED),
+		   showflag(flags, 'I', KEY_FLAG_INSTANTIATED),
+		   showflag(flags, 'R', KEY_FLAG_REVOKED),
+		   showflag(flags, 'D', KEY_FLAG_DEAD),
+		   showflag(flags, 'Q', KEY_FLAG_IN_QUOTA),
+		   showflag(flags, 'U', KEY_FLAG_USER_CONSTRUCT),
+		   showflag(flags, 'N', KEY_FLAG_NEGATIVE),
+		   showflag(flags, 'i', KEY_FLAG_INVALIDATED),
 		   refcount_read(&key->usage),
 		   xbuf,
 		   key->perm,
-- 
2.14.1.992.g2c7b836f3a-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 6/6] KEYS: remove KEY_FLAG_NEGATIVE
  2017-09-26 20:10 ` Eric Biggers
  (?)
@ 2017-09-26 20:11   ` Eric Biggers
  -1 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

Now that a key's reject_error is stored in the flags word, we can check
for nonzero reject_error rather than for KEY_FLAG_NEGATIVE.  Do this,
then remove KEY_FLAG_NEGATIVE.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/key.h                      | 20 ++++++++++++--------
 security/keys/encrypted-keys/encrypted.c |  2 +-
 security/keys/gc.c                       |  4 +---
 security/keys/key.c                      |  4 +---
 security/keys/keyctl.c                   |  2 +-
 security/keys/keyring.c                  |  2 +-
 security/keys/proc.c                     |  2 +-
 security/keys/request_key.c              |  2 +-
 security/keys/trusted.c                  |  2 +-
 security/keys/user_defined.c             |  2 +-
 10 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index fcb79eedbdb5..ecae4c1e4375 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -181,13 +181,12 @@ struct key {
 #define KEY_FLAG_REVOKED	2	/* set if key had been revoked */
 #define KEY_FLAG_IN_QUOTA	3	/* set if key consumes quota */
 #define KEY_FLAG_USER_CONSTRUCT	4	/* set if key is being constructed in userspace */
-#define KEY_FLAG_NEGATIVE	5	/* set if key is negative */
-#define KEY_FLAG_ROOT_CAN_CLEAR	6	/* set if key can be cleared by root without permission */
-#define KEY_FLAG_INVALIDATED	7	/* set if key has been invalidated */
-#define KEY_FLAG_BUILTIN	8	/* set if key is built in to the kernel */
-#define KEY_FLAG_ROOT_CAN_INVAL	9	/* set if key can be invalidated by root without permission */
-#define KEY_FLAG_KEEP		10	/* set if key should not be removed */
-#define KEY_FLAG_UID_KEYRING	11	/* set if key is a user or user session keyring */
+#define KEY_FLAG_ROOT_CAN_CLEAR	5	/* set if key can be cleared by root without permission */
+#define KEY_FLAG_INVALIDATED	6	/* set if key has been invalidated */
+#define KEY_FLAG_BUILTIN	7	/* set if key is built in to the kernel */
+#define KEY_FLAG_ROOT_CAN_INVAL	8	/* set if key can be invalidated by root without permission */
+#define KEY_FLAG_KEEP		9	/* set if key should not be removed */
+#define KEY_FLAG_UID_KEYRING	10	/* set if key is a user or user session keyring */
 
 	/*
 	 * If the key is negatively instantiated, then bits 20-31 hold the error
@@ -376,7 +375,12 @@ static inline bool key_is_instantiated(const struct key *key)
 	unsigned long flags = smp_load_acquire(&key->flags);
 
 	return (flags & KEY_FLAG_INSTANTIATED) &&
-		!(flags & KEY_FLAG_NEGATIVE);
+		!(flags & KEY_FLAGS_REJECT_ERROR_MASK);
+}
+
+static inline bool key_is_negative(const struct key *key)
+{
+	return key->flags & KEY_FLAGS_REJECT_ERROR_MASK;
 }
 
 #define dereference_key_rcu(KEY)					\
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 69855ba0d3b3..f54b92868bc3 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -847,7 +847,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
 	size_t datalen = prep->datalen;
 	int ret = 0;
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_negative(key))
 		return -ENOKEY;
 	if (datalen <= 0 || datalen > 32767 || !prep->data)
 		return -EINVAL;
diff --git a/security/keys/gc.c b/security/keys/gc.c
index 87cb260e4890..0adc52be3ea9 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -135,9 +135,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 		key_check(key);
 
 		/* 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)
+		if (key_is_instantiated(key) && key->type->destroy)
 			key->type->destroy(key);
 
 		security_key_free(key);
diff --git a/security/keys/key.c b/security/keys/key.c
index 3ffb6829972f..990573a14666 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -407,10 +407,8 @@ static void mark_key_instantiated(struct key *key, unsigned int reject_error)
 
 	do {
 		old = READ_ONCE(key->flags);
-		new = (old & ~(KEY_FLAG_NEGATIVE |
-			       KEY_FLAGS_REJECT_ERROR_MASK)) |
+		new = (old & ~KEY_FLAGS_REJECT_ERROR_MASK) |
 		      KEY_FLAG_INSTANTIATED |
-		      (reject_error ? KEY_FLAG_NEGATIVE : 0) |
 		      (reject_error << KEY_FLAGS_REJECT_ERROR_SHIFT);
 	} while (cmpxchg_release(&key->flags, old, new) != old);
 }
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 19a09e121089..e90b352cc3bd 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -766,7 +766,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 
 	key = key_ref_to_ptr(key_ref);
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
+	if (key_is_negative(key)) {
 		ret = -ENOKEY;
 		goto error2;
 	}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 1dfff0eac474..16d21d0e5e45 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -599,7 +599,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 
 	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
 		/* we set a different error code if we pass a negative key */
-		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
+		if (kflags & KEY_FLAGS_REJECT_ERROR_MASK) {
 			ctx->result = ERR_PTR(-(int)(kflags >>
 						KEY_FLAGS_REJECT_ERROR_SHIFT));
 			kleave(" = %d [neg]", ctx->skipped_ret);
diff --git a/security/keys/proc.c b/security/keys/proc.c
index a038069ac46a..7d34e70f8aa1 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -250,7 +250,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
 		   showflag(flags, 'D', KEY_FLAG_DEAD),
 		   showflag(flags, 'Q', KEY_FLAG_IN_QUOTA),
 		   showflag(flags, 'U', KEY_FLAG_USER_CONSTRUCT),
-		   showflag(flags, 'N', KEY_FLAG_NEGATIVE),
+		   (flags & KEY_FLAGS_REJECT_ERROR_MASK) ? 'N' : '-',
 		   showflag(flags, 'i', KEY_FLAG_INVALIDATED),
 		   refcount_read(&key->usage),
 		   xbuf,
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 0aab68344837..1953ceb33efc 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -599,7 +599,7 @@ int wait_for_key_construction(struct key *key, bool intr)
 
 	/* Pairs with RELEASE in mark_key_instantiated() */
 	flags = smp_load_acquire(&key->flags);
-	if (flags & (1 << KEY_FLAG_NEGATIVE))
+	if (flags & KEY_FLAGS_REJECT_ERROR_MASK)
 		return -(int)(flags >> KEY_FLAGS_REJECT_ERROR_SHIFT);
 
 	return key_validate(key);
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ddfaebf60fc8..bd85315cbfeb 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -1066,7 +1066,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
 	char *datablob;
 	int ret = 0;
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_negative(key))
 		return -ENOKEY;
 	p = key->payload.data[0];
 	if (!p->migratable)
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 3d8c68eba516..a5506400836c 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -114,7 +114,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep)
 
 	/* attach the new data, displacing the old */
 	key->expiry = prep->expiry;
-	if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (!key_is_negative(key))
 		zap = dereference_key_locked(key);
 	rcu_assign_keypointer(key, prep->payload.data[0]);
 	prep->payload.data[0] = NULL;
-- 
2.14.1.992.g2c7b836f3a-goog


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 6/6] KEYS: remove KEY_FLAG_NEGATIVE
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: keyrings
  Cc: David Howells, Michael Halcrow, linux-security-module,
	linux-kernel, Eric Biggers

From: Eric Biggers <ebiggers@google.com>

Now that a key's reject_error is stored in the flags word, we can check
for nonzero reject_error rather than for KEY_FLAG_NEGATIVE.  Do this,
then remove KEY_FLAG_NEGATIVE.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/key.h                      | 20 ++++++++++++--------
 security/keys/encrypted-keys/encrypted.c |  2 +-
 security/keys/gc.c                       |  4 +---
 security/keys/key.c                      |  4 +---
 security/keys/keyctl.c                   |  2 +-
 security/keys/keyring.c                  |  2 +-
 security/keys/proc.c                     |  2 +-
 security/keys/request_key.c              |  2 +-
 security/keys/trusted.c                  |  2 +-
 security/keys/user_defined.c             |  2 +-
 10 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index fcb79eedbdb5..ecae4c1e4375 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -181,13 +181,12 @@ struct key {
 #define KEY_FLAG_REVOKED	2	/* set if key had been revoked */
 #define KEY_FLAG_IN_QUOTA	3	/* set if key consumes quota */
 #define KEY_FLAG_USER_CONSTRUCT	4	/* set if key is being constructed in userspace */
-#define KEY_FLAG_NEGATIVE	5	/* set if key is negative */
-#define KEY_FLAG_ROOT_CAN_CLEAR	6	/* set if key can be cleared by root without permission */
-#define KEY_FLAG_INVALIDATED	7	/* set if key has been invalidated */
-#define KEY_FLAG_BUILTIN	8	/* set if key is built in to the kernel */
-#define KEY_FLAG_ROOT_CAN_INVAL	9	/* set if key can be invalidated by root without permission */
-#define KEY_FLAG_KEEP		10	/* set if key should not be removed */
-#define KEY_FLAG_UID_KEYRING	11	/* set if key is a user or user session keyring */
+#define KEY_FLAG_ROOT_CAN_CLEAR	5	/* set if key can be cleared by root without permission */
+#define KEY_FLAG_INVALIDATED	6	/* set if key has been invalidated */
+#define KEY_FLAG_BUILTIN	7	/* set if key is built in to the kernel */
+#define KEY_FLAG_ROOT_CAN_INVAL	8	/* set if key can be invalidated by root without permission */
+#define KEY_FLAG_KEEP		9	/* set if key should not be removed */
+#define KEY_FLAG_UID_KEYRING	10	/* set if key is a user or user session keyring */
 
 	/*
 	 * If the key is negatively instantiated, then bits 20-31 hold the error
@@ -376,7 +375,12 @@ static inline bool key_is_instantiated(const struct key *key)
 	unsigned long flags = smp_load_acquire(&key->flags);
 
 	return (flags & KEY_FLAG_INSTANTIATED) &&
-		!(flags & KEY_FLAG_NEGATIVE);
+		!(flags & KEY_FLAGS_REJECT_ERROR_MASK);
+}
+
+static inline bool key_is_negative(const struct key *key)
+{
+	return key->flags & KEY_FLAGS_REJECT_ERROR_MASK;
 }
 
 #define dereference_key_rcu(KEY)					\
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 69855ba0d3b3..f54b92868bc3 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -847,7 +847,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
 	size_t datalen = prep->datalen;
 	int ret = 0;
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_negative(key))
 		return -ENOKEY;
 	if (datalen <= 0 || datalen > 32767 || !prep->data)
 		return -EINVAL;
diff --git a/security/keys/gc.c b/security/keys/gc.c
index 87cb260e4890..0adc52be3ea9 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -135,9 +135,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 		key_check(key);
 
 		/* 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)
+		if (key_is_instantiated(key) && key->type->destroy)
 			key->type->destroy(key);
 
 		security_key_free(key);
diff --git a/security/keys/key.c b/security/keys/key.c
index 3ffb6829972f..990573a14666 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -407,10 +407,8 @@ static void mark_key_instantiated(struct key *key, unsigned int reject_error)
 
 	do {
 		old = READ_ONCE(key->flags);
-		new = (old & ~(KEY_FLAG_NEGATIVE |
-			       KEY_FLAGS_REJECT_ERROR_MASK)) |
+		new = (old & ~KEY_FLAGS_REJECT_ERROR_MASK) |
 		      KEY_FLAG_INSTANTIATED |
-		      (reject_error ? KEY_FLAG_NEGATIVE : 0) |
 		      (reject_error << KEY_FLAGS_REJECT_ERROR_SHIFT);
 	} while (cmpxchg_release(&key->flags, old, new) != old);
 }
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 19a09e121089..e90b352cc3bd 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -766,7 +766,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 
 	key = key_ref_to_ptr(key_ref);
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
+	if (key_is_negative(key)) {
 		ret = -ENOKEY;
 		goto error2;
 	}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 1dfff0eac474..16d21d0e5e45 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -599,7 +599,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 
 	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
 		/* we set a different error code if we pass a negative key */
-		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
+		if (kflags & KEY_FLAGS_REJECT_ERROR_MASK) {
 			ctx->result = ERR_PTR(-(int)(kflags >>
 						KEY_FLAGS_REJECT_ERROR_SHIFT));
 			kleave(" = %d [neg]", ctx->skipped_ret);
diff --git a/security/keys/proc.c b/security/keys/proc.c
index a038069ac46a..7d34e70f8aa1 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -250,7 +250,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
 		   showflag(flags, 'D', KEY_FLAG_DEAD),
 		   showflag(flags, 'Q', KEY_FLAG_IN_QUOTA),
 		   showflag(flags, 'U', KEY_FLAG_USER_CONSTRUCT),
-		   showflag(flags, 'N', KEY_FLAG_NEGATIVE),
+		   (flags & KEY_FLAGS_REJECT_ERROR_MASK) ? 'N' : '-',
 		   showflag(flags, 'i', KEY_FLAG_INVALIDATED),
 		   refcount_read(&key->usage),
 		   xbuf,
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 0aab68344837..1953ceb33efc 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -599,7 +599,7 @@ int wait_for_key_construction(struct key *key, bool intr)
 
 	/* Pairs with RELEASE in mark_key_instantiated() */
 	flags = smp_load_acquire(&key->flags);
-	if (flags & (1 << KEY_FLAG_NEGATIVE))
+	if (flags & KEY_FLAGS_REJECT_ERROR_MASK)
 		return -(int)(flags >> KEY_FLAGS_REJECT_ERROR_SHIFT);
 
 	return key_validate(key);
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ddfaebf60fc8..bd85315cbfeb 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -1066,7 +1066,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
 	char *datablob;
 	int ret = 0;
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_negative(key))
 		return -ENOKEY;
 	p = key->payload.data[0];
 	if (!p->migratable)
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 3d8c68eba516..a5506400836c 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -114,7 +114,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep)
 
 	/* attach the new data, displacing the old */
 	key->expiry = prep->expiry;
-	if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (!key_is_negative(key))
 		zap = dereference_key_locked(key);
 	rcu_assign_keypointer(key, prep->payload.data[0]);
 	prep->payload.data[0] = NULL;
-- 
2.14.1.992.g2c7b836f3a-goog

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v2 6/6] KEYS: remove KEY_FLAG_NEGATIVE
@ 2017-09-26 20:11   ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:11 UTC (permalink / raw)
  To: linux-security-module

From: Eric Biggers <ebiggers@google.com>

Now that a key's reject_error is stored in the flags word, we can check
for nonzero reject_error rather than for KEY_FLAG_NEGATIVE.  Do this,
then remove KEY_FLAG_NEGATIVE.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/key.h                      | 20 ++++++++++++--------
 security/keys/encrypted-keys/encrypted.c |  2 +-
 security/keys/gc.c                       |  4 +---
 security/keys/key.c                      |  4 +---
 security/keys/keyctl.c                   |  2 +-
 security/keys/keyring.c                  |  2 +-
 security/keys/proc.c                     |  2 +-
 security/keys/request_key.c              |  2 +-
 security/keys/trusted.c                  |  2 +-
 security/keys/user_defined.c             |  2 +-
 10 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index fcb79eedbdb5..ecae4c1e4375 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -181,13 +181,12 @@ struct key {
 #define KEY_FLAG_REVOKED	2	/* set if key had been revoked */
 #define KEY_FLAG_IN_QUOTA	3	/* set if key consumes quota */
 #define KEY_FLAG_USER_CONSTRUCT	4	/* set if key is being constructed in userspace */
-#define KEY_FLAG_NEGATIVE	5	/* set if key is negative */
-#define KEY_FLAG_ROOT_CAN_CLEAR	6	/* set if key can be cleared by root without permission */
-#define KEY_FLAG_INVALIDATED	7	/* set if key has been invalidated */
-#define KEY_FLAG_BUILTIN	8	/* set if key is built in to the kernel */
-#define KEY_FLAG_ROOT_CAN_INVAL	9	/* set if key can be invalidated by root without permission */
-#define KEY_FLAG_KEEP		10	/* set if key should not be removed */
-#define KEY_FLAG_UID_KEYRING	11	/* set if key is a user or user session keyring */
+#define KEY_FLAG_ROOT_CAN_CLEAR	5	/* set if key can be cleared by root without permission */
+#define KEY_FLAG_INVALIDATED	6	/* set if key has been invalidated */
+#define KEY_FLAG_BUILTIN	7	/* set if key is built in to the kernel */
+#define KEY_FLAG_ROOT_CAN_INVAL	8	/* set if key can be invalidated by root without permission */
+#define KEY_FLAG_KEEP		9	/* set if key should not be removed */
+#define KEY_FLAG_UID_KEYRING	10	/* set if key is a user or user session keyring */
 
 	/*
 	 * If the key is negatively instantiated, then bits 20-31 hold the error
@@ -376,7 +375,12 @@ static inline bool key_is_instantiated(const struct key *key)
 	unsigned long flags = smp_load_acquire(&key->flags);
 
 	return (flags & KEY_FLAG_INSTANTIATED) &&
-		!(flags & KEY_FLAG_NEGATIVE);
+		!(flags & KEY_FLAGS_REJECT_ERROR_MASK);
+}
+
+static inline bool key_is_negative(const struct key *key)
+{
+	return key->flags & KEY_FLAGS_REJECT_ERROR_MASK;
 }
 
 #define dereference_key_rcu(KEY)					\
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 69855ba0d3b3..f54b92868bc3 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -847,7 +847,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
 	size_t datalen = prep->datalen;
 	int ret = 0;
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_negative(key))
 		return -ENOKEY;
 	if (datalen <= 0 || datalen > 32767 || !prep->data)
 		return -EINVAL;
diff --git a/security/keys/gc.c b/security/keys/gc.c
index 87cb260e4890..0adc52be3ea9 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -135,9 +135,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 		key_check(key);
 
 		/* 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)
+		if (key_is_instantiated(key) && key->type->destroy)
 			key->type->destroy(key);
 
 		security_key_free(key);
diff --git a/security/keys/key.c b/security/keys/key.c
index 3ffb6829972f..990573a14666 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -407,10 +407,8 @@ static void mark_key_instantiated(struct key *key, unsigned int reject_error)
 
 	do {
 		old = READ_ONCE(key->flags);
-		new = (old & ~(KEY_FLAG_NEGATIVE |
-			       KEY_FLAGS_REJECT_ERROR_MASK)) |
+		new = (old & ~KEY_FLAGS_REJECT_ERROR_MASK) |
 		      KEY_FLAG_INSTANTIATED |
-		      (reject_error ? KEY_FLAG_NEGATIVE : 0) |
 		      (reject_error << KEY_FLAGS_REJECT_ERROR_SHIFT);
 	} while (cmpxchg_release(&key->flags, old, new) != old);
 }
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 19a09e121089..e90b352cc3bd 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -766,7 +766,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 
 	key = key_ref_to_ptr(key_ref);
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
+	if (key_is_negative(key)) {
 		ret = -ENOKEY;
 		goto error2;
 	}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 1dfff0eac474..16d21d0e5e45 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -599,7 +599,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 
 	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
 		/* we set a different error code if we pass a negative key */
-		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
+		if (kflags & KEY_FLAGS_REJECT_ERROR_MASK) {
 			ctx->result = ERR_PTR(-(int)(kflags >>
 						KEY_FLAGS_REJECT_ERROR_SHIFT));
 			kleave(" = %d [neg]", ctx->skipped_ret);
diff --git a/security/keys/proc.c b/security/keys/proc.c
index a038069ac46a..7d34e70f8aa1 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -250,7 +250,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
 		   showflag(flags, 'D', KEY_FLAG_DEAD),
 		   showflag(flags, 'Q', KEY_FLAG_IN_QUOTA),
 		   showflag(flags, 'U', KEY_FLAG_USER_CONSTRUCT),
-		   showflag(flags, 'N', KEY_FLAG_NEGATIVE),
+		   (flags & KEY_FLAGS_REJECT_ERROR_MASK) ? 'N' : '-',
 		   showflag(flags, 'i', KEY_FLAG_INVALIDATED),
 		   refcount_read(&key->usage),
 		   xbuf,
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 0aab68344837..1953ceb33efc 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -599,7 +599,7 @@ int wait_for_key_construction(struct key *key, bool intr)
 
 	/* Pairs with RELEASE in mark_key_instantiated() */
 	flags = smp_load_acquire(&key->flags);
-	if (flags & (1 << KEY_FLAG_NEGATIVE))
+	if (flags & KEY_FLAGS_REJECT_ERROR_MASK)
 		return -(int)(flags >> KEY_FLAGS_REJECT_ERROR_SHIFT);
 
 	return key_validate(key);
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ddfaebf60fc8..bd85315cbfeb 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -1066,7 +1066,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
 	char *datablob;
 	int ret = 0;
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_negative(key))
 		return -ENOKEY;
 	p = key->payload.data[0];
 	if (!p->migratable)
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 3d8c68eba516..a5506400836c 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -114,7 +114,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep)
 
 	/* attach the new data, displacing the old */
 	key->expiry = prep->expiry;
-	if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (!key_is_negative(key))
 		zap = dereference_key_locked(key);
 	rcu_assign_keypointer(key, prep->payload.data[0]);
 	prep->payload.data[0] = NULL;
-- 
2.14.1.992.g2c7b836f3a-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/6] KEYS: fix race between updating and finding negative key
  2017-09-26 20:11   ` Eric Biggers
  (?)
@ 2017-09-26 20:39     ` Eric Biggers
  -1 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:39 UTC (permalink / raw)
  To: linux-security-module

On Tue, Sep 26, 2017 at 01:11:00PM -0700, Eric Biggers wrote:
> +static void mark_key_instantiated(struct key *key, unsigned int reject_error)
> +{
> +	unsigned long old, new;
> +
> +	do {
> +		old = READ_ONCE(key->flags);
> +		new = (old & ~(KEY_FLAG_NEGATIVE |
> +			       KEY_FLAGS_REJECT_ERROR_MASK)) |
> +		      KEY_FLAG_INSTANTIATED |
> +		      (reject_error ? KEY_FLAG_NEGATIVE : 0) |
> +		      (reject_error << KEY_FLAGS_REJECT_ERROR_SHIFT);
> +	} while (cmpxchg_release(&key->flags, old, new) != old);
> +}

Sorry, I realized I screwed this up --- the flags like KEY_FLAG_NEGATIVE need to
be (1 << KEY_FLAG_NEGATIVE).  I'll send another version which will be better
tested...

Eric

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/6] KEYS: fix race between updating and finding negative key
@ 2017-09-26 20:39     ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:39 UTC (permalink / raw)
  To: keyrings
  Cc: David Howells, Michael Halcrow, linux-security-module,
	linux-kernel, Eric Biggers, stable

On Tue, Sep 26, 2017 at 01:11:00PM -0700, Eric Biggers wrote:
> +static void mark_key_instantiated(struct key *key, unsigned int reject_error)
> +{
> +	unsigned long old, new;
> +
> +	do {
> +		old = READ_ONCE(key->flags);
> +		new = (old & ~(KEY_FLAG_NEGATIVE |
> +			       KEY_FLAGS_REJECT_ERROR_MASK)) |
> +		      KEY_FLAG_INSTANTIATED |
> +		      (reject_error ? KEY_FLAG_NEGATIVE : 0) |
> +		      (reject_error << KEY_FLAGS_REJECT_ERROR_SHIFT);
> +	} while (cmpxchg_release(&key->flags, old, new) != old);
> +}

Sorry, I realized I screwed this up --- the flags like KEY_FLAG_NEGATIVE need to
be (1 << KEY_FLAG_NEGATIVE).  I'll send another version which will be better
tested...

Eric

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v2 1/6] KEYS: fix race between updating and finding negative key
@ 2017-09-26 20:39     ` Eric Biggers
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Biggers @ 2017-09-26 20:39 UTC (permalink / raw)
  To: linux-security-module

On Tue, Sep 26, 2017 at 01:11:00PM -0700, Eric Biggers wrote:
> +static void mark_key_instantiated(struct key *key, unsigned int reject_error)
> +{
> +	unsigned long old, new;
> +
> +	do {
> +		old = READ_ONCE(key->flags);
> +		new = (old & ~(KEY_FLAG_NEGATIVE |
> +			       KEY_FLAGS_REJECT_ERROR_MASK)) |
> +		      KEY_FLAG_INSTANTIATED |
> +		      (reject_error ? KEY_FLAG_NEGATIVE : 0) |
> +		      (reject_error << KEY_FLAGS_REJECT_ERROR_SHIFT);
> +	} while (cmpxchg_release(&key->flags, old, new) != old);
> +}

Sorry, I realized I screwed this up --- the flags like KEY_FLAG_NEGATIVE need to
be (1 << KEY_FLAG_NEGATIVE).  I'll send another version which will be better
tested...

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-09-26 20:39 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-26 20:10 [PATCH v2 0/6] KEYS: fix atomicity issues with key flags Eric Biggers
2017-09-26 20:10 ` Eric Biggers
2017-09-26 20:10 ` Eric Biggers
2017-09-26 20:11 ` [PATCH v2 1/6] KEYS: fix race between updating and finding negative key Eric Biggers
2017-09-26 20:11   ` Eric Biggers
2017-09-26 20:11   ` Eric Biggers
2017-09-26 20:39   ` Eric Biggers
2017-09-26 20:39     ` Eric Biggers
2017-09-26 20:39     ` Eric Biggers
2017-09-26 20:11 ` [PATCH v2 2/6] KEYS: load key flags atomically in key_is_instantiated() Eric Biggers
2017-09-26 20:11   ` Eric Biggers
2017-09-26 20:11   ` Eric Biggers
2017-09-26 20:11 ` [PATCH v2 3/6] KEYS: load key flags and expiry time atomically in key_validate() Eric Biggers
2017-09-26 20:11   ` Eric Biggers
2017-09-26 20:11   ` Eric Biggers
2017-09-26 20:11 ` [PATCH v2 4/6] KEYS: load key flags and expiry time atomically in keyring_search_iterator() Eric Biggers
2017-09-26 20:11   ` Eric Biggers
2017-09-26 20:11   ` Eric Biggers
2017-09-26 20:11 ` [PATCH v2 5/6] KEYS: load key flags and expiry time atomically in proc_keys_show() Eric Biggers
2017-09-26 20:11   ` Eric Biggers
2017-09-26 20:11   ` Eric Biggers
2017-09-26 20:11 ` [PATCH v2 6/6] KEYS: remove KEY_FLAG_NEGATIVE Eric Biggers
2017-09-26 20:11   ` Eric Biggers
2017-09-26 20:11   ` Eric Biggers

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.