stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	stable@vger.kernel.org, Eric Biggers <ebiggers@google.com>,
	David Howells <dhowells@redhat.com>
Subject: [PATCH 4.9 30/48] KEYS: dont let add_key() update an uninstantiated key
Date: Tue, 24 Oct 2017 15:03:43 +0200	[thread overview]
Message-ID: <20171024125729.095749546@linuxfoundation.org> (raw)
In-Reply-To: <20171024125727.668462013@linuxfoundation.org>

4.9-stable review patch.  If anyone has any objections, please let me know.

------------------

From: David Howells <dhowells@redhat.com>

commit 60ff5b2f547af3828aebafd54daded44cfb0807a upstream.

Currently, when passed a key that already exists, add_key() will call the
key's ->update() method if such exists.  But this is heavily broken in the
case where the key is uninstantiated because it doesn't call
__key_instantiate_and_link().  Consequently, it doesn't do most of the
things that are supposed to happen when the key is instantiated, such as
setting the instantiation state, clearing KEY_FLAG_USER_CONSTRUCT and
awakening tasks waiting on it, and incrementing key->user->nikeys.

It also never takes key_construction_mutex, which means that
->instantiate() can run concurrently with ->update() on the same key.  In
the case of the "user" and "logon" key types this causes a memory leak, at
best.  Maybe even worse, the ->update() methods of the "encrypted" and
"trusted" key types actually just dereference a NULL pointer when passed an
uninstantiated key.

Change key_create_or_update() to wait interruptibly for the key to finish
construction before continuing.

This patch only affects *uninstantiated* keys.  For now we still allow a
negatively instantiated key to be updated (thereby positively
instantiating it), although that's broken too (the next patch fixes it)
and I'm not sure that anyone actually uses that functionality either.

Here is a simple reproducer for the bug using the "encrypted" key type
(requires CONFIG_ENCRYPTED_KEYS=y), though as noted above the bug
pertained to more than just the "encrypted" key type:

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

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

        if (fork()) {
            for (;;) {
                const char payload[] = "update user:foo 32";

                usleep(rand() % 10000);
                add_key("encrypted", "desc", payload, sizeof(payload), ringid);
                keyctl_clear(ringid);
            }
        } else {
            for (;;)
                request_key("encrypted", "desc", "callout_info", ringid);
        }
    }

It causes:

    BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
    IP: encrypted_update+0xb0/0x170
    PGD 7a178067 P4D 7a178067 PUD 77269067 PMD 0
    PREEMPT SMP
    CPU: 0 PID: 340 Comm: reproduce Tainted: G      D         4.14.0-rc1-00025-g428490e38b2e #796
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
    task: ffff8a467a39a340 task.stack: ffffb15c40770000
    RIP: 0010:encrypted_update+0xb0/0x170
    RSP: 0018:ffffb15c40773de8 EFLAGS: 00010246
    RAX: 0000000000000000 RBX: ffff8a467a275b00 RCX: 0000000000000000
    RDX: 0000000000000005 RSI: ffff8a467a275b14 RDI: ffffffffb742f303
    RBP: ffffb15c40773e20 R08: 0000000000000000 R09: ffff8a467a275b17
    R10: 0000000000000020 R11: 0000000000000000 R12: 0000000000000000
    R13: 0000000000000000 R14: ffff8a4677057180 R15: ffff8a467a275b0f
    FS:  00007f5d7fb08700(0000) GS:ffff8a467f200000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: 0000000000000018 CR3: 0000000077262005 CR4: 00000000001606f0
    Call Trace:
     key_create_or_update+0x2bc/0x460
     SyS_add_key+0x10c/0x1d0
     entry_SYSCALL_64_fastpath+0x1f/0xbe
    RIP: 0033:0x7f5d7f211259
    RSP: 002b:00007ffed03904c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000f8
    RAX: ffffffffffffffda RBX: 000000003b2a7955 RCX: 00007f5d7f211259
    RDX: 00000000004009e4 RSI: 00000000004009ff RDI: 0000000000400a04
    RBP: 0000000068db8bad R08: 000000003b2a7955 R09: 0000000000000004
    R10: 000000000000001a R11: 0000000000000246 R12: 0000000000400868
    R13: 00007ffed03905d0 R14: 0000000000000000 R15: 0000000000000000
    Code: 77 28 e8 64 34 1f 00 45 31 c0 31 c9 48 8d 55 c8 48 89 df 48 8d 75 d0 e8 ff f9 ff ff 85 c0 41 89 c4 0f 88 84 00 00 00 4c 8b 7d c8 <49> 8b 75 18 4c 89 ff e8 24 f8 ff ff 85 c0 41 89 c4 78 6d 49 8b
    RIP: encrypted_update+0xb0/0x170 RSP: ffffb15c40773de8
    CR2: 0000000000000018

