| Message ID | 155856412507.10428.15987388402707639951.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | keys: Miscellany | expand |
On Wed, 22 May 2019, David Howells wrote: > + > + if (flags & ~KEYCTL_MOVE_EXCL) > + return -EINVAL; > + > + key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK); > + if (IS_ERR(key_ref)) { > + ret = PTR_ERR(key_ref); > + goto error; > + } This could probably be a simple return, as there is no cleanup. > + > + from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE); > + if (IS_ERR(from_ref)) { > + ret = PTR_ERR(from_ref); > + goto error2; > + } > + > + to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE); > + if (IS_ERR(to_ref)) { > + ret = PTR_ERR(to_ref); > + goto error3; > + } > + > + ret = key_move(key_ref_to_ptr(key_ref), key_ref_to_ptr(from_ref), > + key_ref_to_ptr(to_ref), flags); > + > + key_ref_put(to_ref); > +error3: > + key_ref_put(from_ref); > +error2: > + key_ref_put(key_ref); > +error: > + return ret; > +} > +
James Morris <jmorris@namei.org> wrote: > > + > > + if (flags & ~KEYCTL_MOVE_EXCL) > > + return -EINVAL; > > + > > + key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK); > > + if (IS_ERR(key_ref)) { > > + ret = PTR_ERR(key_ref); > > + goto error; > > + } > > This could probably be a simple return, as there is no cleanup. Changed. David
On Wed, May 22, 2019 at 11:28:45PM +0100, David Howells wrote: > Add a keyctl to atomically move a link to a key from one keyring to > another. The key must exist in "from" keyring and a flag can be given to > cause the operation to fail if there's a matching key already in the "to" > keyring. > > This can be done with: > > keyctl(KEYCTL_MOVE, > key_serial_t key, > key_serial_t from_keyring, > key_serial_t to_keyring, > unsigned int flags); > > The key being moved must grant Link permission and both keyrings must grant > Write permission. > > flags should be 0 or KEYCTL_MOVE_EXCL, with the latter preventing > displacement of a matching key from the "to" keyring. > > Signed-off-by: David Howells <dhowells@redhat.com> This shows up after a few seconds of syzkaller fuzzing with a description of KEYCTL_MOVE added: WARNING: possible circular locking dependency detected 5.2.0-rc1 #5 Not tainted ------------------------------------------------------ syz-executor.28/27700 is trying to acquire lock: 00000000049888d8 (keyring_serialise_link_sem){+.+.}, at: __key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231 but task is already holding lock: 00000000b171310c (&type->lock_class/1){+.+.}, at: __key_link_begin+0xa4/0x2d0 security/keys/keyring.c:1222 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&type->lock_class/1){+.+.}: lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302 down_write_nested+0x3c/0xa0 kernel/locking/rwsem.c:177 __key_unlink_begin+0x6c/0x110 security/keys/keyring.c:1398 key_move+0x3ad/0x470 security/keys/keyring.c:1538 keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610 __do_sys_keyctl security/keys/keyctl.c:1823 [inline] __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685 __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685 do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301 entry_SYSCALL_64_after_hwframe+0x49/0xbe -> #0 (keyring_serialise_link_sem){+.+.}: check_prevs_add kernel/locking/lockdep.c:2417 [inline] validate_chain kernel/locking/lockdep.c:2799 [inline] __lock_acquire+0x38a4/0x3c30 kernel/locking/lockdep.c:3792 lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302 down_write+0x38/0xa0 kernel/locking/rwsem.c:66 __key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231 key_move+0xf0/0x470 security/keys/keyring.c:1529 keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610 __do_sys_keyctl security/keys/keyctl.c:1823 [inline] __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685 __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685 do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301 entry_SYSCALL_64_after_hwframe+0x49/0xbe other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&type->lock_class/1); lock(keyring_serialise_link_sem); lock(&type->lock_class/1); lock(keyring_serialise_link_sem); *** DEADLOCK *** 2 locks held by syz-executor.28/27700: #0: 000000002a03f208 (&type->lock_class){++++}, at: __key_unlink_begin+0x6c/0x110 security/keys/keyring.c:1398 #1: 00000000b171310c (&type->lock_class/1){+.+.}, at: __key_link_begin+0xa4/0x2d0 security/keys/keyring.c:1222 stack backtrace: CPU: 8 PID: 27700 Comm: syz-executor.28 Not tainted 5.2.0-rc1 #5 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0xb1/0x118 lib/dump_stack.c:113 print_circular_bug+0x4a4/0x4b5 kernel/locking/lockdep.c:1564 check_prev_add+0xd1f/0x1af7 kernel/locking/lockdep.c:2309 check_prevs_add kernel/locking/lockdep.c:2417 [inline] validate_chain kernel/locking/lockdep.c:2799 [inline] __lock_acquire+0x38a4/0x3c30 kernel/locking/lockdep.c:3792 lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302 down_write+0x38/0xa0 kernel/locking/rwsem.c:66 __key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231 key_move+0xf0/0x470 security/keys/keyring.c:1529 keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610 __do_sys_keyctl security/keys/keyctl.c:1823 [inline] __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685 __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685 do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x458a09 Code: dd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 ab b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007f9d53755c88 EFLAGS: 00000246 ORIG_RAX: 00000000000000fa RAX: ffffffffffffffda RBX: 000000000071bf00 RCX: 0000000000458a09 RDX: 000000001f62d16e RSI: 000000002490e642 RDI: 000000000000001e RBP: 00007f9d53755ca0 R08: 0000000000000000 R09: 0000000000000000 R10: 000000001afbc80a R11: 0000000000000246 R12: 00007f9d537566d4 R13: 00000000004ac12c R14: 00000000006ebd68 R15: 0000000000000003 FAULT_INJECTION: forcing a failure. name failslab, interval 1, probability 0, space 0, times 0 CPU: 8 PID: 27700 Comm: syz-executor.28 Not tainted 5.2.0-rc1 #5 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0xb1/0x118 lib/dump_stack.c:113 fail_dump lib/fault-inject.c:51 [inline] should_fail+0x61e/0x720 lib/fault-inject.c:143 __should_failslab+0xec/0x120 mm/failslab.c:32 should_failslab+0x9/0x14 mm/slab_common.c:1610 slab_pre_alloc_hook mm/slab.h:420 [inline] slab_alloc mm/slab.c:3312 [inline] kmem_cache_alloc_trace+0x146/0x2a0 mm/slab.c:3553 kmalloc include/linux/slab.h:547 [inline] kzalloc include/linux/slab.h:742 [inline] assoc_array_insert+0xcc/0x440 lib/assoc_array.c:985 __key_link_begin+0x120/0x2d0 security/keys/keyring.c:1236 key_move+0xf0/0x470 security/keys/keyring.c:1529 keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610 __do_sys_keyctl security/keys/keyctl.c:1823 [inline] __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685 __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685 do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x458a09 Code: dd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 ab b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007f9d53755c88 EFLAGS: 00000246 ORIG_RAX: 00000000000000fa RAX: ffffffffffffffda RBX: 000000000071bf00 RCX: 0000000000458a09 RDX: 000000001f62d16e RSI: 000000002490e642 RDI: 000000000000001e RBP: 00007f9d53755ca0 R08: 0000000000000000 R09: 0000000000000000 R10: 000000001afbc80a R11: 0000000000000246 R12: 00007f9d537566d4 R13: 00000000004ac12c R14: 00000000006ebd68 R15: 0000000000000003
Eric Biggers <ebiggers@kernel.org> wrote: > This shows up after a few seconds of syzkaller fuzzing with a description of > KEYCTL_MOVE added: Yeah... I'm fixing that now. I've also created a bunch of tests, manpages, etc. for keyutils which I'll push when I've fixed my patches. David
diff --git a/include/linux/key.h b/include/linux/key.h index 1f09aad1c98c..612e1cf84049 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -310,6 +310,11 @@ extern int key_update(key_ref_t key, extern int key_link(struct key *keyring, struct key *key); +extern int key_move(struct key *key, + struct key *from_keyring, + struct key *to_keyring, + unsigned int flags); + extern int key_unlink(struct key *keyring, struct key *key); diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h index f45ee0f69c0c..fd9fb11b312b 100644 --- a/include/uapi/linux/keyctl.h +++ b/include/uapi/linux/keyctl.h @@ -67,6 +67,7 @@ #define KEYCTL_PKEY_SIGN 27 /* Create a public key signature */ #define KEYCTL_PKEY_VERIFY 28 /* Verify a public key signature */ #define KEYCTL_RESTRICT_KEYRING 29 /* Restrict keys allowed to link to a keyring */ +#define KEYCTL_MOVE 30 /* Move keys between keyrings */ /* keyctl structures */ struct keyctl_dh_params { @@ -112,4 +113,6 @@ struct keyctl_pkey_params { __u32 __spare[7]; }; +#define KEYCTL_MOVE_EXCL 0x00000001 /* Do not displace from the to-keyring */ + #endif /* _LINUX_KEYCTL_H */ diff --git a/security/keys/compat.c b/security/keys/compat.c index 9482df601dc3..b326bc4f84d7 100644 --- a/security/keys/compat.c +++ b/security/keys/compat.c @@ -159,6 +159,9 @@ COMPAT_SYSCALL_DEFINE5(keyctl, u32, option, return keyctl_pkey_verify(compat_ptr(arg2), compat_ptr(arg3), compat_ptr(arg4), compat_ptr(arg5)); + case KEYCTL_MOVE: + return keyctl_keyring_move(arg2, arg3, arg4, arg5); + default: return -EOPNOTSUPP; } diff --git a/security/keys/internal.h b/security/keys/internal.h index 93513667ff9a..821819b4ee13 100644 --- a/security/keys/internal.h +++ b/security/keys/internal.h @@ -215,6 +215,7 @@ extern long keyctl_update_key(key_serial_t, const void __user *, size_t); extern long keyctl_revoke_key(key_serial_t); extern long keyctl_keyring_clear(key_serial_t); extern long keyctl_keyring_link(key_serial_t, key_serial_t); +extern long keyctl_keyring_move(key_serial_t, key_serial_t, key_serial_t, unsigned int); extern long keyctl_keyring_unlink(key_serial_t, key_serial_t); extern long keyctl_describe_key(key_serial_t, char __user *, size_t); extern long keyctl_keyring_search(key_serial_t, const char __user *, diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index 0f947bcbad46..46188cda177e 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -572,6 +572,55 @@ long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid) return ret; } +/* + * Move a link to a key from one keyring to another, displacing any matching + * key from the destination keyring. + * + * The key must grant the caller Link permission and both keyrings must grant + * the caller Write permission. There must also be a link in the from keyring + * to the key. If both keyrings are the same, nothing is done. + * + * If successful, 0 will be returned. + */ +long keyctl_keyring_move(key_serial_t id, key_serial_t from_ringid, + key_serial_t to_ringid, unsigned int flags) +{ + key_ref_t key_ref, from_ref, to_ref; + long ret; + + if (flags & ~KEYCTL_MOVE_EXCL) + return -EINVAL; + + key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK); + if (IS_ERR(key_ref)) { + ret = PTR_ERR(key_ref); + goto error; + } + + from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE); + if (IS_ERR(from_ref)) { + ret = PTR_ERR(from_ref); + goto error2; + } + + to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE); + if (IS_ERR(to_ref)) { + ret = PTR_ERR(to_ref); + goto error3; + } + + ret = key_move(key_ref_to_ptr(key_ref), key_ref_to_ptr(from_ref), + key_ref_to_ptr(to_ref), flags); + + key_ref_put(to_ref); +error3: + key_ref_put(from_ref); +error2: + key_ref_put(key_ref); +error: + return ret; +} + /* * Return a description of a key to userspace. * @@ -1772,6 +1821,12 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3, (const void __user *)arg4, (const void __user *)arg5); + case KEYCTL_MOVE: + return keyctl_keyring_move((key_serial_t)arg2, + (key_serial_t)arg3, + (key_serial_t)arg4, + (unsigned int)arg5); + default: return -EOPNOTSUPP; } diff --git a/security/keys/keyring.c b/security/keys/keyring.c index cd669f758632..df3144f9c1aa 100644 --- a/security/keys/keyring.c +++ b/security/keys/keyring.c @@ -1476,6 +1476,94 @@ int key_unlink(struct key *keyring, struct key *key) } EXPORT_SYMBOL(key_unlink); +/** + * key_move - Move a key from one keyring to another + * @key: The key to move + * @from_keyring: The keyring to remove the link from. + * @to_keyring: The keyring to make the link in. + * @flags: Qualifying flags, such as KEYCTL_MOVE_EXCL. + * + * Make a link in @to_keyring to a key, such that the keyring holds a reference + * on that key and the key can potentially be found by searching that keyring + * whilst simultaneously removing a link to the key from @from_keyring. + * + * This function will write-lock both keyring's semaphores and will consume + * some of the user's key data quota to hold the link on @to_keyring. + * + * Returns 0 if successful, -ENOTDIR if either keyring isn't a keyring, + * -EKEYREVOKED if either keyring has been revoked, -ENFILE if the second + * keyring is full, -EDQUOT if there is insufficient key data quota remaining + * to add another link or -ENOMEM if there's insufficient memory. If + * KEYCTL_MOVE_EXCL is set, then -EEXIST will be returned if there's already a + * matching key in @to_keyring. + * + * It is assumed that the caller has checked that it is permitted for a link to + * be made (the keyring should have Write permission and the key Link + * permission). + */ +int key_move(struct key *key, + struct key *from_keyring, + struct key *to_keyring, + unsigned int flags) +{ + struct assoc_array_edit *from_edit, *to_edit; + int ret; + + kenter("%d,%d,%d", key->serial, from_keyring->serial, to_keyring->serial); + + if (from_keyring == to_keyring) + return 0; + + key_check(key); + key_check(from_keyring); + key_check(to_keyring); + + /* We have to be very careful here to take the keyring locks in the + * right order, lest we open ourselves to deadlocking against another + * move operation. + */ + if (from_keyring < to_keyring) { + ret = __key_unlink_begin(from_keyring, 0, key, &from_edit); + if (ret < 0) + goto out; + ret = __key_link_begin(to_keyring, 1, &key->index_key, &to_edit); + if (ret < 0) { + assoc_array_cancel_edit(from_edit); + goto out; + } + } else { + ret = __key_link_begin(to_keyring, 0, &key->index_key, &to_edit); + if (ret < 0) + goto out; + ret = __key_unlink_begin(from_keyring, 1, key, &from_edit); + if (ret < 0) { + __key_link_end(to_keyring, &key->index_key, to_edit); + goto out; + } + } + + ret = -EEXIST; + if (to_edit->dead_leaf && (flags & KEYCTL_MOVE_EXCL)) + goto error; + + ret = __key_link_check_restriction(to_keyring, key); + if (ret < 0) + goto error; + ret = __key_link_check_live_key(to_keyring, key); + if (ret < 0) + goto error; + + __key_unlink(from_keyring, key, &from_edit); + __key_link(key, &to_edit); +error: + __key_unlink_end(from_keyring, key, from_edit); + __key_link_end(to_keyring, &key->index_key, to_edit); +out: + kleave(" = %d", ret); + return ret; +} +EXPORT_SYMBOL(key_move); + /** * keyring_clear - Clear a keyring * @keyring: The keyring to clear.
Add a keyctl to atomically move a link to a key from one keyring to another. The key must exist in "from" keyring and a flag can be given to cause the operation to fail if there's a matching key already in the "to" keyring. This can be done with: keyctl(KEYCTL_MOVE, key_serial_t key, key_serial_t from_keyring, key_serial_t to_keyring, unsigned int flags); The key being moved must grant Link permission and both keyrings must grant Write permission. flags should be 0 or KEYCTL_MOVE_EXCL, with the latter preventing displacement of a matching key from the "to" keyring. Signed-off-by: David Howells <dhowells@redhat.com> --- include/linux/key.h | 5 ++ include/uapi/linux/keyctl.h | 3 + security/keys/compat.c | 3 + security/keys/internal.h | 1 security/keys/keyctl.c | 55 +++++++++++++++++++++++++++ security/keys/keyring.c | 88 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 155 insertions(+)