From 413cd3d9abeaef590e5ce00564f7a443165db238 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Fri, 11 May 2012 10:59:08 +1000 Subject: [PATCH] keys: change keyctl_session_to_parent() to use task_work_add() Change keyctl_session_to_parent() to use task_work_add() and move key_replace_session_keyring() logic into task_work->func(). Note that we do task_work_cancel() before task_work_add() to ensure that only one work can be pending at any time. This is important, we must not allow user-space to abuse the parent's ->task_works list. The callback, replace_session_keyring(), checks PF_EXITING. I guess this is not really needed but looks better. As a side effect, this fixes the (unlikely) race. The callers of key_replace_session_keyring() and keyctl_session_to_parent() lack the necessary barriers, the parent can miss the request. Now we can remove task_struct->replacement_session_keyring and related code. Signed-off-by: Oleg Nesterov Acked-by: David Howells Cc: Thomas Gleixner Cc: Richard Kuo Cc: Linus Torvalds Cc: Alexander Gordeev Cc: Chris Zankel Cc: David Smith Cc: "Frank Ch. Eigler" Cc: Geert Uytterhoeven Cc: Larry Woodman Cc: Peter Zijlstra Cc: Tejun Heo Cc: Ingo Molnar Signed-off-by: Andrew Morton Signed-off-by: Al Viro --- include/linux/key.h | 6 ++-- security/keys/internal.h | 2 ++ security/keys/keyctl.c | 63 ++++++++++++++++++++---------------- security/keys/process_keys.c | 20 ++++-------- 4 files changed, 46 insertions(+), 45 deletions(-) diff --git a/include/linux/key.h b/include/linux/key.h index 5231800770e1..2a0ee11584e9 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -33,6 +33,8 @@ typedef uint32_t key_perm_t; struct key; +#define key_replace_session_keyring() do { } while (0) + #ifdef CONFIG_KEYS #undef KEY_DEBUGGING @@ -308,9 +310,6 @@ static inline bool key_is_instantiated(const struct key *key) #ifdef CONFIG_SYSCTL extern ctl_table key_sysctls[]; #endif - -extern void key_replace_session_keyring(void); - /* * the userspace interface */ @@ -334,7 +333,6 @@ extern void key_init(void); #define key_fsuid_changed(t) do { } while(0) #define key_fsgid_changed(t) do { } while(0) #define key_init() do { } while(0) -#define key_replace_session_keyring() do { } while(0) #endif /* CONFIG_KEYS */ #endif /* __KERNEL__ */ diff --git a/security/keys/internal.h b/security/keys/internal.h index f711b094ed41..3dcbf86b0d31 100644 --- a/security/keys/internal.h +++ b/security/keys/internal.h @@ -14,6 +14,7 @@ #include #include +#include #ifdef __KDEBUG #define kenter(FMT, ...) \ @@ -148,6 +149,7 @@ extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags, #define KEY_LOOKUP_FOR_UNLINK 0x04 extern long join_session_keyring(const char *name); +extern void key_change_session_keyring(struct task_work *twork); extern struct work_struct key_gc_work; extern unsigned key_gc_delay; diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index 534a634283a4..2f28126215a2 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -1456,47 +1456,55 @@ long keyctl_session_to_parent(void) { struct task_struct *me, *parent; const struct cred *mycred, *pcred; - struct cred *cred, *oldcred; + struct task_work *newwork, *oldwork; key_ref_t keyring_r; + struct cred *cred; int ret; keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_LINK); if (IS_ERR(keyring_r)) return PTR_ERR(keyring_r); + ret = -ENOMEM; + newwork = kmalloc(sizeof(struct task_work), GFP_KERNEL); + if (!newwork) + goto error_keyring; + /* our parent is going to need a new cred struct, a new tgcred struct * and new security data, so we allocate them here to prevent ENOMEM in * our parent */ - ret = -ENOMEM; cred = cred_alloc_blank(); if (!cred) - goto error_keyring; + goto error_newwork; cred->tgcred->session_keyring = key_ref_to_ptr(keyring_r); - keyring_r = NULL; + init_task_work(newwork, key_change_session_keyring, cred); me = current; rcu_read_lock(); write_lock_irq(&tasklist_lock); - parent = me->real_parent; ret = -EPERM; + oldwork = NULL; + parent = me->real_parent; /* the parent mustn't be init and mustn't be a kernel thread */ if (parent->pid <= 1 || !parent->mm) - goto not_permitted; + goto unlock; /* the parent must be single threaded */ if (!thread_group_empty(parent)) - goto not_permitted; + goto unlock; /* the parent and the child must have different session keyrings or * there's no point */ mycred = current_cred(); pcred = __task_cred(parent); if (mycred == pcred || - mycred->tgcred->session_keyring == pcred->tgcred->session_keyring) - goto already_same; + mycred->tgcred->session_keyring == pcred->tgcred->session_keyring) { + ret = 0; + goto unlock; + } /* the parent must have the same effective ownership and mustn't be * SUID/SGID */ @@ -1506,38 +1514,37 @@ long keyctl_session_to_parent(void) pcred->gid != mycred->egid || pcred->egid != mycred->egid || pcred->sgid != mycred->egid) - goto not_permitted; + goto unlock; /* the keyrings must have the same UID */ if ((pcred->tgcred->session_keyring && pcred->tgcred->session_keyring->uid != mycred->euid) || mycred->tgcred->session_keyring->uid != mycred->euid) - goto not_permitted; + goto unlock; - /* if there's an already pending keyring replacement, then we replace - * that */ - oldcred = parent->replacement_session_keyring; + /* cancel an already pending keyring replacement */ + oldwork = task_work_cancel(parent, key_change_session_keyring); /* the replacement session keyring is applied just prior to userspace * restarting */ - parent->replacement_session_keyring = cred; - cred = NULL; - set_ti_thread_flag(task_thread_info(parent), TIF_NOTIFY_RESUME); - + ret = task_work_add(parent, newwork, true); + if (!ret) + newwork = NULL; +unlock: write_unlock_irq(&tasklist_lock); rcu_read_unlock(); - if (oldcred) - put_cred(oldcred); - return 0; - -already_same: - ret = 0; -not_permitted: - write_unlock_irq(&tasklist_lock); - rcu_read_unlock(); - put_cred(cred); + if (oldwork) { + put_cred(oldwork->data); + kfree(oldwork); + } + if (newwork) { + put_cred(newwork->data); + kfree(newwork); + } return ret; +error_newwork: + kfree(newwork); error_keyring: key_ref_put(keyring_r); return ret; diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c index d71056db7b67..4ad54eea1ea4 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -834,23 +834,17 @@ error: * Replace a process's session keyring on behalf of one of its children when * the target process is about to resume userspace execution. */ -void key_replace_session_keyring(void) +void key_change_session_keyring(struct task_work *twork) { - const struct cred *old; - struct cred *new; + const struct cred *old = current_cred(); + struct cred *new = twork->data; - if (!current->replacement_session_keyring) + kfree(twork); + if (unlikely(current->flags & PF_EXITING)) { + put_cred(new); return; + } - write_lock_irq(&tasklist_lock); - new = current->replacement_session_keyring; - current->replacement_session_keyring = NULL; - write_unlock_irq(&tasklist_lock); - - if (!new) - return; - - old = current_cred(); new-> uid = old-> uid; new-> euid = old-> euid; new-> suid = old-> suid;