| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 17 Apr 2012 21:34:09 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: David Howells <dhowells@...hat.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>,
Alexander Gordeev <agordeev@...hat.com>,
Chris Zankel <chris@...kel.net>,
David Smith <dsmith@...hat.com>,
"Frank Ch. Eigler" <fche@...hat.com>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Larry Woodman <lwoodman@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Tejun Heo <tj@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use
task_work_add()
On 04/17, Oleg Nesterov wrote:
>
> On 04/17, David Howells wrote:
> >
> > > -void key_replace_session_keyring(void)
> > > +void key_change_session_keyring(struct task_work *twork)
> >
> > There's no particular need to change the name of the function.
>
> There is.
>
> Until we cleanup arch/* do_notify_resume() still does
>
> if (current->replacement_session_keyring)
> key_replace_session_keyring();
So the rename is still here.
Anything else? Please see the updated patch below.
Oleg.
>From 2ce2ff8e8715d0d25cc0228fa58f5721caebb780 Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <oleg@...hat.com>
Date: Thu, 12 Apr 2012 02:30:44 +0200
Subject: [PATCH] cred: 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 <oleg@...hat.com>
---
include/linux/key.h | 6 +--
security/keys/internal.h | 2 +
security/keys/keyctl.c | 73 ++++++++++++++++++++----------------------
security/keys/process_keys.c | 21 ++++--------
4 files changed, 46 insertions(+), 56 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index 96933b1..0c263d6 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
@@ -302,9 +304,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
*/
@@ -327,7 +326,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 65647f8..c2986da 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -14,6 +14,7 @@
#include <linux/sched.h>
#include <linux/key-type.h>
+#include <linux/task_work.h>
#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 fb767c6..55a451b 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1423,50 +1423,57 @@ long keyctl_get_security(key_serial_t keyid,
*/
long keyctl_session_to_parent(void)
{
-#ifdef TIF_NOTIFY_RESUME
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 */
@@ -1476,50 +1483,40 @@ 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;
-
-#else /* !TIF_NOTIFY_RESUME */
- /*
- * To be removed when TIF_NOTIFY_RESUME has been implemented on
- * m68k/xtensa
- */
-#warning TIF_NOTIFY_RESUME not implemented
- return -EOPNOTSUPP;
-#endif /* !TIF_NOTIFY_RESUME */
}
/*
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index be7ecb2..556dea2 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -832,23 +832,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)
- return;
-
- write_lock_irq(&tasklist_lock);
- new = current->replacement_session_keyring;
- current->replacement_session_keyring = NULL;
- write_unlock_irq(&tasklist_lock);
-
- if (!new)
+ kfree(twork);
+ if (unlikely(current->flags & PF_EXITING)) {
+ put_cred(new);
return;
+ }
- old = current_cred();
new-> uid = old-> uid;
new-> euid = old-> euid;
new-> suid = old-> suid;
@@ -873,6 +867,5 @@ void key_replace_session_keyring(void)
new->tgcred->process_keyring = key_get(old->tgcred->process_keyring);
security_transfer_creds(new, old);
-
commit_creds(new);
}
--
1.5.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists