KEYS: fix race between updating and finding negative key

From: Eric Biggers <ebiggers@google.com>

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
retrieve the ->reject_error, which is in union with ->payload.

The problem is that no lock can be held during this, and it's impossible
to update KEY_FLAG_NEGATIVE atomically with respect to ->reject_error
and ->payload.  Updating ->reject_error or ->payload first as
key_update(), __key_update(), and key_reject_and_link() do is
insufficient because ->reject_error can be observed to have a bogus
value, having been overwritten with ->payload, before KEY_FLAG_NEGATIVE
is cleared.

Conversely, if we were to change KEY_FLAG_NEGATIVE first, then someone
could use ->reject_error or ->payload before it has been set.

Fix the bug by moving ->reject_error out of the union with ->payload,
then using nonzero ->reject_error to mean that the key is negative.

This eliminates the need for KEY_FLAG_NEGATIVE, which we remove as well
so that we don't have to handle memory ordering between
KEY_FLAG_NEGATIVE and ->reject_error.  We *do* still need to handle
memory ordering between KEY_FLAG_INSTANTIATED and ->reject_error, but
that was needed before (and for KEY_FLAG_NEGATIVE as well --- though it
wasn't done correctly, which was another bug).

The following program reproduces the primary bug, which dates back to
v4.4 when ->reject_error and ->payload were placed in union:

    #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);
        }
    }

It causes the following 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                      | 34 ++++++++++++++++++++++++++++----
 security/keys/encrypted-keys/encrypted.c |  2 +-
 security/keys/gc.c                       |  4 +---
 security/keys/key.c                      | 16 ++++++++++-----
 security/keys/keyctl.c                   |  2 +-
 security/keys/keyring.c                  |  5 ++---
 security/keys/proc.c                     |  2 +-
 security/keys/request_key.c              |  8 ++++----
 security/keys/trusted.c                  |  2 +-
 security/keys/user_defined.c             |  2 +-
 10 files changed, 53 insertions(+), 24 deletions(-)

KEYS: fix race between updating and finding negative key

Commit Message

Comments

Patch