Re: [PATCH v3 2/7] KEYS: fix race between updating and finding negative key

2017-10-04 Thread David Howells
Eric Biggers  wrote:

> 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.

I would rather not eat up that much of the flags word.  I would rather
something like the attached patch, which allows both the NEGATIVE and
INSTANTIATED flags to be eliminated.

Changing the state member is always done under a write lock on the key
semaphore, and it might be possible to roll in the REVOKE and DEAD flags as
separate states also.

David
---
commit b23bda640e9a9bfe66bb3384d1b9db85ad701e04
Author: David Howells 
Date:   Wed Oct 4 16:43:25 2017 +0100

KEYS: Fix race between updating and finding a negative key

Consolidate KEY_FLAG_INSTANTIATED, KEY_FLAG_NEGATIVE and the rejection
error into one field such that:

 (1) The instantiation state can be modified/read atomically.

 (2) The error can be accessed atomically with the state.

 (3) The error isn't stored unioned with the payload pointers.

Possibly the revocation flags can also be combined with this also.

Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload 
data")
Cc: [v4.4+]
Reported-by: Eric Biggers 
Signed-off-by: David Howells 

diff --git a/include/linux/key.h b/include/linux/key.h
index e315e16b6ff8..f50e88c52bd0 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -138,6 +138,11 @@ struct key_restriction {
struct key_type *keytype;
 };
 
+enum key_state {
+   KEY_IS_UNINSTANTIATED,
+   KEY_IS_INSTANTIATED,
+};
+
 /*/
 /*
  * authentication token / access credential / keyring
@@ -169,6 +174,7 @@ struct key {
 * - may not match RCU 
dereferenced payload
 * - payload should contain own 
length
 */
+   short   state;  /* Key state (+) or rejection 
error (-) */
 
 #ifdef KEY_DEBUGGING