Reported-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Eric Biggers <ebiggers@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 security/keys/key.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -935,6 +935,16 @@ error:
 	 */
 	__key_link_end(keyring, &index_key, edit);
 
+	key = key_ref_to_ptr(key_ref);
+	if (test_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags)) {
+		ret = wait_for_key_construction(key, true);
+		if (ret < 0) {
+			key_ref_put(key_ref);
+			key_ref = ERR_PTR(ret);
+			goto error_free_prep;
+		}
+	}
+
 	key_ref = __key_update(key_ref, &prep);
 	goto error_free_prep;
 }

  parent reply	other threads:[~2017-10-24 13:06 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-24 13:03 [PATCH 4.9 00/48] 4.9.59-stable review Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 01/48] USB: devio: Revert "USB: devio: Dont corrupt user memory" Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 02/48] USB: core: fix out-of-bounds access bug in usb_get_bos_descriptor() Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 03/48] USB: serial: metro-usb: add MS7820 device id Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 04/48] usb: cdc_acm: Add quirk for Elatec TWN3 Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 06/48] usb: hub: Allow reset retry for USB2 devices on connect bounce Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 07/48] ALSA: usb-audio: Add native DSD support for Pro-Ject Pre Box S2 Digital Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 08/48] can: gs_usb: fix busy loop if no more TX context is available Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 09/48] parisc: Fix double-word compare and exchange in LWS code on 32-bit kernels Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 10/48] iio: dummy: events: Add missing break Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 11/48] usb: musb: sunxi: Explicitly release USB PHY on exit Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 12/48] usb: musb: Check for host-mode using is_host_active() on reset interrupt Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 13/48] xhci: Identify USB 3.1 capable hosts by their port protocol capability Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 15/48] drm/nouveau/bsp/g92: disable by default Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 16/48] drm/nouveau/mmu: flush tlbs before deleting page tables Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 17/48] ALSA: seq: Enable use locking in all configurations Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 18/48] ALSA: hda: Remove superfluous - added by printk conversion Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 19/48] ALSA: hda: Abort capability probe at invalid register read Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 20/48] i2c: ismt: Separate I2C block read from SMBus block read Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 22/48] brcmfmac: Add check for short event packets Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 23/48] brcmsmac: make some local variables static const to reduce stack size Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 24/48] bus: mbus: fix window size calculation for 4GB windows Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 25/48] clockevents/drivers/cs5535: Improve resilience to spurious interrupts Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 26/48] rtlwifi: rtl8821ae: Fix connection lost problem Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 27/48] x86/microcode/intel: Disable late loading on model 79 Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 28/48] KEYS: encrypted: fix dereference of NULL user_key_payload Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 29/48] lib/digsig: " Greg Kroah-Hartman
2017-10-24 13:03 ` Greg Kroah-Hartman [this message]
2017-10-24 13:03 ` [PATCH 4.9 31/48] pkcs7: Prevent NULL pointer dereference, since sinfo is not always set Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 32/48] vmbus: fix missing signaling in hv_signal_on_read() Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 33/48] xfs: dont unconditionally clear the reflink flag on zero-block files Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 34/48] xfs: evict CoW fork extents when performing finsert/fcollapse Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 35/48] fs/xfs: Use %pS printk format for direct addresses Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 36/48] xfs: report zeroed or not correctly in xfs_zero_range() Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 37/48] xfs: update i_size after unwritten conversion in dio completion Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 38/48] xfs: perag initialization should only touch m_ag_max_usable for AG 0 Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 39/48] xfs: Capture state of the right inode in xfs_iflush_done Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 40/48] xfs: always swap the cow forks when swapping extents Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 41/48] xfs: handle racy AIO in xfs_reflink_end_cow Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 42/48] xfs: Dont log uninitialised fields in inode structures Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 43/48] xfs: move more RT specific code under CONFIG_XFS_RT Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 44/48] xfs: dont change inode mode if ACL update fails Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 45/48] xfs: reinit btree pointer on attr tree inactivation walk Greg Kroah-Hartman
2017-10-24 13:03 ` [PATCH 4.9 46/48] xfs: handle error if xfs_btree_get_bufs fails Greg Kroah-Hartman
2017-10-24 13:04 ` [PATCH 4.9 47/48] xfs: cancel dirty pages on invalidation Greg Kroah-Hartman
2017-10-24 13:04 ` [PATCH 4.9 48/48] xfs: trim writepage mapping to within eof Greg Kroah-Hartman
2017-10-24 21:28 ` [PATCH 4.9 00/48] 4.9.59-stable review Guenter Roeck
2017-10-24 22:26 ` Tom Gall
2017-10-25  7:02   ` Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171024125729.095749546@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=dhowells@redhat.com \
    --cc=ebiggers@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).