kernel/linux.git/security/keys/key.c, branch v3.2.95

KEYS: don't let add_key() update an uninstantiated key

2017-11-11T13:34:38+00:00

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 #include #include 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 Signed-off-by: David Howells cc: Eric Biggers [bwh: Backported to 3.2: - Use the 'error' label to return, not 'error_free_prep' - Adjust context] Signed-off-by: Ben Hutchings

KEYS: potential uninitialized variable

2016-08-22T21:37:14+00:00

commit 38327424b40bcebe2de92d07312c89360ac9229a upstream. If __key_link_begin() failed then "edit" would be uninitialized. I've added a check to fix that. This allows a random user to crash the kernel, though it's quite difficult to achieve. There are three ways it can be done as the user would have to cause an error to occur in __key_link(): (1) Cause the kernel to run out of memory. In practice, this is difficult to achieve without ENOMEM cropping up elsewhere and aborting the attempt. (2) Revoke the destination keyring between the keyring ID being looked up and it being tested for revocation. In practice, this is difficult to time correctly because the KEYCTL_REJECT function can only be used from the request-key upcall process. Further, users can only make use of what's in /sbin/request-key.conf, though this does including a rejection debugging test - which means that the destination keyring has to be the caller's session keyring in practice. (3) Have just enough key quota available to create a key, a new session keyring for the upcall and a link in the session keyring, but not then sufficient quota to create a link in the nominated destination keyring so that it fails with EDQUOT. The bug can be triggered using option (3) above using something like the following: echo 80 >/proc/sys/kernel/keys/root_maxbytes keyctl request2 user debug:fred negate @t The above sets the quota to something much lower (80) to make the bug easier to trigger, but this is dependent on the system. Note also that the name of the keyring created contains a random number that may be between 1 and 10 characters in size, so may throw the test off by changing the amount of quota used. Assuming the failure occurs, something like the following will be seen: kfree_debugcheck: out of range ptr 6b6b6b6b6b6b6b68h ------------[ cut here ]------------ kernel BUG at ../mm/slab.c:2821! ... RIP: 0010:[] kfree_debugcheck+0x20/0x25 RSP: 0018:ffff8804014a7de8 EFLAGS: 00010092 RAX: 0000000000000034 RBX: 6b6b6b6b6b6b6b68 RCX: 0000000000000000 RDX: 0000000000040001 RSI: 00000000000000f6 RDI: 0000000000000300 RBP: ffff8804014a7df0 R08: 0000000000000001 R09: 0000000000000000 R10: ffff8804014a7e68 R11: 0000000000000054 R12: 0000000000000202 R13: ffffffff81318a66 R14: 0000000000000000 R15: 0000000000000001 ... Call Trace: kfree+0xde/0x1bc assoc_array_cancel_edit+0x1f/0x36 __key_link_end+0x55/0x63 key_reject_and_link+0x124/0x155 keyctl_reject_key+0xb6/0xe0 keyctl_negate_key+0x10/0x12 SyS_keyctl+0x9f/0xe7 do_syscall_64+0x63/0x13a entry_SYSCALL64_slow_path+0x25/0x25 Fixes: f70e2e06196a ('KEYS: Do preallocation for __key_link()') Signed-off-by: Dan Carpenter Signed-off-by: David Howells Signed-off-by: Linus Torvalds [bwh: Backported to 3.2: adjust context] Signed-off-by: Ben Hutchings

KEYS: Correctly destroy key payloads when their keytype is removed

2011-08-22T23:57:37+00:00

