From patchwork Tue Sep 26 20:11:00 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Biggers X-Patchwork-Id: 9972657 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D69D8602BD for ; Tue, 26 Sep 2017 20:14:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CA55728858 for ; Tue, 26 Sep 2017 20:14:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id BF26628BD9; Tue, 26 Sep 2017 20:14:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 046D528858 for ; Tue, 26 Sep 2017 20:14:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S970350AbdIZUOi (ORCPT ); Tue, 26 Sep 2017 16:14:38 -0400 Received: from mail-pf0-f172.google.com ([209.85.192.172]:56985 "EHLO mail-pf0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S967404AbdIZUO3 (ORCPT ); Tue, 26 Sep 2017 16:14:29 -0400 Received: by mail-pf0-f172.google.com with SMTP id g65so6066596pfe.13; Tue, 26 Sep 2017 13:14:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=VWUes66MtE8qeJOD/Ej3OVHei1LHGDwZ3FGLFfdNO7E=; b=nGVbQkLuD8MniBRrIsxgTkf4YU6hcaiLk7YSHRMhXFI04KU84gGdmLiaTkjAFVeq9b FNVyp2pcTCZzSlsSRFhlddUrfghdZ7IGiXOB0jnn7cmjg+NMAtAQHYuCPlVy0By8r6tY EUjtAqiXBVUuBKCg7h3cyqhgxv+LrFMblHHDOIkAKSWLXLbGyL7ySPlW1wY+DudT79fB sr4nIht0tUa/0YqQKuqyZmpOD9SwYsboQ+twSbHgDfSSdJQND7zUdzKJB9mVzLcESU1I tivLGejqkiS+OHeCHUmbop6Z48pfksv6MlNZSbGtwrB2ZYj722orLjqs/NB96E/cC5i5 fK2Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=VWUes66MtE8qeJOD/Ej3OVHei1LHGDwZ3FGLFfdNO7E=; b=Xlucc93FThjp5KjexA4LMOB2az2GI6bFiGgcX5YNWZ2NNgEHUnHJY2Ah7hehXr2oeP Wuv+JQhXx44qkJ5UpOm0qyOfN1LORzJRfePrs/GsSSRvfTm0ZM8s3Yh7o/LEGs2DMOUm NDsFLHE/uwwRIhQKGdC6OY0VCudNXKupDBNB5Fba6slMqJwdhyzJNEDwySTstFFhnAzb Y9wArYwwenokS2tHMJAdvGogrwjZeuah4vrC/MDYqNPLa3Ps2WtwRC8iwjA3VIdtGTgT fMaomUgw5ZnrLBQiNEnhem0Iois++ypUYGiySBtxAoH47UJNHFn1OMk4OzIBXgtp1Usc ZjSA== X-Gm-Message-State: AHPjjUjr0xL+zpChpt1VSIl597Fvi+jrG2JR0ZjFe/qxLal8bFXV0fdP OONhm0+bdULM6yEtTeJyF4B6oW4H X-Google-Smtp-Source: AOwi7QCtvK857/j3JCyI9BHa5slK7S1aFMpLf1ul2VDymP3nZZHmnw8LGSduFe/hGP9Byb6bZuyP3A== X-Received: by 10.84.191.129 with SMTP id a1mr11497073pld.272.1506456868254; Tue, 26 Sep 2017 13:14:28 -0700 (PDT) Received: from ebiggers-linuxstation.kir.corp.google.com ([100.66.174.81]) by smtp.gmail.com with ESMTPSA id g68sm16597640pfc.64.2017.09.26.13.14.27 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 26 Sep 2017 13:14:27 -0700 (PDT) From: Eric Biggers To: keyrings@vger.kernel.org Cc: David Howells , Michael Halcrow , linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Eric Biggers , stable@vger.kernel.org Subject: [PATCH v2 1/6] KEYS: fix race between updating and finding negative key Date: Tue, 26 Sep 2017 13:11:00 -0700 Message-Id: <20170926201105.126166-2-ebiggers3@gmail.com> X-Mailer: git-send-email 2.14.1.992.g2c7b836f3a-goog In-Reply-To: <20170926201105.126166-1-ebiggers3@gmail.com> References: <20170926201105.126166-1-ebiggers3@gmail.com> Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP 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 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: [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 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);