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 <oleg@redhat.com> Acked-by: David Howells <dhowells@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Richard Kuo <rkuo@codeaurora.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Gordeev <agordeev@redhat.com> Cc: Chris Zankel <chris@zankel.net> Cc: David Smith <dsmith@redhat.com> Cc: "Frank Ch. Eigler" <fche@redhat.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Larry Woodman <lwoodman@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Tejun Heo <tj@kernel.org> Cc: Ingo Molnar <mingo@elte.hu> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

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 <oleg@redhat.com> Acked-by: David Howells <dhowells@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Richard Kuo <rkuo@codeaurora.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Gordeev <agordeev@redhat.com> Cc: Chris Zankel <chris@zankel.net> Cc: David Smith <dsmith@redhat.com> Cc: "Frank Ch. Eigler" <fche@redhat.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Larry Woodman <lwoodman@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Tejun Heo <tj@kernel.org> Cc: Ingo Molnar <mingo@elte.hu> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Oleg Nesterov · Al Viro
1 parent 4d1d61a6b2
Showing 4 changed files with 46 additions and 45 deletions Side-by-side Diff
include/linux/key.h
security/keys/internal.h
security/keys/keyctl.c
security/keys/process_keys.c
@@ -33,6 +33,8 @@
  
 struct key;
  
+#define key_replace_session_keyring()	do { } while (0)
+
 #ifdef CONFIG_KEYS
  
 #undef KEY_DEBUGGING
@@ -308,9 +310,6 @@
 #ifdef CONFIG_SYSCTL
 extern ctl_table key_sysctls[];
 #endif
-
-extern void key_replace_session_keyring(void);
-
 /*
  * the userspace interface
  */
@@ -334,7 +333,6 @@
 #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__ */
@@ -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 @@
 #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;
@@ -1456,47 +1456,55 @@
 {
 	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 @@
 	    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;
@@ -834,23 +834,17 @@
  * 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;
...	...	@@ -33,6 +33,8 @@
33	33
34	34	struct key;
35	35
	36	+#define key_replace_session_keyring() do { } while (0)
	37	+
36	38	#ifdef CONFIG_KEYS
37	39
38	40	#undef KEY_DEBUGGING
...	...	@@ -308,9 +310,6 @@
308	310	#ifdef CONFIG_SYSCTL
309	311	extern ctl_table key_sysctls[];
310	312	#endif
311		-
312		-extern void key_replace_session_keyring(void);
313		-
314	313	/*
315	314	* the userspace interface
316	315	*/
...	...	@@ -334,7 +333,6 @@
334	333	#define key_fsuid_changed(t) do { } while(0)
335	334	#define key_fsgid_changed(t) do { } while(0)
336	335	#define key_init() do { } while(0)
337		-#define key_replace_session_keyring() do { } while(0)
338	336
339	337	#endif /* CONFIG_KEYS */
340	338	#endif /* __KERNEL__ */
...	...	@@ -14,6 +14,7 @@
14	14
15	15	#include <linux/sched.h>
16	16	#include <linux/key-type.h>
	17	+#include <linux/task_work.h>
17	18
18	19	#ifdef __KDEBUG
19	20	#define kenter(FMT, ...) \
...	...	@@ -148,6 +149,7 @@
148	149	#define KEY_LOOKUP_FOR_UNLINK 0x04
149	150
150	151	extern long join_session_keyring(const char *name);
	152	+extern void key_change_session_keyring(struct task_work *twork);
151	153
152	154	extern struct work_struct key_gc_work;
153	155	extern unsigned key_gc_delay;
...	...	@@ -1456,47 +1456,55 @@
1456	1456	{
1457	1457	struct task_struct me, parent;
1458	1458	const struct cred mycred, pcred;
1459		- struct cred cred, oldcred;
	1459	+ struct task_work newwork, oldwork;
1460	1460	key_ref_t keyring_r;
	1461	+ struct cred *cred;
1461	1462	int ret;
1462	1463
1463	1464	keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_LINK);
1464	1465	if (IS_ERR(keyring_r))
1465	1466	return PTR_ERR(keyring_r);
1466	1467
	1468	+ ret = -ENOMEM;
	1469	+ newwork = kmalloc(sizeof(struct task_work), GFP_KERNEL);
	1470	+ if (!newwork)
	1471	+ goto error_keyring;
	1472	+
1467	1473	/* our parent is going to need a new cred struct, a new tgcred struct
1468	1474	* and new security data, so we allocate them here to prevent ENOMEM in
1469	1475	* our parent */
1470		- ret = -ENOMEM;
1471	1476	cred = cred_alloc_blank();
1472	1477	if (!cred)
1473		- goto error_keyring;
	1478	+ goto error_newwork;
1474	1479
1475	1480	cred->tgcred->session_keyring = key_ref_to_ptr(keyring_r);
1476		- keyring_r = NULL;
	1481	+ init_task_work(newwork, key_change_session_keyring, cred);
1477	1482
1478	1483	me = current;
1479	1484	rcu_read_lock();
1480	1485	write_lock_irq(&tasklist_lock);
1481	1486
1482		- parent = me->real_parent;
1483	1487	ret = -EPERM;
	1488	+ oldwork = NULL;
	1489	+ parent = me->real_parent;
1484	1490
1485	1491	/* the parent mustn't be init and mustn't be a kernel thread */
1486	1492	if (parent->pid <= 1 \|\| !parent->mm)
1487		- goto not_permitted;
	1493	+ goto unlock;
1488	1494
1489	1495	/* the parent must be single threaded */
1490	1496	if (!thread_group_empty(parent))
1491		- goto not_permitted;
	1497	+ goto unlock;
1492	1498
1493	1499	/* the parent and the child must have different session keyrings or
1494	1500	* there's no point */
1495	1501	mycred = current_cred();
1496	1502	pcred = __task_cred(parent);
1497	1503	if (mycred == pcred \|\|
1498		- mycred->tgcred->session_keyring == pcred->tgcred->session_keyring)
1499		- goto already_same;
	1504	+ mycred->tgcred->session_keyring == pcred->tgcred->session_keyring) {
	1505	+ ret = 0;
	1506	+ goto unlock;
	1507	+ }