unregister_key_type() has code to mark a key as dead and make it unavailable in one loop and then destroy all those unavailable key payloads in the next loop. However, the loop to mark keys dead renders the key undetectable to the second loop by changing the key type pointer also. Fix this by the following means: (1) The key code has two garbage collectors: one deletes unreferenced keys and the other alters keyrings to delete links to old dead, revoked and expired keys. They can end up holding each other up as both want to scan the key serial tree under spinlock. Combine these into a single routine. (2) Move the dead key marking, dead link removal and dead key removal into the garbage collector as a three phase process running over the three cycles of the normal garbage collection procedure. This is tracked by the KEY_GC_REAPING_DEAD_1, _2 and _3 state flags. unregister_key_type() then just unlinks the key type from the list, wakes up the garbage collector and waits for the third phase to complete. (3) Downgrade the key types sem in unregister_key_type() once it has deleted the key type from the list so that it doesn't block the keyctl() syscall. (4) Dead keys that cannot be simply removed in the third phase have their payloads destroyed with the key's semaphore write-locked to prevent interference by the keyctl() syscall. There should be no in-kernel users of dead keys of that type by the point of unregistration, though keyctl() may be holding a reference. (5) Only perform timer recalculation in the GC if the timer actually expired. If it didn't, we'll get another cycle when it goes off - and if the key that actually triggered it has been removed, it's not a problem. (6) Only garbage collect link if the timer expired or if we're doing dead key clean up phase 2. (7) As only key_garbage_collector() is permitted to use rb_erase() on the key serial tree, it doesn't need to revalidate its cursor after dropping the spinlock as the node the cursor points to must still exist in the tree. (8) Drop the spinlock in the GC if there is contention on it or if we need to reschedule. After dealing with that, get the spinlock again and resume scanning. This has been tested in the following ways: (1) Run the keyutils testsuite against it. (2) Using the AF_RXRPC and RxKAD modules to test keytype removal: Load the rxrpc_s key type: # insmod /tmp/af-rxrpc.ko # insmod /tmp/rxkad.ko Create a key (http://people.redhat.com/~dhowells/rxrpc/listen.c): # /tmp/listen & [1] 8173 Find the key: # grep rxrpc_s /proc/keys 091086e1 I--Q-- 1 perm 39390000 0 0 rxrpc_s 52:2 Link it to a session keyring, preferably one with a higher serial number: # keyctl link 0x20e36251 @s Kill the process (the key should remain as it's linked to another place): # fg /tmp/listen ^C Remove the key type: rmmod rxkad rmmod af-rxrpc This can be made a more effective test by altering the following part of the patch: if (unlikely(gc_state & KEY_GC_REAPING_DEAD_2)) { /* Make sure everyone revalidates their keys if we marked a * bunch as being dead and make sure all keyring ex-payloads * are destroyed. */ kdebug("dead sync"); synchronize_rcu(); To call synchronize_rcu() in GC phase 1 instead. That causes that the keyring's old payload content to hang around longer until it's RCU destroyed - which usually happens after GC phase 3 is complete. This allows the destroy_dead_key branch to be tested. Reported-by: Benjamin Coddington Signed-off-by: David Howells Signed-off-by: James Morris

KEYS: Make the key reaper non-reentrant

2011-08-22T23:57:36+00:00

Make the key reaper non-reentrant by sticking it on the appropriate system work queue when we queue it. This will allow it to have global state and drop locks. It should probably be non-reentrant already as it may spend a long time holding the key serial spinlock, and so multiple entrants can spend long periods of time just sitting there spinning, waiting to get the lock. Signed-off-by: David Howells Signed-off-by: James Morris

KEYS: Move the unreferenced key reaper to the keys garbage collector file

2011-08-22T23:57:36+00:00

Move the unreferenced key reaper function to the keys garbage collector file as that's a more appropriate place with the dead key link reaper. Signed-off-by: David Howells Signed-off-by: James Morris

KEYS: Add a new keyctl op to reject a key with a specified error code

2011-03-08T00:17:18+00:00

Add a new keyctl op to reject a key with a specified error code. This works much the same as negating a key, and so keyctl_negate_key() is made a special case of keyctl_reject_key(). The difference is that keyctl_negate_key() selects ENOKEY as the error to be reported. Typically the key would be rejected with EKEYEXPIRED, EKEYREVOKED or EKEYREJECTED, but this is not mandatory. Signed-off-by: David Howells Signed-off-by: James Morris

KEYS: Add a key type op to permit the key description to be vetted

2011-03-08T00:17:15+00:00

Add a key type operation to permit the key type to vet the description of a new key that key_alloc() is about to allocate. The operation may reject the description if it wishes with an error of its choosing. If it does this, the key will not be allocated. Signed-off-by: David Howells Reviewed-by: Mimi Zohar Signed-off-by: James Morris

KEYS: Fix __key_link_end() quota fixup on error

2011-01-25T22:58:20+00:00

Fix __key_link_end()'s attempt to fix up the quota if an error occurs. There are two erroneous cases: Firstly, we always decrease the quota if the preallocated replacement keyring needs cleaning up, irrespective of whether or not we should (we may have replaced a pointer rather than adding another pointer). Secondly, we never clean up the quota if we added a pointer without the keyring storage being extended (we allocate multiple pointers at a time, even if we're not going to use them all immediately). We handle this by setting the bottom bit of the preallocation pointer in __key_link_begin() to indicate that the quota needs fixing up, which is then passed to __key_link() (which clears the whole thing) and __key_link_end(). Signed-off-by: David Howells Signed-off-by: Linus Torvalds

KEYS: Fix up comments in key management code

2011-01-21T22:59:30+00:00

Fix up comments in the key management code. No functional changes. Signed-off-by: David Howells Signed-off-by: Linus Torvalds

KEYS: Do some style cleanup in the key management code.

2011-01-21T22:59:29+00:00

Do a bit of a style clean up in the key management code. No functional changes. Done using: perl -p -i -e 's!^/[*]*/\n!!' security/keys/*.c perl -p -i -e 's!} /[*] end [a-z0-9_]*[(][)] [*]/\n!}\n!' security/keys/*.c sed -i -s -e ": next" -e N -e 's/^\n[}]$/}/' -e t -e P -e 's/^.*\n//' -e "b next" security/keys/*.c To remove /*****/ lines, remove comments on the closing brace of a function to name the function and remove blank lines before the closing brace of a function. Signed-off-by: David Howells Signed-off-by: Linus Torvalds