From patchwork Tue Jan 10 17:28:32 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Smalley X-Patchwork-Id: 9508199 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id DD305606E1 for ; Tue, 10 Jan 2017 17:26:13 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CE46C28589 for ; Tue, 10 Jan 2017 17:26:13 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C2EE22858C; Tue, 10 Jan 2017 17:26:13 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A7D6828589 for ; Tue, 10 Jan 2017 17:26:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1764131AbdAJR0L (ORCPT ); Tue, 10 Jan 2017 12:26:11 -0500 Received: from smtp.nsa.gov ([8.44.101.8]:44049 "EHLO emsm-gh1-uea10.nsa.gov" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1764116AbdAJR0K (ORCPT ); Tue, 10 Jan 2017 12:26:10 -0500 X-IronPort-AV: E=Sophos;i="5.33,344,1477958400"; d="scan'208";a="2666845" IronPort-PHdr: =?us-ascii?q?9a23=3AV+EmDRzUV8ZIrxfXCy+O+j09IxM/srCxBDY+r6Qd?= =?us-ascii?q?0uoTKfad9pjvdHbS+e9qxAeQG96Kt7Qe1aGP6/iocFdDyK7JiGoFfp1IWk1Nou?= =?us-ascii?q?QttCtkPvS4D1bmJuXhdS0wEZcKflZk+3amLRodQ56mNBXdrXKo8DEdBAj0OxZr?= =?us-ascii?q?KeTpAI7SiNm82/yv95HJbQhFgDWwbalsIBi3ogndq9UajZd/Iast1xXFpWdFdf?= =?us-ascii?q?5Lzm1yP1KTmBj85sa0/JF99ilbpuws+c1dX6jkZqo0VbNXAigoPGAz/83rqALM?= =?us-ascii?q?TRCT6XsGU2UZiQRHDg7Y5xznRJjxsy/6tu1g2CmGOMD9UL45VSi+46ptVRTnhj?= =?us-ascii?q?0HNzok+2/JjMJ+gr9QrBa4qxBh34LYZYeYP+d8cKzAZ9MXXWpPUNhMWSJPAY2y?= =?us-ascii?q?aJACA/YdMetCs4XwvUcCoQe4CAKxBO3v0DhIhnru0KMnz+QuDxnG3Aw+ENIIrX?= =?us-ascii?q?/asdD1O70WUeCx0qbJzSjIYvRN2Tjg84jFaQwhoPGQUrJwdsrd008vFxjfgVmK?= =?us-ascii?q?pozlOC2V2/0LvmOG7ORgTfqih3Mopgx+uDSixtoghpPXio8a1FzI7zh1zYAoLt?= =?us-ascii?q?OiUkF7e8SrEJ5IuiGfMIt5X90tTnlzuCY/1r0GoZm7fDUWyJg/xx7QdfiHc4+Q?= =?us-ascii?q?7xL/TumROzZ4hG9+eL6lmxaz8VSvyu37VsWu1lZFsjFFncXWunAI1hzT7tCLSv?= =?us-ascii?q?p7/ki/xTaCzx3f5+5LLEwulafXNoQtzqA/m5YNq0jPAzf6mEDsg6+XckUk9PKo?= =?us-ascii?q?6+PiYrj+vZ+TKpR0hxriMqUuhsO/AeM4PhIIX2iA4+uwzrLj/UrnQLlSlP05jr?= =?us-ascii?q?HZsIzGJcQcvqO5ARVa0oM95BakFTum1M4UnXwALFJfYhKHjpPpNkrJIPDiF/iw?= =?us-ascii?q?n1Csnylxy//aOb3hB43HLmLfn7f5YbZ990lcxRI3zdBe4ZJUF74ALOvoWkDvqN?= =?us-ascii?q?PYEwU5Mw2ow+fnEdl904QeVn+SAq+dLqzfqkGI5u0xLOmWfoMVuyjyK+Ij5/Hw?= =?us-ascii?q?iX81g1gdfbOm3ZEPcnC3AuxmI1mFYXrrmtoODX0FvhEgQ+3qk1CCSiJcZ3aoUK?= =?us-ascii?q?Ih6DE7DJypDZ3aSo+xmrONxju0HppTZmpeEFCDDW/od5mYW/cLcC+SJcthnSIL?= =?us-ascii?q?VbW6UY8uywyhtA/gxLp7NObb5ioYtZf73thv++LTjQ0y9SBzD8mF02CCVWd0nm?= =?us-ascii?q?wTRz82waB/olF9ylaY3Kh4nvxXD9JS6O1IUgsgKZHcyOl6AcjoWg3dZteJVEqm?= =?us-ascii?q?QtK+DDEpVN0x3tsObl1lG9q4kxDD2zOmA7oSl7yMHpw77LjQ0GT2J8Z4mD760/?= =?us-ascii?q?w5hkQiatNGKGnjg6l47QWVDInMwGuDkKP/TrgRxC7A8i+4yGOKuExJGFprXb7t?= =?us-ascii?q?QWEUZkyQq8/woEzFUen9WvwcLgJdxJvaeeNxYdrzgAADHq/u?= X-IPAS-Result: =?us-ascii?q?A2FMBAAUGHVY/wHyM5BdGwEBAQMBAQEJAQEBFgEBAQMBAQE?= =?us-ascii?q?JAQEBgw8BAQEBAR+BbJ5bBpFwgkSEGhqGCIIFUwEBAQEBAQEBAgECYCiCMxuCS?= =?us-ascii?q?VIogSmIYw2yRTomAoltMoYBjDkMgwwFiHWHIosMkVICikGGHAJIkhZYgQwGAhA?= =?us-ascii?q?HGw+EahyBfSA1iGYBAQE?= Received: from unknown (HELO tarius.tycho.ncsc.mil) ([144.51.242.1]) by emsm-gh1-uea10.nsa.gov with ESMTP; 10 Jan 2017 17:26:07 +0000 Received: from moss-pluto.infosec.tycho.ncsc.mil (moss-pluto [192.168.25.131]) by tarius.tycho.ncsc.mil (8.14.4/8.14.4) with ESMTP id v0AHQ153011161; Tue, 10 Jan 2017 12:26:03 -0500 From: Stephen Smalley To: paul@paul-moore.com Cc: selinux@tycho.nsa.gov, yangshukui@huawei.com, oleg@redhat.com, casey@schaufler-ca.com, linux-security-module@vger.kernel.org, james.l.morris@oracle.com, Stephen Smalley Subject: [PATCH] security,selinux,smack: kill security_task_wait hook Date: Tue, 10 Jan 2017 12:28:32 -0500 Message-Id: <1484069312-26653-1-git-send-email-sds@tycho.nsa.gov> X-Mailer: git-send-email 2.7.4 Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP 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 Signed-off-by: Stephen Smalley Acked-by: Casey Schaufler Acked-by: Oleg Nesterov Acked-by: Casey Schaufler --- include/linux/lsm_hooks.h | 7 ------- include/linux/security.h | 6 ------ kernel/exit.c | 19 ++----------------- security/security.c | 6 ------ security/selinux/hooks.c | 7 ------- security/smack/smack_lsm.c | 20 -------------------- 6 files changed, 2 insertions(+), 63 deletions(-) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 0dde959..6fe7a5c 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); @@ -1767,7 +1761,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 f4ebac1..d3868f2 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 8f14b86..60f2451 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -14,7 +14,6 @@ #include #include #include -#include #include #include #include @@ -1360,7 +1359,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) @@ -1380,20 +1379,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 @@ -1486,7 +1471,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 32052f5..8c9fee5 100644 --- a/security/security.c +++ b/security/security.c @@ -1025,11 +1025,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) { @@ -1769,7 +1764,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 bada3cd..720dbd0 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3969,12 +3969,6 @@ static int selinux_task_kill(struct task_struct *p, struct siginfo *info, return avc_has_perm(secid, task_sid(p), SECCLASS_PROCESS, perm, NULL); } -static int selinux_task_wait(struct task_struct *p) -{ - return avc_has_perm(task_sid(p), current_sid(), SECCLASS_PROCESS, - PROCESS__SIGCHLD, NULL); -} - static void selinux_task_to_inode(struct task_struct *p, struct inode *inode) { @@ -6217,7 +6211,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 8da4a6b..2166373 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -2272,25 +2272,6 @@ static int smack_task_kill(struct task_struct *p, struct siginfo *info, } /** - * 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 * @inode: inode to copy to @@ -4658,7 +4639,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),