1500	1508
1501	1509	/* the parent must have the same effective ownership and mustn't be
1502	1510	* SUID/SGID */
1503	1511
1504	1512
1505	1513
1506	1514
1507	1515
...	...	@@ -1506,38 +1514,37 @@
1506	1514	pcred->gid != mycred->egid \|\|
1507	1515	pcred->egid != mycred->egid \|\|
1508	1516	pcred->sgid != mycred->egid)
1509		- goto not_permitted;
	1517	+ goto unlock;
1510	1518
1511	1519	/* the keyrings must have the same UID */
1512	1520	if ((pcred->tgcred->session_keyring &&
1513	1521	pcred->tgcred->session_keyring->uid != mycred->euid) \|\|
1514	1522	mycred->tgcred->session_keyring->uid != mycred->euid)
1515		- goto not_permitted;
	1523	+ goto unlock;
1516	1524
1517		- /* if there's an already pending keyring replacement, then we replace
1518		- * that */
1519		- oldcred = parent->replacement_session_keyring;
	1525	+ /* cancel an already pending keyring replacement */
	1526	+ oldwork = task_work_cancel(parent, key_change_session_keyring);
1520	1527
1521	1528	/* the replacement session keyring is applied just prior to userspace
1522	1529	* restarting */
1523		- parent->replacement_session_keyring = cred;
1524		- cred = NULL;
1525		- set_ti_thread_flag(task_thread_info(parent), TIF_NOTIFY_RESUME);
1526		-
	1530	+ ret = task_work_add(parent, newwork, true);
	1531	+ if (!ret)
	1532	+ newwork = NULL;
	1533	+unlock:
1527	1534	write_unlock_irq(&tasklist_lock);
1528	1535	rcu_read_unlock();
1529		- if (oldcred)
1530		- put_cred(oldcred);
1531		- return 0;
1532		-
1533		-already_same:
1534		- ret = 0;
1535		-not_permitted:
1536		- write_unlock_irq(&tasklist_lock);
1537		- rcu_read_unlock();
1538		- put_cred(cred);
	1536	+ if (oldwork) {
	1537	+ put_cred(oldwork->data);
	1538	+ kfree(oldwork);
	1539	+ }
	1540	+ if (newwork) {
	1541	+ put_cred(newwork->data);
	1542	+ kfree(newwork);
	1543	+ }
1539	1544	return ret;
1540	1545
	1546	+error_newwork:
	1547	+ kfree(newwork);
1541	1548	error_keyring:
1542	1549	key_ref_put(keyring_r);
1543	1550	return ret;
...	...	@@ -834,23 +834,17 @@
834	834	* Replace a process's session keyring on behalf of one of its children when
835	835	* the target process is about to resume userspace execution.
836	836	*/
837		-void key_replace_session_keyring(void)
	837	+void key_change_session_keyring(struct task_work *twork)
838	838	{
839		- const struct cred *old;
840		- struct cred *new;
	839	+ const struct cred *old = current_cred();
	840	+ struct cred *new = twork->data;
841	841
842		- if (!current->replacement_session_keyring)
	842	+ kfree(twork);
	843	+ if (unlikely(current->flags & PF_EXITING)) {
	844	+ put_cred(new);
843	845	return;
	846	+ }
844	847
845		- write_lock_irq(&tasklist_lock);
846		- new = current->replacement_session_keyring;
847		- current->replacement_session_keyring = NULL;
848		- write_unlock_irq(&tasklist_lock);
849		-
850		- if (!new)
851		- return;
852		-
853		- old = current_cred();
854	848	new-> uid = old-> uid;
855	849	new-> euid = old-> euid;
856	850	new-> suid = old-> suid;