unsignedmagic;
@@ -176,18 +182,16 @@ struct key {
 #endif
 
unsigned long   flags;  /* status flags (change with 
bitops) */
-#define KEY_FLAG_INSTANTIATED  0   /* set if key has been instantiated */
-#define KEY_FLAG_DEAD  1   /* set if key type has been deleted */
-#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_CONSTRUCT4   /* set if key is being 
constructed in userspace */
-#define KEY_FLAG_NEGATIVE  5   /* set if key is negative */
-#define KEY_FLAG_ROOT_CAN_CLEAR6   /* 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_INVAL9   /* 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_DEAD  0   /* set if key type has been deleted */
+#define KEY_FLAG_REVOKED   1   /* set if key had been revoked */
+#define KEY_FLAG_IN_QUOTA  2   /* set if key consumes quota */
+#define KEY_FLAG_USER_CONSTRUCT3   /* set if key is being 
constructed in userspace */
+#define KEY_FLAG_ROOT_CAN_CLEAR4   /* set if key can be cleared by 
root without permission */
+#define KEY_FLAG_INVALIDATED   5   /* set if key has been invalidated */
+#define KEY_FLAG_BUILTIN   6   /* set if key is built in to the kernel 
*/
+#define KEY_FLAG_ROOT_CAN_INVAL7   /* set if key can be 
invalidated by root without permission */
+#define KEY_FLAG_KEEP  8   /* set if key should not be removed */
+#define KEY_FLAG_UID_KEYRING   9   /* set if key is a user or user session 
keyring */
 
/* the key type and key description string
 * - the desc is used to match a key against search criteria
@@ -213,7 +217,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
@@ -362,8 +365,12 @@ extern void key_set_timeout(struct key *, unsigned);
  */
 static inline bool key_is_instantiated(const struct key *key)
 {
-   return test_bit(KEY_FLAG_INSTANTIATED, >flags) &&
-   !test_bit(KEY_FLAG_NEGATIVE, 

Re: [PATCH v3 2/7] KEYS: fix race between updating and finding negative key

2017-10-04 Thread David Howells
Eric Biggers  wrote:

> 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.

I would rather not eat up that much of the flags word.  I would rather
something like the attached patch, which allows both the NEGATIVE and
INSTANTIATED flags to be eliminated.

Changing the state member is always done under a write lock on the key
semaphore, and it might be possible to roll in the REVOKE and DEAD flags as
separate states also.

David
---
commit b23bda640e9a9bfe66bb3384d1b9db85ad701e04
Author: David Howells 
Date:   Wed Oct 4 16:43:25 2017 +0100

KEYS: Fix race between updating and finding a negative key

Consolidate KEY_FLAG_INSTANTIATED, KEY_FLAG_NEGATIVE and the rejection
error into one field such that:

 (1) The instantiation state can be modified/read atomically.

 (2) The error can be accessed atomically with the state.

 (3) The error isn't stored unioned with the payload pointers.

Possibly the revocation flags can also be combined with this also.

Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload 
data")
Cc: [v4.4+]
Reported-by: Eric Biggers 
Signed-off-by: David Howells 

diff --git a/include/linux/key.h b/include/linux/key.h
index e315e16b6ff8..f50e88c52bd0 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -138,6 +138,11 @@ struct key_restriction {
struct key_type *keytype;
 };
 
+enum key_state {
+   KEY_IS_UNINSTANTIATED,
+   KEY_IS_INSTANTIATED,
+};
+
 /*/
 /*
  * authentication token / access credential / keyring
@@ -169,6 +174,7 @@ struct key {
 * - may not match RCU 
dereferenced payload
 * - payload should contain own 
length
 */
+   short   state;  /* Key state (+) or rejection 
error (-) */
 
 #ifdef KEY_DEBUGGING
unsignedmagic;
@@ -176,18 +182,16 @@ struct key {
 #endif
 
unsigned long   flags;  /* status flags (change with 
bitops) */
-#define KEY_FLAG_INSTANTIATED  0   /* set if key has been instantiated */
-#define KEY_FLAG_DEAD  1   /* set if key type has been deleted */
-#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_CONSTRUCT4   /* set if key is being 
constructed in userspace */
-#define KEY_FLAG_NEGATIVE  5   /* set if key is negative */
-#define KEY_FLAG_ROOT_CAN_CLEAR6   /* 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_INVAL9   /* 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_DEAD  0   /* set if key type has been deleted */
+#define KEY_FLAG_REVOKED   1   /* set if key had been revoked */
+#define KEY_FLAG_IN_QUOTA  2   /* set if key consumes quota */
+#define KEY_FLAG_USER_CONSTRUCT3   /* set if key is being 
constructed in userspace */
+#define KEY_FLAG_ROOT_CAN_CLEAR4   /* set if key can be cleared by 
root without permission */
+#define KEY_FLAG_INVALIDATED   5   /* set if key has been invalidated */
+#define KEY_FLAG_BUILTIN   6   /* set if key is built in to the kernel 
*/
+#define KEY_FLAG_ROOT_CAN_INVAL7   /* set if key can be 
invalidated by root without permission */
+#define KEY_FLAG_KEEP  8   /* set if key should not be removed */
+#define KEY_FLAG_UID_KEYRING   9   /* set if key is a user or user session 
keyring */
 
/* the key type and key description string
 * - the desc is used to match a key against search criteria
@@ -213,7 +217,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
@@ -362,8 +365,12 @@ extern void key_set_timeout(struct key *, unsigned);
  */
 static inline bool key_is_instantiated(const struct key *key)
 {
-   return test_bit(KEY_FLAG_INSTANTIATED, >flags) &&
-   !test_bit(KEY_FLAG_NEGATIVE, >flags);
+   return key->state == KEY_IS_INSTANTIATED;
+}
+
+static inline bool key_is_negative(const 

[PATCH v3 2/7] KEYS: fix race between updating and finding negative key

2017-09-27 Thread Eric Biggers
From: Eric Biggers 

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 
#include 
#include 

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 fd39a6b0
IP: __key_link_begin+0x0/0x100
PGD 7a0a067 P4D 7a0a067 PUD 7a0c067 PMD 0
Oops:  [#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: 9791fd809140 task.stack: acba402bc000
RIP: 0010:__key_link_begin+0x0/0x100
RSP: 0018:acba402bfdc8 EFLAGS: 00010282
RAX: 9791fd809140 RBX: fd39a620 RCX: 0008
RDX: acba402bfdd0 RSI: fd39a6a0 RDI: 9791fd810600
RBP: acba402bfdf8 R08: 0063 R09: 94845620
R10: 8080808080808080 R11: 0004 R12: 9791fd810600
R13: 9791fd39a940 R14: fd39a6a0 R15: 
FS:  7fbf14a90740() GS:9791ffd0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: fd39a6b0 CR3: 3b910003 CR4: 003606e0
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:7ffd8e4fe6c8 EFLAGS: 0246 ORIG_RAX: 00f9
RAX: ffda RBX: 36cc28fb RCX: 7fbf14190bb9
RDX: 55748b56ca4a RSI: 55748b56ca46 RDI: 55748b56ca4b
RBP: 55748b56ca4a R08: 0001 R09: 0001
R10: 36cc28fb R11: 0246 R12: 55748b56c8b0
R13: 7ffd8e4fe7d0 R14:  R15: 
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: acba402bfdc8
CR2: fd39a6b0

Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
Cc: [v4.4+]
Signed-off-by: Eric Biggers 
---
 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
+  

[PATCH v3 2/7] KEYS: fix race between updating and finding negative key

2017-09-27 Thread Eric Biggers
From: Eric Biggers 

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 
#include 
#include 

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 fd39a6b0
IP: __key_link_begin+0x0/0x100
PGD 7a0a067 P4D 7a0a067 PUD 7a0c067 PMD 0
Oops:  [#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: 9791fd809140 task.stack: acba402bc000
RIP: 0010:__key_link_begin+0x0/0x100
RSP: 0018:acba402bfdc8 EFLAGS: 00010282
RAX: 9791fd809140 RBX: fd39a620 RCX: 0008
RDX: acba402bfdd0 RSI: fd39a6a0 RDI: 9791fd810600
RBP: acba402bfdf8 R08: 0063 R09: 94845620
R10: 8080808080808080 R11: 0004 R12: 9791fd810600
R13: 9791fd39a940 R14: fd39a6a0 R15: 
FS:  7fbf14a90740() GS:9791ffd0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: fd39a6b0 CR3: 3b910003 CR4: 003606e0
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:7ffd8e4fe6c8 EFLAGS: 0246 ORIG_RAX: 00f9
RAX: ffda RBX: 36cc28fb RCX: 7fbf14190bb9
RDX: 55748b56ca4a RSI: 55748b56ca46 RDI: 55748b56ca4b
RBP: 55748b56ca4a R08: 0001 R09: 0001
R10: 36cc28fb R11: 0246 R12: 55748b56c8b0
R13: 7ffd8e4fe7d0 R14:  R15: 
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: acba402bfdc8
CR2: fd39a6b0

Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
Cc: [v4.4+]
Signed-off-by: Eric Biggers 
---
 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