[PATCH 4.9 0/1] security,selinux,smack: kill security_task_wait hook

[PATCH 4.9 0/1] security,selinux,smack: kill security_task_wait hook

[Alexander Grund]

The following (backported) patch removes a hook which has already been removed upstream. [1]
Reason is that a permission denial can lead to soft lockups and zombies.
A reproducer can be found in the initial report. [2]
I hence consider this a bugfix which is allowed for stable branches.

Background:
To reduce divergence of 4.9 to upstream before proposing to apply upstream commit
3dfc9b02864bt (LSM: Initialize security_hook_heads upon registration.)
I'm checking which changes to the LSM hooks may be applicable to 4.9 as
doing so after backporting 33dfc9b02864bt will lead to conflicts for each such commit.

[1] https://patchwork.kernel.org/project/linux-security-module/patch/1484069312-26653-1-git-send-email-sds@tycho.nsa.gov/
[2] https://patchwork.kernel.org/project/selinux/patch/58736B2E.90201@huawei.com

Stephen Smalley (1):
  security,selinux,smack: kill security_task_wait hook

 include/linux/lsm_hooks.h  |  7 -------
 include/linux/security.h   |  6 ------
 kernel/exit.c              | 19 ++-----------------
 security/security.c        |  6 ------
 security/selinux/hooks.c   |  6 ------
 security/smack/smack_lsm.c | 20 --------------------
 6 files changed, 2 insertions(+), 62 deletions(-)

-- 
2.25.1

[PATCH 4.9 1/1] security,selinux,smack: kill security_task_wait hook

[Alexander Grund]

From: Stephen Smalley <sds@tycho.nsa.gov>

commit 3a2f5a59a695a73e0cde9a61e0feae5fa730e936 upstream.

As reported by yangshukui, a permission denial from security_task_wait()
can lead to a soft lockup in zap_pid_ns_processes() since it only expects
sys_wait4() to return 0 or -ECHILD. Further, security_task_wait() can
in general lead to zombies; in the absence of some way to automatically
reparent a child process upon a denial, the hook is not useful.  Remove
the security hook and its implementations in SELinux and Smack.  Smack
already removed its check from its hook.

Reported-by: yangshukui <yangshukui@huawei.com>
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
---
 include/linux/lsm_hooks.h  |  7 -------
 include/linux/security.h   |  6 ------
 kernel/exit.c              | 19 ++-----------------
 security/security.c        |  6 ------
 security/selinux/hooks.c   |  6 ------
 security/smack/smack_lsm.c | 20 --------------------
 6 files changed, 2 insertions(+), 62 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 53ac461f342b..491385a8a69d 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -666,11 +666,6 @@
  *	@sig contains the signal value.
  *	@secid contains the sid of the process where the signal originated
  *	Return 0 if permission is granted.
- * @task_wait:
- *	Check permission before allowing a process to reap a child process @p
- *	and collect its status information.
- *	@p contains the task_struct for process.
- *	Return 0 if permission is granted.
  * @task_prctl:
  *	Check permission before performing a process control operation on the
  *	current process.
@@ -1507,7 +1502,6 @@ union security_list_options {
 	int (*task_movememory)(struct task_struct *p);
 	int (*task_kill)(struct task_struct *p, struct siginfo *info,
 				int sig, u32 secid);
-	int (*task_wait)(struct task_struct *p);
 	int (*task_prctl)(int option, unsigned long arg2, unsigned long arg3,
 				unsigned long arg4, unsigned long arg5);
 	void (*task_to_inode)(struct task_struct *p, struct inode *inode);
@@ -1768,7 +1762,6 @@ struct security_hook_heads {
 	struct list_head task_getscheduler;
 	struct list_head task_movememory;
 	struct list_head task_kill;
-	struct list_head task_wait;
 	struct list_head task_prctl;
 	struct list_head task_to_inode;
 	struct list_head ipc_permission;
diff --git a/include/linux/security.h b/include/linux/security.h
index 2f5d282bd3ec..472822a1e02b 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -332,7 +332,6 @@ int security_task_getscheduler(struct task_struct *p);
 int security_task_movememory(struct task_struct *p);
 int security_task_kill(struct task_struct *p, struct siginfo *info,
 			int sig, u32 secid);
-int security_task_wait(struct task_struct *p);
 int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			unsigned long arg4, unsigned long arg5);
 void security_task_to_inode(struct task_struct *p, struct inode *inode);
@@ -980,11 +979,6 @@ static inline int security_task_kill(struct task_struct *p,
 	return 0;
 }
 
-static inline int security_task_wait(struct task_struct *p)
-{
-	return 0;
-}
-
 static inline int security_task_prctl(int option, unsigned long arg2,
 				      unsigned long arg3,
 				      unsigned long arg4,
diff --git a/kernel/exit.c b/kernel/exit.c
index 8716f0780fe3..e0db254a405b 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -14,7 +14,6 @@
 #include <linux/tty.h>
 #include <linux/iocontext.h>
 #include <linux/key.h>
-#include <linux/security.h>
 #include <linux/cpu.h>
 #include <linux/acct.h>
 #include <linux/tsacct_kern.h>
@@ -1342,7 +1341,7 @@ static int wait_task_continued(struct wait_opts *wo, struct task_struct *p)
  * Returns nonzero for a final return, when we have unlocked tasklist_lock.
  * Returns zero if the search for a child should continue;
  * then ->notask_error is 0 if @p is an eligible child,
- * or another error from security_task_wait(), or still -ECHILD.
+ * or still -ECHILD.
  */
 static int wait_consider_task(struct wait_opts *wo, int ptrace,
 				struct task_struct *p)
@@ -1362,20 +1361,6 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
 	if (!ret)
 		return ret;
 
-	ret = security_task_wait(p);
-	if (unlikely(ret < 0)) {
-		/*
-		 * If we have not yet seen any eligible child,
-		 * then let this error code replace -ECHILD.
-		 * A permission error will give the user a clue
-		 * to look for security policy problems, rather
-		 * than for mysterious wait bugs.
-		 */
-		if (wo->notask_error)
-			wo->notask_error = ret;
-		return 0;
-	}
-
 	if (unlikely(exit_state == EXIT_TRACE)) {
 		/*
 		 * ptrace == 0 means we are the natural parent. In this case
@@ -1468,7 +1453,7 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
  * Returns nonzero for a final return, when we have unlocked tasklist_lock.
  * Returns zero if the search for a child should continue; then
  * ->notask_error is 0 if there were any eligible children,
- * or another error from security_task_wait(), or still -ECHILD.
+ * or still -ECHILD.
  */
 static int do_wait_thread(struct wait_opts *wo, struct task_struct *tsk)
 {
diff --git a/security/security.c b/security/security.c
index 9a13d72a6446..5171c3cd1d30 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1032,11 +1032,6 @@ int security_task_kill(struct task_struct *p, struct siginfo *info,
 	return call_int_hook(task_kill, 0, p, info, sig, secid);
 }
 
-int security_task_wait(struct task_struct *p)
-{
-	return call_int_hook(task_wait, 0, p);
-}
-
 int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			 unsigned long arg4, unsigned long arg5)
 {
@@ -1776,7 +1771,6 @@ struct security_hook_heads security_hook_heads = {
 	.task_movememory =
 		LIST_HEAD_INIT(security_hook_heads.task_movememory),
 	.task_kill =	LIST_HEAD_INIT(security_hook_heads.task_kill),
-	.task_wait =	LIST_HEAD_INIT(security_hook_heads.task_wait),
 	.task_prctl =	LIST_HEAD_INIT(security_hook_heads.task_prctl),
 	.task_to_inode =
 		LIST_HEAD_INIT(security_hook_heads.task_to_inode),
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index eb9e2b4e81d9..eb503eccbacc 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3951,11 +3951,6 @@ static int selinux_task_kill(struct task_struct *p, struct siginfo *info,
 	return rc;
 }
 
-static int selinux_task_wait(struct task_struct *p)
-{
-	return task_has_perm(p, current, PROCESS__SIGCHLD);
-}
-
 static void selinux_task_to_inode(struct task_struct *p,
 				  struct inode *inode)
 {
@@ -6220,7 +6215,6 @@ static struct security_hook_list selinux_hooks[] = {
 	LSM_HOOK_INIT(task_getscheduler, selinux_task_getscheduler),
 	LSM_HOOK_INIT(task_movememory, selinux_task_movememory),
 	LSM_HOOK_INIT(task_kill, selinux_task_kill),
-	LSM_HOOK_INIT(task_wait, selinux_task_wait),
 	LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode),
 
 	LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission),
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 84ed47195cdd..f01b69ead47e 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2276,25 +2276,6 @@ static int smack_task_kill(struct task_struct *p, struct siginfo *info,
 	return rc;
 }
 
-/**
- * smack_task_wait - Smack access check for waiting
- * @p: task to wait for
- *
- * Returns 0
- */
-static int smack_task_wait(struct task_struct *p)
-{
-	/*
-	 * Allow the operation to succeed.
-	 * Zombies are bad.
-	 * In userless environments (e.g. phones) programs
-	 * get marked with SMACK64EXEC and even if the parent
-	 * and child shouldn't be talking the parent still
-	 * may expect to know when the child exits.
-	 */
-	return 0;
-}
-
 /**
  * smack_task_to_inode - copy task smack into the inode blob
  * @p: task to copy from
@@ -4686,7 +4667,6 @@ static struct security_hook_list smack_hooks[] = {
 	LSM_HOOK_INIT(task_getscheduler, smack_task_getscheduler),
 	LSM_HOOK_INIT(task_movememory, smack_task_movememory),
 	LSM_HOOK_INIT(task_kill, smack_task_kill),
-	LSM_HOOK_INIT(task_wait, smack_task_wait),
 	LSM_HOOK_INIT(task_to_inode, smack_task_to_inode),
 
 	LSM_HOOK_INIT(ipc_permission, smack_ipc_permission),
-- 
2.25.1

Re: [PATCH 4.9 1/1] security,selinux,smack: kill security_task_wait hook

[Alexander Grund]

Hi Greg,

after the previous discussion about what kind of patches are acceptable for stable
and your hints on how to send them to the ML in https://lore.kernel.org/all/YsrfDfe3urGkepvJ@kroah.com/
I'd like to know if this patch meets the requirements and if it can be considered.

I do have a few more similar ones which I think meet the stable requirements
and finally the init-cleanup patch
(upstream 3dfc9b02864bt "LSM: Initialize security_hook_heads upon registration.")
which I'd like to backport to 4.9. But first I want to know whether I now got
the formal requirements right before sending further patches.

Thanks,
Alex


On 11.07.22 11:56, Alexander Grund wrote:
> From: Stephen Smalley <sds@tycho.nsa.gov>
> 
> commit 3a2f5a59a695a73e0cde9a61e0feae5fa730e936 upstream.
> 
> As reported by yangshukui, a permission denial from security_task_wait()
> can lead to a soft lockup in zap_pid_ns_processes() since it only expects
> sys_wait4() to return 0 or -ECHILD. Further, security_task_wait() can
> in general lead to zombies; in the absence of some way to automatically
> reparent a child process upon a denial, the hook is not useful.  Remove
> the security hook and its implementations in SELinux and Smack.  Smack
> already removed its check from its hook.
> 
> <snip>

Re: [PATCH 4.9 1/1] security,selinux,smack: kill security_task_wait hook

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Sat, Jul 23, 2022 at 10:37:10AM +0200, Alexander Grund wrote:
> Hi Greg,
> 
> after the previous discussion about what kind of patches are acceptable for stable
> and your hints on how to send them to the ML in https://lore.kernel.org/all/YsrfDfe3urGkepvJ@kroah.com/
> I'd like to know if this patch meets the requirements and if it can be considered.

Yes, it is now queued up.  It was delayed due to the RETBLEED mess.

> I do have a few more similar ones which I think meet the stable requirements
> and finally the init-cleanup patch
> (upstream 3dfc9b02864bt "LSM: Initialize security_hook_heads upon registration.")
> which I'd like to backport to 4.9. But first I want to know whether I now got
> the formal requirements right before sending further patches.

Let's see what the other patches look like, I can't guarantee anything
without reviewing them first.

thanks,

greg k-h