[PATCH] Split bprm_apply_creds into two functions

[PATCH] Split bprm_apply_creds into two functions

[Serge E. Hallyn]

The security_bprm_apply_creds() function is called from
fs/exec.c:compute_creds() under task_lock(current).  SELinux must
perform some work which is unsafe in that context, and therefore
explicitly drops the task_lock, does the work, and re-acquires the
task_lock.  This is unsafe if other security modules are stacked after
SELinux, as their bprm_apply_creds assumes that the 'unsafe' variable is
still meaningful, that is, that the task_lock has not been dropped.

The attached patch splits bprm_apply_creds into two functions,
bprm_apply_creds and bprm_final_setup, where final_setup is called right
after the task_lock has been dropped.

thanks,
-serge

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Index: linux-2.6.10-rc3-mm1/fs/exec.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/fs/exec.c	2004-12-13 15:56:15.000000000 -0600
+++ linux-2.6.10-rc3-mm1/fs/exec.c	2004-12-13 17:57:35.071345712 -0600
@@ -962,6 +962,7 @@ void compute_creds(struct linux_binprm *
 	unsafe = unsafe_exec(current);
 	security_bprm_apply_creds(bprm, unsafe);
 	task_unlock(current);
+	security_bprm_final_setup(bprm);
 }
 
 EXPORT_SYMBOL(compute_creds);
Index: linux-2.6.10-rc3-mm1/include/linux/security.h
===================================================================
--- linux-2.6.10-rc3-mm1.orig/include/linux/security.h	2004-12-13 15:57:05.000000000 -0600
+++ linux-2.6.10-rc3-mm1/include/linux/security.h	2004-12-13 17:57:35.133336288 -0600
@@ -115,6 +115,11 @@ struct swap_info_struct;
  *	bprm_apply_creds is called under task_lock.  @unsafe indicates various
  *	reasons why it may be unsafe to change security state.
  *	@bprm contains the linux_binprm structure.
+ * @bprm_final_setup:
+ *	Runs after bprm_apply_creds with the task_lock dropped, so that
+ *	functions which cannot be called safely under the task_list can
+ *	be used.
+ *	@bprm contains the linux_binprm structure.
  * @bprm_set_security:
  *	Save security information in the bprm->security field, typically based
  *	on information about the bprm->file, for later use by the apply_creds
@@ -1046,6 +1051,7 @@ struct security_operations {
 	int (*bprm_alloc_security) (struct linux_binprm * bprm);
 	void (*bprm_free_security) (struct linux_binprm * bprm);
 	void (*bprm_apply_creds) (struct linux_binprm * bprm, int unsafe);
+	void (*bprm_final_setup) (struct linux_binprm * bprm);
 	int (*bprm_set_security) (struct linux_binprm * bprm);
 	int (*bprm_check_security) (struct linux_binprm * bprm);
 	int (*bprm_secureexec) (struct linux_binprm * bprm);
@@ -1319,6 +1325,10 @@ static inline void security_bprm_apply_c
 {
 	security_ops->bprm_apply_creds (bprm, unsafe);
 }
+static inline void security_bprm_final_setup (struct linux_binprm *bprm)
+{
+	security_ops->bprm_final_setup (bprm);
+}
 static inline int security_bprm_set (struct linux_binprm *bprm)
 {
 	return security_ops->bprm_set_security (bprm);
@@ -2001,6 +2011,11 @@ static inline void security_bprm_apply_c
 	cap_bprm_apply_creds (bprm, unsafe);
 }
 
+static inline void security_bprm_final_setup (struct linux_binprm *bprm)
+{ 
+	return;
+}
+
 static inline int security_bprm_set (struct linux_binprm *bprm)
 {
 	return cap_bprm_set_security (bprm);
Index: linux-2.6.10-rc3-mm1/security/dummy.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/dummy.c	2004-12-13 15:56:15.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/dummy.c	2004-12-13 17:57:35.194327016 -0600
@@ -201,6 +201,11 @@ static void dummy_bprm_apply_creds (stru
 	current->sgid = current->egid = current->fsgid = bprm->e_gid;
 }
 
+static void dummy_bprm_final_setup (struct linux_binprm *bprm)
+{
+	return;
+}
+
 static int dummy_bprm_set_security (struct linux_binprm *bprm)
 {
 	return 0;
@@ -921,6 +926,7 @@ void security_fixup_ops (struct security
 	set_to_dummy_if_null(ops, bprm_alloc_security);
 	set_to_dummy_if_null(ops, bprm_free_security);
 	set_to_dummy_if_null(ops, bprm_apply_creds);
+	set_to_dummy_if_null(ops, bprm_final_setup);
 	set_to_dummy_if_null(ops, bprm_set_security);
 	set_to_dummy_if_null(ops, bprm_check_security);
 	set_to_dummy_if_null(ops, bprm_secureexec);
Index: linux-2.6.10-rc3-mm1/security/selinux/hooks.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/selinux/hooks.c	2004-12-13 15:56:15.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/selinux/hooks.c	2004-12-13 17:57:27.544489968 -0600
@@ -1804,10 +1804,7 @@ static void selinux_bprm_apply_creds(str
 	struct task_security_struct *tsec;
 	struct bprm_security_struct *bsec;
 	u32 sid;
-	struct av_decision avd;
-	struct itimerval itimer;
-	struct rlimit *rlim, *initrlim;
-	int rc, i;
+	int rc;
 
 	secondary_ops->bprm_apply_creds(bprm, unsafe);
 
@@ -1817,91 +1814,99 @@ static void selinux_bprm_apply_creds(str
 	sid = bsec->sid;
 
 	tsec->osid = tsec->sid;
+	tsec->unsafe = 0;
 	if (tsec->sid != sid) {
 		/* Check for shared state.  If not ok, leave SID
 		   unchanged and kill. */
 		if (unsafe & LSM_UNSAFE_SHARE) {
-			rc = avc_has_perm_noaudit(tsec->sid, sid,
-					  SECCLASS_PROCESS, PROCESS__SHARE, &avd);
+			rc = avc_has_perm(tsec->sid, sid, SECCLASS_PROCESS,
+					PROCESS__SHARE, NULL);
 			if (rc) {
-				task_unlock(current);
-				avc_audit(tsec->sid, sid, SECCLASS_PROCESS,
-				    PROCESS__SHARE, &avd, rc, NULL);
-				force_sig_specific(SIGKILL, current);
-				goto lock_out;
+				tsec->unsafe = 1;
+				return;
 			}
 		}
 
 		/* Check for ptracing, and update the task SID if ok.
 		   Otherwise, leave SID unchanged and kill. */
 		if (unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
-			rc = avc_has_perm_noaudit(tsec->ptrace_sid, sid,
-					  SECCLASS_PROCESS, PROCESS__PTRACE, &avd);
-			if (!rc)
-				tsec->sid = sid;
-			task_unlock(current);
-			avc_audit(tsec->ptrace_sid, sid, SECCLASS_PROCESS,
-				  PROCESS__PTRACE, &avd, rc, NULL);
+			rc = avc_has_perm(tsec->ptrace_sid, sid,
+					  SECCLASS_PROCESS, PROCESS__PTRACE,
+					  NULL);
 			if (rc) {
-				force_sig_specific(SIGKILL, current);
-				goto lock_out;
+				tsec->unsafe = 1;
+				return;
 			}
-		} else {
-			tsec->sid = sid;
-			task_unlock(current);
 		}
+		tsec->sid = sid;
+	}
+}
 
-		/* Close files for which the new task SID is not authorized. */
-		flush_unauthorized_files(current->files);
+/*
+ * called after apply_creds without the task lock held
+ */
+static void selinux_bprm_final_setup(struct linux_binprm *bprm)
+{
+	struct task_security_struct *tsec;
+	struct rlimit *rlim, *initrlim;
+	struct itimerval itimer;
+	int rc, i;
 
-		/* Check whether the new SID can inherit signal state
-		   from the old SID.  If not, clear itimers to avoid
-		   subsequent signal generation and flush and unblock
-		   signals. This must occur _after_ the task SID has
-                  been updated so that any kill done after the flush
-                  will be checked against the new SID. */
-		rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
-				  PROCESS__SIGINH, NULL);
-		if (rc) {
-			memset(&itimer, 0, sizeof itimer);
-			for (i = 0; i < 3; i++)
-				do_setitimer(i, &itimer, NULL);
-			flush_signals(current);
-			spin_lock_irq(&current->sighand->siglock);
-			flush_signal_handlers(current, 1);
-			sigemptyset(&current->blocked);
-			recalc_sigpending();
-			spin_unlock_irq(&current->sighand->siglock);
-		}
+	tsec = current->security;
 
-		/* Check whether the new SID can inherit resource limits
-		   from the old SID.  If not, reset all soft limits to
-		   the lower of the current task's hard limit and the init
-		   task's soft limit.  Note that the setting of hard limits 
-		   (even to lower them) can be controlled by the setrlimit 
-		   check. The inclusion of the init task's soft limit into
-	           the computation is to avoid resetting soft limits higher
-		   than the default soft limit for cases where the default
-		   is lower than the hard limit, e.g. RLIMIT_CORE or 
-		   RLIMIT_STACK.*/
-		rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
-				  PROCESS__RLIMITINH, NULL);
-		if (rc) {
-			for (i = 0; i < RLIM_NLIMITS; i++) {
-				rlim = current->signal->rlim + i;
-				initrlim = init_task.signal->rlim+i;
-				rlim->rlim_cur = min(rlim->rlim_max,initrlim->rlim_cur);
-			}
-		}
+	if (tsec->unsafe) {
+		force_sig_specific(SIGKILL, current);
+		return;
+	}
+	if (tsec->osid == tsec->sid)
+		return;
 
-		/* Wake up the parent if it is waiting so that it can
-		   recheck wait permission to the new task SID. */
-		wake_up_interruptible(&current->parent->signal->wait_chldexit);
+	/* Close files for which the new task SID is not authorized. */
+	flush_unauthorized_files(current->files);
 
-lock_out:
-		task_lock(current);
-		return;
+	/* Check whether the new SID can inherit signal state
+	   from the old SID.  If not, clear itimers to avoid
+	   subsequent signal generation and flush and unblock
+	   signals. This must occur _after_ the task SID has
+	  been updated so that any kill done after the flush
+	  will be checked against the new SID. */
+	rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
+			  PROCESS__SIGINH, NULL);
+	if (rc) {
+		memset(&itimer, 0, sizeof itimer);
+		for (i = 0; i < 3; i++)
+			do_setitimer(i, &itimer, NULL);
+		flush_signals(current);
+		spin_lock_irq(&current->sighand->siglock);
+		flush_signal_handlers(current, 1);
+		sigemptyset(&current->blocked);
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
+	}
+
+	/* Check whether the new SID can inherit resource limits
+	   from the old SID.  If not, reset all soft limits to
+	   the lower of the current task's hard limit and the init
+	   task's soft limit.  Note that the setting of hard limits 
+	   (even to lower them) can be controlled by the setrlimit 
+	   check. The inclusion of the init task's soft limit into
+	   the computation is to avoid resetting soft limits higher
+	   than the default soft limit for cases where the default
+	   is lower than the hard limit, e.g. RLIMIT_CORE or 
+	   RLIMIT_STACK.*/
+	rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
+			  PROCESS__RLIMITINH, NULL);
+	if (rc) {
+		for (i = 0; i < RLIM_NLIMITS; i++) {
+			rlim = current->signal->rlim + i;
+			initrlim = init_task.signal->rlim+i;
+			rlim->rlim_cur = min(rlim->rlim_max,initrlim->rlim_cur);
+		}
 	}
+
+	/* Wake up the parent if it is waiting so that it can
+	   recheck wait permission to the new task SID. */
+	wake_up_interruptible(&current->parent->signal->wait_chldexit);
 }
 
 /* superblock security operations */
@@ -4229,6 +4234,7 @@ struct security_operations selinux_ops =
 	.bprm_alloc_security =		selinux_bprm_alloc_security,
 	.bprm_free_security =		selinux_bprm_free_security,
 	.bprm_apply_creds =		selinux_bprm_apply_creds,
+	.bprm_final_setup =		selinux_bprm_final_setup,
 	.bprm_set_security =		selinux_bprm_set_security,
 	.bprm_check_security =		selinux_bprm_check_security,
 	.bprm_secureexec =		selinux_bprm_secureexec,
Index: linux-2.6.10-rc3-mm1/security/selinux/include/objsec.h
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/selinux/include/objsec.h	2004-12-13 15:56:15.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/selinux/include/objsec.h	2004-12-13 17:57:27.573485560 -0600
@@ -34,6 +34,12 @@ struct task_security_struct {
 	u32 exec_sid;        /* exec SID */
 	u32 create_sid;      /* fscreate SID */
 	u32 ptrace_sid;      /* SID of ptrace parent */
+
+	/*
+	 * used to share failure information from bprm_apply_creds()
+	 * to bprm_final_setup().
+	 */
+	char unsafe;
 };
 
 struct inode_security_struct {

Re: [PATCH] Split bprm_apply_creds into two functions

[Stephen Smalley]

On Wed, 2004-12-15 at 15:00, Serge E. Hallyn wrote:
> The security_bprm_apply_creds() function is called from
> fs/exec.c:compute_creds() under task_lock(current).  SELinux must
> perform some work which is unsafe in that context, and therefore
> explicitly drops the task_lock, does the work, and re-acquires the
> task_lock.  This is unsafe if other security modules are stacked after
> SELinux, as their bprm_apply_creds assumes that the 'unsafe' variable is
> still meaningful, that is, that the task_lock has not been dropped.
> 
> The attached patch splits bprm_apply_creds into two functions,
> bprm_apply_creds and bprm_final_setup, where final_setup is called right
> after the task_lock has been dropped.

Two minor notes:
1) I'd suggest moving the unsafe field from task_security_struct to
bprm_security_struct, as there is no reason for this flag to live beyond
the lifecycle of the binprm.
2) Comment in security.h says 'task_list' where it should be
'task_lock'.

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH] Split bprm_apply_creds into two functions

[Serge E. Hallyn]

Quoting Stephen Smalley (sds@epoch.ncsc.mil):
> Two minor notes:
> 1) I'd suggest moving the unsafe field from task_security_struct to
> bprm_security_struct, as there is no reason for this flag to live beyond
> the lifecycle of the binprm.
> 2) Comment in security.h says 'task_list' where it should be
> 'task_lock'.

Thanks.  Here is an updated patch.

-serge

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Index: linux-2.6.10-rc3-mm1/fs/exec.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/fs/exec.c	2004-12-15 15:57:23.000000000 -0600
+++ linux-2.6.10-rc3-mm1/fs/exec.c	2004-12-16 12:53:29.000000000 -0600
@@ -962,6 +962,7 @@ void compute_creds(struct linux_binprm *
 	unsafe = unsafe_exec(current);
 	security_bprm_apply_creds(bprm, unsafe);
 	task_unlock(current);
+	security_bprm_final_setup(bprm);
 }
 
 EXPORT_SYMBOL(compute_creds);
Index: linux-2.6.10-rc3-mm1/include/linux/security.h
===================================================================
--- linux-2.6.10-rc3-mm1.orig/include/linux/security.h	2004-12-15 17:14:39.000000000 -0600
+++ linux-2.6.10-rc3-mm1/include/linux/security.h	2004-12-16 13:36:44.000000000 -0600
@@ -116,6 +116,11 @@ struct swap_info_struct;
  *	bprm_apply_creds is called under task_lock.  @unsafe indicates various
  *	reasons why it may be unsafe to change security state.
  *	@bprm contains the linux_binprm structure.
+ * @bprm_final_setup:
+ *	Runs after bprm_apply_creds with the task_lock dropped, so that
+ *	functions which cannot be called safely under the task_lock can
+ *	be used.
+ *	@bprm contains the linux_binprm structure.
  * @bprm_set_security:
  *	Save security information in the bprm->security field, typically based
  *	on information about the bprm->file, for later use by the apply_creds
@@ -1047,6 +1052,7 @@ struct security_operations {
 	int (*bprm_alloc_security) (struct linux_binprm * bprm);
 	void (*bprm_free_security) (struct linux_binprm * bprm);
 	void (*bprm_apply_creds) (struct linux_binprm * bprm, int unsafe);
+	void (*bprm_final_setup) (struct linux_binprm * bprm);
 	int (*bprm_set_security) (struct linux_binprm * bprm);
 	int (*bprm_check_security) (struct linux_binprm * bprm);
 	int (*bprm_secureexec) (struct linux_binprm * bprm);
@@ -1320,6 +1326,10 @@ static inline void security_bprm_apply_c
 {
 	security_ops->bprm_apply_creds (bprm, unsafe);
 }
+static inline void security_bprm_final_setup (struct linux_binprm *bprm)
+{
+	security_ops->bprm_final_setup (bprm);
+}
 static inline int security_bprm_set (struct linux_binprm *bprm)
 {
 	return security_ops->bprm_set_security (bprm);
@@ -2003,6 +2013,11 @@ static inline void security_bprm_apply_c
 	cap_bprm_apply_creds (bprm, unsafe);
 }
 
+static inline void security_bprm_final_setup (struct linux_binprm *bprm)
+{ 
+	return;
+}
+
 static inline int security_bprm_set (struct linux_binprm *bprm)
 {
 	return cap_bprm_set_security (bprm);
Index: linux-2.6.10-rc3-mm1/security/dummy.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/dummy.c	2004-12-15 15:57:23.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/dummy.c	2004-12-16 12:53:29.000000000 -0600
@@ -201,6 +201,11 @@ static void dummy_bprm_apply_creds (stru
 	current->sgid = current->egid = current->fsgid = bprm->e_gid;
 }
 
+static void dummy_bprm_final_setup (struct linux_binprm *bprm)
+{
+	return;
+}
+
 static int dummy_bprm_set_security (struct linux_binprm *bprm)
 {
 	return 0;
@@ -921,6 +926,7 @@ void security_fixup_ops (struct security
 	set_to_dummy_if_null(ops, bprm_alloc_security);
 	set_to_dummy_if_null(ops, bprm_free_security);
 	set_to_dummy_if_null(ops, bprm_apply_creds);
+	set_to_dummy_if_null(ops, bprm_final_setup);
 	set_to_dummy_if_null(ops, bprm_set_security);
 	set_to_dummy_if_null(ops, bprm_check_security);
 	set_to_dummy_if_null(ops, bprm_secureexec);
Index: linux-2.6.10-rc3-mm1/security/selinux/hooks.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/selinux/hooks.c	2004-12-15 17:25:55.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/selinux/hooks.c	2004-12-16 12:54:33.000000000 -0600
@@ -1810,10 +1810,7 @@ static void selinux_bprm_apply_creds(str
 	struct task_security_struct *tsec;
 	struct bprm_security_struct *bsec;
 	u32 sid;
-	struct av_decision avd;
-	struct itimerval itimer;
-	struct rlimit *rlim, *initrlim;
-	int rc, i;
+	int rc;
 
 	secondary_ops->bprm_apply_creds(bprm, unsafe);
 
@@ -1823,91 +1820,101 @@ static void selinux_bprm_apply_creds(str
 	sid = bsec->sid;
 
 	tsec->osid = tsec->sid;
+	bsec->unsafe = 0;
 	if (tsec->sid != sid) {
 		/* Check for shared state.  If not ok, leave SID
 		   unchanged and kill. */
 		if (unsafe & LSM_UNSAFE_SHARE) {
-			rc = avc_has_perm_noaudit(tsec->sid, sid,
-					  SECCLASS_PROCESS, PROCESS__SHARE, &avd);
+			rc = avc_has_perm(tsec->sid, sid, SECCLASS_PROCESS,
+					PROCESS__SHARE, NULL);
 			if (rc) {
-				task_unlock(current);
-				avc_audit(tsec->sid, sid, SECCLASS_PROCESS,
-				    PROCESS__SHARE, &avd, rc, NULL);
-				force_sig_specific(SIGKILL, current);
-				goto lock_out;
+				bsec->unsafe = 1;
+				return;
 			}
 		}
 
 		/* Check for ptracing, and update the task SID if ok.
 		   Otherwise, leave SID unchanged and kill. */
 		if (unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
-			rc = avc_has_perm_noaudit(tsec->ptrace_sid, sid,
-					  SECCLASS_PROCESS, PROCESS__PTRACE, &avd);
-			if (!rc)
-				tsec->sid = sid;
-			task_unlock(current);
-			avc_audit(tsec->ptrace_sid, sid, SECCLASS_PROCESS,
-				  PROCESS__PTRACE, &avd, rc, NULL);
+			rc = avc_has_perm(tsec->ptrace_sid, sid,
+					  SECCLASS_PROCESS, PROCESS__PTRACE,
+					  NULL);
 			if (rc) {
-				force_sig_specific(SIGKILL, current);
-				goto lock_out;
+				bsec->unsafe = 1;
+				return;
 			}
-		} else {
-			tsec->sid = sid;
-			task_unlock(current);
 		}
+		tsec->sid = sid;
+	}
+}
 
-		/* Close files for which the new task SID is not authorized. */
-		flush_unauthorized_files(current->files);
+/*
+ * called after apply_creds without the task lock held
+ */
+static void selinux_bprm_final_setup(struct linux_binprm *bprm)
+{
+	struct task_security_struct *tsec;
+	struct rlimit *rlim, *initrlim;
+	struct itimerval itimer;
+	struct bprm_security_struct *bsec;
+	int rc, i;
 
-		/* Check whether the new SID can inherit signal state
-		   from the old SID.  If not, clear itimers to avoid
-		   subsequent signal generation and flush and unblock
-		   signals. This must occur _after_ the task SID has
-                  been updated so that any kill done after the flush
-                  will be checked against the new SID. */
-		rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
-				  PROCESS__SIGINH, NULL);
-		if (rc) {
-			memset(&itimer, 0, sizeof itimer);
-			for (i = 0; i < 3; i++)
-				do_setitimer(i, &itimer, NULL);
-			flush_signals(current);
-			spin_lock_irq(&current->sighand->siglock);
-			flush_signal_handlers(current, 1);
-			sigemptyset(&current->blocked);
-			recalc_sigpending();
-			spin_unlock_irq(&current->sighand->siglock);
-		}
+	tsec = current->security;
+	bsec = bprm->security;
 
-		/* Check whether the new SID can inherit resource limits
-		   from the old SID.  If not, reset all soft limits to
-		   the lower of the current task's hard limit and the init
-		   task's soft limit.  Note that the setting of hard limits 
-		   (even to lower them) can be controlled by the setrlimit 
-		   check. The inclusion of the init task's soft limit into
-	           the computation is to avoid resetting soft limits higher
-		   than the default soft limit for cases where the default
-		   is lower than the hard limit, e.g. RLIMIT_CORE or 
-		   RLIMIT_STACK.*/
-		rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
-				  PROCESS__RLIMITINH, NULL);
-		if (rc) {
-			for (i = 0; i < RLIM_NLIMITS; i++) {
-				rlim = current->signal->rlim + i;
-				initrlim = init_task.signal->rlim+i;
-				rlim->rlim_cur = min(rlim->rlim_max,initrlim->rlim_cur);
-			}
-		}
+	if (bsec->unsafe) {
+		force_sig_specific(SIGKILL, current);
+		return;
+	}
+	if (tsec->osid == tsec->sid)
+		return;
 
-		/* Wake up the parent if it is waiting so that it can
-		   recheck wait permission to the new task SID. */
-		wake_up_interruptible(&current->parent->signal->wait_chldexit);
+	/* Close files for which the new task SID is not authorized. */
+	flush_unauthorized_files(current->files);
 
-lock_out:
-		task_lock(current);
-		return;
+	/* Check whether the new SID can inherit signal state
+	   from the old SID.  If not, clear itimers to avoid
+	   subsequent signal generation and flush and unblock
+	   signals. This must occur _after_ the task SID has
+	  been updated so that any kill done after the flush
+	  will be checked against the new SID. */
+	rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
+			  PROCESS__SIGINH, NULL);
+	if (rc) {
+		memset(&itimer, 0, sizeof itimer);
+		for (i = 0; i < 3; i++)
+			do_setitimer(i, &itimer, NULL);
+		flush_signals(current);
+		spin_lock_irq(&current->sighand->siglock);
+		flush_signal_handlers(current, 1);
+		sigemptyset(&current->blocked);
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
+	}
+
+	/* Check whether the new SID can inherit resource limits
+	   from the old SID.  If not, reset all soft limits to
+	   the lower of the current task's hard limit and the init
+	   task's soft limit.  Note that the setting of hard limits 
+	   (even to lower them) can be controlled by the setrlimit 
+	   check. The inclusion of the init task's soft limit into
+	   the computation is to avoid resetting soft limits higher
+	   than the default soft limit for cases where the default
+	   is lower than the hard limit, e.g. RLIMIT_CORE or 
+	   RLIMIT_STACK.*/
+	rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
+			  PROCESS__RLIMITINH, NULL);
+	if (rc) {
+		for (i = 0; i < RLIM_NLIMITS; i++) {
+			rlim = current->signal->rlim + i;
+			initrlim = init_task.signal->rlim+i;
+			rlim->rlim_cur = min(rlim->rlim_max,initrlim->rlim_cur);
+		}
 	}
+
+	/* Wake up the parent if it is waiting so that it can
+	   recheck wait permission to the new task SID. */
+	wake_up_interruptible(&current->parent->signal->wait_chldexit);
 }
 
 /* superblock security operations */
@@ -4235,6 +4242,7 @@ struct security_operations selinux_ops =
 	.bprm_alloc_security =		selinux_bprm_alloc_security,
 	.bprm_free_security =		selinux_bprm_free_security,
 	.bprm_apply_creds =		selinux_bprm_apply_creds,
+	.bprm_final_setup =		selinux_bprm_final_setup,
 	.bprm_set_security =		selinux_bprm_set_security,
 	.bprm_check_security =		selinux_bprm_check_security,
 	.bprm_secureexec =		selinux_bprm_secureexec,
Index: linux-2.6.10-rc3-mm1/security/selinux/include/objsec.h
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/selinux/include/objsec.h	2004-12-15 15:57:23.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/selinux/include/objsec.h	2004-12-16 12:53:48.000000000 -0600
@@ -87,6 +87,12 @@ struct bprm_security_struct {
 	struct linux_binprm *bprm;     /* back pointer to bprm object */
 	u32 sid;                       /* SID for transformed process */
 	unsigned char set;
+
+	/*
+	 * used to share failure information from bprm_apply_creds()
+	 * to bprm_final_setup().
+	 */
+	char unsafe;
 };
 
 struct netif_security_struct {

Re: [PATCH] Split bprm_apply_creds into two functions

[Stephen Smalley]

On Thu, 2004-12-16 at 13:25, Serge E. Hallyn wrote:
> Thanks.  Here is an updated patch.
> 
> -serge
> 
> Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Ok with me.  Chris, would it help alleviate your concerns to give the
hook a clearer name and description, e.g. bprm_post_apply_creds and move
the discussion about performing other state changes on the process like
closing descriptors from the current description of bprm_apply_creds to
it?

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH] Split bprm_apply_creds into two functions

[Chris Wright]

* Stephen Smalley (sds@epoch.ncsc.mil) wrote:
> On Thu, 2004-12-16 at 13:25, Serge E. Hallyn wrote:
> > Thanks.  Here is an updated patch.
> > 
> > -serge
> > 
> > Signed-off-by: Serge Hallyn <serue@us.ibm.com>
> 
> Ok with me.  Chris, would it help alleviate your concerns to give the
> hook a clearer name and description, e.g. bprm_post_apply_creds and move
> the discussion about performing other state changes on the process like
> closing descriptors from the current description of bprm_apply_creds to
> it?

Yes.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] Split bprm_apply_creds into two functions

[Serge E. Hallyn]

The attached version renames bprm_final_setup to bprm_post_apply_creds
and moves the comment about closing file descriptors from the
bprm_apply_creds description to the bprm_post_apply_creds description
in security.h.

thanks,
-serge

Quoting Chris Wright (chrisw@osdl.org):
> * Stephen Smalley (sds@epoch.ncsc.mil) wrote:
> > On Thu, 2004-12-16 at 13:25, Serge E. Hallyn wrote:
> > > Thanks.  Here is an updated patch.
> > > 
> > > -serge
> > > 
> > > Signed-off-by: Serge Hallyn <serue@us.ibm.com>
> > 
> > Ok with me.  Chris, would it help alleviate your concerns to give the
> > hook a clearer name and description, e.g. bprm_post_apply_creds and move
> > the discussion about performing other state changes on the process like
> > closing descriptors from the current description of bprm_apply_creds to
> > it?
> 
> Yes.
> 
> thanks,
> -chris

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Index: linux-2.6.10-rc3-mm1/fs/exec.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/fs/exec.c	2004-12-15 15:57:23.000000000 -0600
+++ linux-2.6.10-rc3-mm1/fs/exec.c	2004-12-17 13:07:54.000000000 -0600
@@ -962,6 +962,7 @@ void compute_creds(struct linux_binprm *
 	unsafe = unsafe_exec(current);
 	security_bprm_apply_creds(bprm, unsafe);
 	task_unlock(current);
+	security_bprm_post_apply_creds(bprm);
 }
 
 EXPORT_SYMBOL(compute_creds);
Index: linux-2.6.10-rc3-mm1/include/linux/security.h
===================================================================
--- linux-2.6.10-rc3-mm1.orig/include/linux/security.h	2004-12-15 17:14:39.000000000 -0600
+++ linux-2.6.10-rc3-mm1/include/linux/security.h	2004-12-17 14:40:35.000000000 -0600
@@ -109,13 +109,20 @@ struct swap_info_struct;
  *	and the information saved in @bprm->security by the set_security hook.
  *	Since this hook function (and its caller) are void, this hook can not
  *	return an error.  However, it can leave the security attributes of the
- *	process unchanged if an access failure occurs at this point. It can
- *	also perform other state changes on the process (e.g.  closing open
- *	file descriptors to which access is no longer granted if the attributes
- *	were changed). 
+ *	process unchanged if an access failure occurs at this point.
  *	bprm_apply_creds is called under task_lock.  @unsafe indicates various
  *	reasons why it may be unsafe to change security state.
  *	@bprm contains the linux_binprm structure.
+ * @bprm_post_apply_creds:
+ *	Runs after bprm_apply_creds with the task_lock dropped, so that
+ *	functions which cannot be called safely under the task_lock can
+ *	be used.  This hook is a good place to perform state changes on
+ *	the process such as closing open file descriptors to which access
+ *	is no longer granted if the attributes were changed). 
+ *	Note that a security module might need to save state between
+ *	bprm_apply_creds and bprm_post_apply_creds to store the decision
+ *	on whether the process may proceed.
+ *	@bprm contains the linux_binprm structure.
  * @bprm_set_security:
  *	Save security information in the bprm->security field, typically based
  *	on information about the bprm->file, for later use by the apply_creds
@@ -1047,6 +1054,7 @@ struct security_operations {
 	int (*bprm_alloc_security) (struct linux_binprm * bprm);
 	void (*bprm_free_security) (struct linux_binprm * bprm);
 	void (*bprm_apply_creds) (struct linux_binprm * bprm, int unsafe);
+	void (*bprm_post_apply_creds) (struct linux_binprm * bprm);
 	int (*bprm_set_security) (struct linux_binprm * bprm);
 	int (*bprm_check_security) (struct linux_binprm * bprm);
 	int (*bprm_secureexec) (struct linux_binprm * bprm);
@@ -1320,6 +1328,10 @@ static inline void security_bprm_apply_c
 {
 	security_ops->bprm_apply_creds (bprm, unsafe);
 }
+static inline void security_bprm_post_apply_creds (struct linux_binprm *bprm)
+{
+	security_ops->bprm_post_apply_creds (bprm);
+}
 static inline int security_bprm_set (struct linux_binprm *bprm)
 {
 	return security_ops->bprm_set_security (bprm);
@@ -2003,6 +2015,11 @@ static inline void security_bprm_apply_c
 	cap_bprm_apply_creds (bprm, unsafe);
 }
 
+static inline void security_bprm_post_apply_creds (struct linux_binprm *bprm)
+{ 
+	return;
+}
+
 static inline int security_bprm_set (struct linux_binprm *bprm)
 {
 	return cap_bprm_set_security (bprm);
Index: linux-2.6.10-rc3-mm1/security/dummy.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/dummy.c	2004-12-15 15:57:23.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/dummy.c	2004-12-17 13:08:16.000000000 -0600
@@ -201,6 +201,11 @@ static void dummy_bprm_apply_creds (stru
 	current->sgid = current->egid = current->fsgid = bprm->e_gid;
 }
 
+static void dummy_bprm_post_apply_creds (struct linux_binprm *bprm)
+{
+	return;
+}
+
 static int dummy_bprm_set_security (struct linux_binprm *bprm)
 {
 	return 0;
@@ -921,6 +926,7 @@ void security_fixup_ops (struct security
 	set_to_dummy_if_null(ops, bprm_alloc_security);
 	set_to_dummy_if_null(ops, bprm_free_security);
 	set_to_dummy_if_null(ops, bprm_apply_creds);
+	set_to_dummy_if_null(ops, bprm_post_apply_creds);
 	set_to_dummy_if_null(ops, bprm_set_security);
 	set_to_dummy_if_null(ops, bprm_check_security);
 	set_to_dummy_if_null(ops, bprm_secureexec);
Index: linux-2.6.10-rc3-mm1/security/selinux/hooks.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/selinux/hooks.c	2004-12-15 17:25:55.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/selinux/hooks.c	2004-12-17 13:08:28.000000000 -0600
@@ -1810,10 +1810,7 @@ static void selinux_bprm_apply_creds(str
 	struct task_security_struct *tsec;
 	struct bprm_security_struct *bsec;
 	u32 sid;
-	struct av_decision avd;
-	struct itimerval itimer;
-	struct rlimit *rlim, *initrlim;
-	int rc, i;
+	int rc;
 
 	secondary_ops->bprm_apply_creds(bprm, unsafe);
 
@@ -1823,91 +1820,101 @@ static void selinux_bprm_apply_creds(str
 	sid = bsec->sid;
 
 	tsec->osid = tsec->sid;
+	bsec->unsafe = 0;
 	if (tsec->sid != sid) {
 		/* Check for shared state.  If not ok, leave SID
 		   unchanged and kill. */
 		if (unsafe & LSM_UNSAFE_SHARE) {
-			rc = avc_has_perm_noaudit(tsec->sid, sid,
-					  SECCLASS_PROCESS, PROCESS__SHARE, &avd);
+			rc = avc_has_perm(tsec->sid, sid, SECCLASS_PROCESS,
+					PROCESS__SHARE, NULL);
 			if (rc) {
-				task_unlock(current);
-				avc_audit(tsec->sid, sid, SECCLASS_PROCESS,
-				    PROCESS__SHARE, &avd, rc, NULL);
-				force_sig_specific(SIGKILL, current);
-				goto lock_out;
+				bsec->unsafe = 1;
+				return;
 			}
 		}
 
 		/* Check for ptracing, and update the task SID if ok.
 		   Otherwise, leave SID unchanged and kill. */
 		if (unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
-			rc = avc_has_perm_noaudit(tsec->ptrace_sid, sid,
-					  SECCLASS_PROCESS, PROCESS__PTRACE, &avd);
-			if (!rc)
-				tsec->sid = sid;
-			task_unlock(current);
-			avc_audit(tsec->ptrace_sid, sid, SECCLASS_PROCESS,
-				  PROCESS__PTRACE, &avd, rc, NULL);
+			rc = avc_has_perm(tsec->ptrace_sid, sid,
+					  SECCLASS_PROCESS, PROCESS__PTRACE,
+					  NULL);
 			if (rc) {
-				force_sig_specific(SIGKILL, current);
-				goto lock_out;
+				bsec->unsafe = 1;
+				return;
 			}
-		} else {
-			tsec->sid = sid;
-			task_unlock(current);
 		}
+		tsec->sid = sid;
+	}
+}
 
-		/* Close files for which the new task SID is not authorized. */
-		flush_unauthorized_files(current->files);
+/*
+ * called after apply_creds without the task lock held
+ */
+static void selinux_bprm_post_apply_creds(struct linux_binprm *bprm)
+{
+	struct task_security_struct *tsec;
+	struct rlimit *rlim, *initrlim;
+	struct itimerval itimer;
+	struct bprm_security_struct *bsec;
+	int rc, i;
 
-		/* Check whether the new SID can inherit signal state
-		   from the old SID.  If not, clear itimers to avoid
-		   subsequent signal generation and flush and unblock
-		   signals. This must occur _after_ the task SID has
-                  been updated so that any kill done after the flush
-                  will be checked against the new SID. */
-		rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
-				  PROCESS__SIGINH, NULL);
-		if (rc) {
-			memset(&itimer, 0, sizeof itimer);
-			for (i = 0; i < 3; i++)
-				do_setitimer(i, &itimer, NULL);
-			flush_signals(current);
-			spin_lock_irq(&current->sighand->siglock);
-			flush_signal_handlers(current, 1);
-			sigemptyset(&current->blocked);
-			recalc_sigpending();
-			spin_unlock_irq(&current->sighand->siglock);
-		}
+	tsec = current->security;
+	bsec = bprm->security;
 
-		/* Check whether the new SID can inherit resource limits
-		   from the old SID.  If not, reset all soft limits to
-		   the lower of the current task's hard limit and the init
-		   task's soft limit.  Note that the setting of hard limits 
-		   (even to lower them) can be controlled by the setrlimit 
-		   check. The inclusion of the init task's soft limit into
-	           the computation is to avoid resetting soft limits higher
-		   than the default soft limit for cases where the default
-		   is lower than the hard limit, e.g. RLIMIT_CORE or 
-		   RLIMIT_STACK.*/
-		rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
-				  PROCESS__RLIMITINH, NULL);
-		if (rc) {
-			for (i = 0; i < RLIM_NLIMITS; i++) {
-				rlim = current->signal->rlim + i;
-				initrlim = init_task.signal->rlim+i;
-				rlim->rlim_cur = min(rlim->rlim_max,initrlim->rlim_cur);
-			}
-		}
+	if (bsec->unsafe) {
+		force_sig_specific(SIGKILL, current);
+		return;
+	}
+	if (tsec->osid == tsec->sid)
+		return;
 
-		/* Wake up the parent if it is waiting so that it can
-		   recheck wait permission to the new task SID. */
-		wake_up_interruptible(&current->parent->signal->wait_chldexit);
+	/* Close files for which the new task SID is not authorized. */
+	flush_unauthorized_files(current->files);
 
-lock_out:
-		task_lock(current);
-		return;
+	/* Check whether the new SID can inherit signal state
+	   from the old SID.  If not, clear itimers to avoid
+	   subsequent signal generation and flush and unblock
+	   signals. This must occur _after_ the task SID has
+	  been updated so that any kill done after the flush
+	  will be checked against the new SID. */
+	rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
+			  PROCESS__SIGINH, NULL);
+	if (rc) {
+		memset(&itimer, 0, sizeof itimer);
+		for (i = 0; i < 3; i++)
+			do_setitimer(i, &itimer, NULL);
+		flush_signals(current);
+		spin_lock_irq(&current->sighand->siglock);
+		flush_signal_handlers(current, 1);
+		sigemptyset(&current->blocked);
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
+	}
+
+	/* Check whether the new SID can inherit resource limits
+	   from the old SID.  If not, reset all soft limits to
+	   the lower of the current task's hard limit and the init
+	   task's soft limit.  Note that the setting of hard limits 
+	   (even to lower them) can be controlled by the setrlimit 
+	   check. The inclusion of the init task's soft limit into
+	   the computation is to avoid resetting soft limits higher
+	   than the default soft limit for cases where the default
+	   is lower than the hard limit, e.g. RLIMIT_CORE or 
+	   RLIMIT_STACK.*/
+	rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
+			  PROCESS__RLIMITINH, NULL);
+	if (rc) {
+		for (i = 0; i < RLIM_NLIMITS; i++) {
+			rlim = current->signal->rlim + i;
+			initrlim = init_task.signal->rlim+i;
+			rlim->rlim_cur = min(rlim->rlim_max,initrlim->rlim_cur);
+		}
 	}
+
+	/* Wake up the parent if it is waiting so that it can
+	   recheck wait permission to the new task SID. */
+	wake_up_interruptible(&current->parent->signal->wait_chldexit);
 }
 
 /* superblock security operations */
@@ -4235,6 +4242,7 @@ struct security_operations selinux_ops =
 	.bprm_alloc_security =		selinux_bprm_alloc_security,
 	.bprm_free_security =		selinux_bprm_free_security,
 	.bprm_apply_creds =		selinux_bprm_apply_creds,
+	.bprm_post_apply_creds =	selinux_bprm_post_apply_creds,
 	.bprm_set_security =		selinux_bprm_set_security,
 	.bprm_check_security =		selinux_bprm_check_security,
 	.bprm_secureexec =		selinux_bprm_secureexec,
Index: linux-2.6.10-rc3-mm1/security/selinux/include/objsec.h
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/selinux/include/objsec.h	2004-12-15 15:57:23.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/selinux/include/objsec.h	2004-12-17 13:17:45.000000000 -0600
@@ -87,6 +87,12 @@ struct bprm_security_struct {
 	struct linux_binprm *bprm;     /* back pointer to bprm object */
 	u32 sid;                       /* SID for transformed process */
 	unsigned char set;
+
+	/*
+	 * unsafe is used to share failure information from bprm_apply_creds()
+	 * to bprm_post_apply_creds().
+	 */
+	char unsafe;
 };
 
 struct netif_security_struct {

Re: [PATCH] Split bprm_apply_creds into two functions

[Chris Wright]

* Serge E. Hallyn (serue@us.ibm.com) wrote:
> The security_bprm_apply_creds() function is called from
> fs/exec.c:compute_creds() under task_lock(current).  SELinux must
> perform some work which is unsafe in that context, and therefore
> explicitly drops the task_lock, does the work, and re-acquires the
> task_lock.  This is unsafe if other security modules are stacked after
> SELinux, as their bprm_apply_creds assumes that the 'unsafe' variable is
> still meaningful, that is, that the task_lock has not been dropped.

I don't like this approach.  The whole point is to ensure safety, and
avoid races that have been found in the past.  This gives a new interface
that could be easily used under the wrong conditions, and breaking
the interface into two pieces looks kinda hackish.  Is there no other
solution?  I looked at this once before and wondered why task_unlock()
is needed to call avc_audit?  audit should be as lock friendly as printk
IMO, and I don't recall seeing any deadlock after short review of it.
But I didn't get much beyond that.  Is it all the flushing that can't
hold task_lock?

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] Split bprm_apply_creds into two functions

[Stephen Smalley]

On Wed, 2004-12-15 at 17:52, Chris Wright wrote:
> I don't like this approach.  The whole point is to ensure safety, and
> avoid races that have been found in the past.  This gives a new interface
> that could be easily used under the wrong conditions, and breaking
> the interface into two pieces looks kinda hackish.  Is there no other
> solution?  I looked at this once before and wondered why task_unlock()
> is needed to call avc_audit?  audit should be as lock friendly as printk
> IMO, and I don't recall seeing any deadlock after short review of it.
> But I didn't get much beyond that.  Is it all the flushing that can't
> hold task_lock?

avc_audit() used to always call get_task_mm() when fetching the mm for
use in determining the executable, which was producing deadlock when the
caller held task lock.  However, I changed it to only use get_task_mm
when acting on a task other than current, since we can safely access
current->mm, which eliminated the deadlock for the checks in
bprm_apply_creds.  That is why Serge's patch folds
avc_has_perm_noaudit()+avc_audit() pairs down to avc_has_perm() and
keeps them in bprm_apply_creds.  Hence, avc_audit is no longer a
concern.

The concern is with lock nesting for the flushing code, e.g. call to
force_sig_specific and signal inheritance flush would nest siglock under
task lock and call to flush_unauthorized_files would nest file_list_lock
and file_lock under task lock.  That code didn't used to be called under
task lock prior to the reworking of compute_creds by Andy Lutomirski,
and when compute_creds was overhauled, there was some concern expressed
by Andrew Morton about the lock nesting, iirc.

Note that the flushing code isn't relying on the safety flags computed
earlier by unsafe_exec, so it really doesn't need the task lock for that
purpose.

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH] Split bprm_apply_creds into two functions

[Serge E. Hallyn]

Quoting Chris Wright (chrisw@osdl.org):
> * Serge E. Hallyn (serue@us.ibm.com) wrote:
> > The security_bprm_apply_creds() function is called from
> > fs/exec.c:compute_creds() under task_lock(current).  SELinux must
> > perform some work which is unsafe in that context, and therefore
> > explicitly drops the task_lock, does the work, and re-acquires the
> > task_lock.  This is unsafe if other security modules are stacked after
> > SELinux, as their bprm_apply_creds assumes that the 'unsafe' variable is
> > still meaningful, that is, that the task_lock has not been dropped.
> 
> I don't like this approach.  The whole point is to ensure safety, and
> avoid races that have been found in the past.  This gives a new interface
> that could be easily used under the wrong conditions, and breaking
> the interface into two pieces looks kinda hackish.  Is there no other
> solution?  I looked at this once before and wondered why task_unlock()
> is needed to call avc_audit?  audit should be as lock friendly as printk
> IMO, and I don't recall seeing any deadlock after short review of it.
> But I didn't get much beyond that.  Is it all the flushing that can't
> hold task_lock?

As Stephen points out, it was more a concern about lock nesting.  The
attached patch simply removes the task_unlock from selinux_bprm_apply_creds,
and runs just fine on my machine.  Stephen, do you have a preference
either way, or was the task_unlock to relieve the concerns of others?

thanks,
-serge

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Index: linux-2.6.9-test/security/selinux/hooks.c
===================================================================
--- linux-2.6.9-test.orig/security/selinux/hooks.c	2004-12-16 12:14:32.000000000 -0600
+++ linux-2.6.9-test/security/selinux/hooks.c	2004-12-16 12:51:55.000000000 -0600
@@ -1827,35 +1827,26 @@ static void selinux_bprm_apply_creds(str
 		/* Check for shared state.  If not ok, leave SID
 		   unchanged and kill. */
 		if (unsafe & LSM_UNSAFE_SHARE) {
-			rc = avc_has_perm_noaudit(tsec->sid, sid,
-					  SECCLASS_PROCESS, PROCESS__SHARE, &avd);
+			rc = avc_has_perm(tsec->sid, sid, SECCLASS_PROCESS,
+						PROCESS__SHARE, NULL);
 			if (rc) {
-				task_unlock(current);
-				avc_audit(tsec->sid, sid, SECCLASS_PROCESS,
-				    PROCESS__SHARE, &avd, rc, NULL);
 				force_sig_specific(SIGKILL, current);
-				goto lock_out;
+				return;
 			}
 		}
 
 		/* Check for ptracing, and update the task SID if ok.
 		   Otherwise, leave SID unchanged and kill. */
 		if (unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
-			rc = avc_has_perm_noaudit(tsec->ptrace_sid, sid,
-					  SECCLASS_PROCESS, PROCESS__PTRACE, &avd);
-			if (!rc)
-				tsec->sid = sid;
-			task_unlock(current);
-			avc_audit(tsec->ptrace_sid, sid, SECCLASS_PROCESS,
-				  PROCESS__PTRACE, &avd, rc, NULL);
+			rc = avc_has_perm(tsec->ptrace_sid, sid,
+					  SECCLASS_PROCESS, PROCESS__PTRACE,
+					  NULL);
 			if (rc) {
 				force_sig_specific(SIGKILL, current);
-				goto lock_out;
+				return;
 			}
-		} else {
-			tsec->sid = sid;
-			task_unlock(current);
 		}
+		tsec->sid = sid;
 
 		/* Close files for which the new task SID is not authorized. */
 		flush_unauthorized_files(current->files);
@@ -1903,10 +1894,6 @@ static void selinux_bprm_apply_creds(str
 		/* Wake up the parent if it is waiting so that it can
 		   recheck wait permission to the new task SID. */
 		wake_up_interruptible(&current->parent->signal->wait_chldexit);
-
-lock_out:
-		task_lock(current);
-		return;
 	}
 }
 

Re: [PATCH] Split bprm_apply_creds into two functions

[Stephen Smalley]

On Thu, 2004-12-16 at 13:16, Serge E. Hallyn wrote:
> As Stephen points out, it was more a concern about lock nesting.  The
> attached patch simply removes the task_unlock from selinux_bprm_apply_creds,
> and runs just fine on my machine.  Stephen, do you have a preference
> either way, or was the task_unlock to relieve the concerns of others?

I prefer your original patch (with the two minor changes I suggested),
to avoid unnecessary lock nesting.

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH] Split bprm_apply_creds into two functions

[Chris Wright]

* Serge E. Hallyn (serue@us.ibm.com) wrote:
> Quoting Chris Wright (chrisw@osdl.org):
> > * Serge E. Hallyn (serue@us.ibm.com) wrote:
> > > The security_bprm_apply_creds() function is called from
> > > fs/exec.c:compute_creds() under task_lock(current).  SELinux must
> > > perform some work which is unsafe in that context, and therefore
> > > explicitly drops the task_lock, does the work, and re-acquires the
> > > task_lock.  This is unsafe if other security modules are stacked after
> > > SELinux, as their bprm_apply_creds assumes that the 'unsafe' variable is
> > > still meaningful, that is, that the task_lock has not been dropped.
> > 
> > I don't like this approach.  The whole point is to ensure safety, and
> > avoid races that have been found in the past.  This gives a new interface
> > that could be easily used under the wrong conditions, and breaking
> > the interface into two pieces looks kinda hackish.  Is there no other
> > solution?  I looked at this once before and wondered why task_unlock()
> > is needed to call avc_audit?  audit should be as lock friendly as printk
> > IMO, and I don't recall seeing any deadlock after short review of it.
> > But I didn't get much beyond that.  Is it all the flushing that can't
> > hold task_lock?
> 
> As Stephen points out, it was more a concern about lock nesting.  The
> attached patch simply removes the task_unlock from selinux_bprm_apply_creds,
> and runs just fine on my machine.  Stephen, do you have a preference
> either way, or was the task_unlock to relieve the concerns of others?

Unfortunately, running fine isn't indication that it's not
deadlock-able.  Issues are making sure nesting is appropriate, as
task_lock is normally inner lock, and making sure normal things like not
taking sem when holding spinlock.  It requires more inspection than just
running.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

[PATCH] split bprm_apply_creds into two functions

[Serge E. Hallyn]

The following patch splits bprm_apply_creds into two functions,
bprm_apply_creds and bprm_post_apply_creds.  The latter is called
after the task_lock has been dropped.  Without this patch, SELinux
must drop the task_lock and re-acquire it during apply_creds, making
the 'unsafe' flag meaningless to any later security modules.  Please
apply.

thanks,
-serge

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Index: linux-2.6.10-mm1/fs/exec.c
===================================================================
--- linux-2.6.10-mm1.orig/fs/exec.c	2005-01-04 12:15:45.000000000 -0600
+++ linux-2.6.10-mm1/fs/exec.c	2005-01-04 12:42:43.000000000 -0600
@@ -963,6 +963,7 @@ void compute_creds(struct linux_binprm *
 	unsafe = unsafe_exec(current);
 	security_bprm_apply_creds(bprm, unsafe);
 	task_unlock(current);
+	security_bprm_post_apply_creds(bprm);
 }
 
 EXPORT_SYMBOL(compute_creds);
Index: linux-2.6.10-mm1/include/linux/security.h
===================================================================
--- linux-2.6.10-mm1.orig/include/linux/security.h	2005-01-04 12:15:45.000000000 -0600
+++ linux-2.6.10-mm1/include/linux/security.h	2005-01-04 13:02:15.000000000 -0600
@@ -109,13 +109,20 @@ struct swap_info_struct;
  *	and the information saved in @bprm->security by the set_security hook.
  *	Since this hook function (and its caller) are void, this hook can not
  *	return an error.  However, it can leave the security attributes of the
- *	process unchanged if an access failure occurs at this point. It can
- *	also perform other state changes on the process (e.g.  closing open
- *	file descriptors to which access is no longer granted if the attributes
- *	were changed). 
+ *	process unchanged if an access failure occurs at this point.
  *	bprm_apply_creds is called under task_lock.  @unsafe indicates various
  *	reasons why it may be unsafe to change security state.
  *	@bprm contains the linux_binprm structure.
+ * @bprm_post_apply_creds:
+ *	Runs after bprm_apply_creds with the task_lock dropped, so that
+ *	functions which cannot be called safely under the task_lock can
+ *	be used.  This hook is a good place to perform state changes on
+ *	the process such as closing open file descriptors to which access
+ *	is no longer granted if the attributes were changed. 
+ *	Note that a security module might need to save state between
+ *	bprm_apply_creds and bprm_post_apply_creds to store the decision
+ *	on whether the process may proceed.
+ *	@bprm contains the linux_binprm structure.
  * @bprm_set_security:
  *	Save security information in the bprm->security field, typically based
  *	on information about the bprm->file, for later use by the apply_creds
@@ -1042,6 +1049,7 @@ struct security_operations {
 	int (*bprm_alloc_security) (struct linux_binprm * bprm);
 	void (*bprm_free_security) (struct linux_binprm * bprm);
 	void (*bprm_apply_creds) (struct linux_binprm * bprm, int unsafe);
+	void (*bprm_post_apply_creds) (struct linux_binprm * bprm);
 	int (*bprm_set_security) (struct linux_binprm * bprm);
 	int (*bprm_check_security) (struct linux_binprm * bprm);
 	int (*bprm_secureexec) (struct linux_binprm * bprm);
@@ -1314,6 +1322,10 @@ static inline void security_bprm_apply_c
 {
 	security_ops->bprm_apply_creds (bprm, unsafe);
 }
+static inline void security_bprm_post_apply_creds (struct linux_binprm *bprm)
+{
+	security_ops->bprm_post_apply_creds (bprm);
+}
 static inline int security_bprm_set (struct linux_binprm *bprm)
 {
 	return security_ops->bprm_set_security (bprm);
@@ -1992,6 +2004,11 @@ static inline void security_bprm_apply_c
 	cap_bprm_apply_creds (bprm, unsafe);
 }
 
+static inline void security_bprm_post_apply_creds (struct linux_binprm *bprm)
+{ 
+	return;
+}
+
 static inline int security_bprm_set (struct linux_binprm *bprm)
 {
 	return cap_bprm_set_security (bprm);
Index: linux-2.6.10-mm1/security/dummy.c
===================================================================
--- linux-2.6.10-mm1.orig/security/dummy.c	2005-01-04 12:15:45.000000000 -0600
+++ linux-2.6.10-mm1/security/dummy.c	2005-01-04 12:42:43.000000000 -0600
@@ -201,6 +201,11 @@ static void dummy_bprm_apply_creds (stru
 	current->sgid = current->egid = current->fsgid = bprm->e_gid;
 }
 
+static void dummy_bprm_post_apply_creds (struct linux_binprm *bprm)
+{
+	return;
+}
+
 static int dummy_bprm_set_security (struct linux_binprm *bprm)
 {
 	return 0;
@@ -916,6 +921,7 @@ void security_fixup_ops (struct security
 	set_to_dummy_if_null(ops, bprm_alloc_security);
 	set_to_dummy_if_null(ops, bprm_free_security);
 	set_to_dummy_if_null(ops, bprm_apply_creds);
+	set_to_dummy_if_null(ops, bprm_post_apply_creds);
 	set_to_dummy_if_null(ops, bprm_set_security);
 	set_to_dummy_if_null(ops, bprm_check_security);
 	set_to_dummy_if_null(ops, bprm_secureexec);
Index: linux-2.6.10-mm1/security/selinux/hooks.c
===================================================================
--- linux-2.6.10-mm1.orig/security/selinux/hooks.c	2005-01-04 12:15:45.000000000 -0600
+++ linux-2.6.10-mm1/security/selinux/hooks.c	2005-01-04 12:42:43.000000000 -0600
@@ -1801,10 +1801,7 @@ static void selinux_bprm_apply_creds(str
 	struct task_security_struct *tsec;
 	struct bprm_security_struct *bsec;
 	u32 sid;
-	struct av_decision avd;
-	struct itimerval itimer;
-	struct rlimit *rlim, *initrlim;
-	int rc, i;
+	int rc;
 
 	secondary_ops->bprm_apply_creds(bprm, unsafe);
 
@@ -1814,91 +1811,101 @@ static void selinux_bprm_apply_creds(str
 	sid = bsec->sid;
 
 	tsec->osid = tsec->sid;
+	bsec->unsafe = 0;
 	if (tsec->sid != sid) {
 		/* Check for shared state.  If not ok, leave SID
 		   unchanged and kill. */
 		if (unsafe & LSM_UNSAFE_SHARE) {
-			rc = avc_has_perm_noaudit(tsec->sid, sid,
-					  SECCLASS_PROCESS, PROCESS__SHARE, &avd);
+			rc = avc_has_perm(tsec->sid, sid, SECCLASS_PROCESS,
+					PROCESS__SHARE, NULL);
 			if (rc) {
-				task_unlock(current);
-				avc_audit(tsec->sid, sid, SECCLASS_PROCESS,
-				    PROCESS__SHARE, &avd, rc, NULL);
-				force_sig_specific(SIGKILL, current);
-				goto lock_out;
+				bsec->unsafe = 1;
+				return;
 			}
 		}
 
 		/* Check for ptracing, and update the task SID if ok.
 		   Otherwise, leave SID unchanged and kill. */
 		if (unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
-			rc = avc_has_perm_noaudit(tsec->ptrace_sid, sid,
-					  SECCLASS_PROCESS, PROCESS__PTRACE, &avd);
-			if (!rc)
-				tsec->sid = sid;
-			task_unlock(current);
-			avc_audit(tsec->ptrace_sid, sid, SECCLASS_PROCESS,
-				  PROCESS__PTRACE, &avd, rc, NULL);
+			rc = avc_has_perm(tsec->ptrace_sid, sid,
+					  SECCLASS_PROCESS, PROCESS__PTRACE,
+					  NULL);
 			if (rc) {
-				force_sig_specific(SIGKILL, current);
-				goto lock_out;
+				bsec->unsafe = 1;
+				return;
 			}
-		} else {
-			tsec->sid = sid;
-			task_unlock(current);
 		}
+		tsec->sid = sid;
+	}
+}
 
-		/* Close files for which the new task SID is not authorized. */
-		flush_unauthorized_files(current->files);
+/*
+ * called after apply_creds without the task lock held
+ */
+static void selinux_bprm_post_apply_creds(struct linux_binprm *bprm)
+{
+	struct task_security_struct *tsec;
+	struct rlimit *rlim, *initrlim;
+	struct itimerval itimer;
+	struct bprm_security_struct *bsec;
+	int rc, i;
 
-		/* Check whether the new SID can inherit signal state
-		   from the old SID.  If not, clear itimers to avoid
-		   subsequent signal generation and flush and unblock
-		   signals. This must occur _after_ the task SID has
-                  been updated so that any kill done after the flush
-                  will be checked against the new SID. */
-		rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
-				  PROCESS__SIGINH, NULL);
-		if (rc) {
-			memset(&itimer, 0, sizeof itimer);
-			for (i = 0; i < 3; i++)
-				do_setitimer(i, &itimer, NULL);
-			flush_signals(current);
-			spin_lock_irq(&current->sighand->siglock);
-			flush_signal_handlers(current, 1);
-			sigemptyset(&current->blocked);
-			recalc_sigpending();
-			spin_unlock_irq(&current->sighand->siglock);
-		}
+	tsec = current->security;
+	bsec = bprm->security;
 
-		/* Check whether the new SID can inherit resource limits
-		   from the old SID.  If not, reset all soft limits to
-		   the lower of the current task's hard limit and the init
-		   task's soft limit.  Note that the setting of hard limits 
-		   (even to lower them) can be controlled by the setrlimit 
-		   check. The inclusion of the init task's soft limit into
-	           the computation is to avoid resetting soft limits higher
-		   than the default soft limit for cases where the default
-		   is lower than the hard limit, e.g. RLIMIT_CORE or 
-		   RLIMIT_STACK.*/
-		rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
-				  PROCESS__RLIMITINH, NULL);
-		if (rc) {
-			for (i = 0; i < RLIM_NLIMITS; i++) {
-				rlim = current->signal->rlim + i;
-				initrlim = init_task.signal->rlim+i;
-				rlim->rlim_cur = min(rlim->rlim_max,initrlim->rlim_cur);
-			}
-		}
+	if (bsec->unsafe) {
+		force_sig_specific(SIGKILL, current);
+		return;
+	}
+	if (tsec->osid == tsec->sid)
+		return;
 
-		/* Wake up the parent if it is waiting so that it can
-		   recheck wait permission to the new task SID. */
-		wake_up_interruptible(&current->parent->signal->wait_chldexit);
+	/* Close files for which the new task SID is not authorized. */
+	flush_unauthorized_files(current->files);
 
-lock_out:
-		task_lock(current);
-		return;
+	/* Check whether the new SID can inherit signal state
+	   from the old SID.  If not, clear itimers to avoid
+	   subsequent signal generation and flush and unblock
+	   signals. This must occur _after_ the task SID has
+	  been updated so that any kill done after the flush
+	  will be checked against the new SID. */
+	rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
+			  PROCESS__SIGINH, NULL);
+	if (rc) {
+		memset(&itimer, 0, sizeof itimer);
+		for (i = 0; i < 3; i++)
+			do_setitimer(i, &itimer, NULL);
+		flush_signals(current);
+		spin_lock_irq(&current->sighand->siglock);
+		flush_signal_handlers(current, 1);
+		sigemptyset(&current->blocked);
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
+	}
+
+	/* Check whether the new SID can inherit resource limits
+	   from the old SID.  If not, reset all soft limits to
+	   the lower of the current task's hard limit and the init
+	   task's soft limit.  Note that the setting of hard limits 
+	   (even to lower them) can be controlled by the setrlimit 
+	   check. The inclusion of the init task's soft limit into
+	   the computation is to avoid resetting soft limits higher
+	   than the default soft limit for cases where the default
+	   is lower than the hard limit, e.g. RLIMIT_CORE or 
+	   RLIMIT_STACK.*/
+	rc = avc_has_perm(tsec->osid, tsec->sid, SECCLASS_PROCESS,
+			  PROCESS__RLIMITINH, NULL);
+	if (rc) {
+		for (i = 0; i < RLIM_NLIMITS; i++) {
+			rlim = current->signal->rlim + i;
+			initrlim = init_task.signal->rlim+i;
+			rlim->rlim_cur = min(rlim->rlim_max,initrlim->rlim_cur);
+		}
 	}
+
+	/* Wake up the parent if it is waiting so that it can
+	   recheck wait permission to the new task SID. */
+	wake_up_interruptible(&current->parent->signal->wait_chldexit);
 }
 
 /* superblock security operations */
@@ -4218,6 +4225,7 @@ struct security_operations selinux_ops =
 	.bprm_alloc_security =		selinux_bprm_alloc_security,
 	.bprm_free_security =		selinux_bprm_free_security,
 	.bprm_apply_creds =		selinux_bprm_apply_creds,
+	.bprm_post_apply_creds =	selinux_bprm_post_apply_creds,
 	.bprm_set_security =		selinux_bprm_set_security,
 	.bprm_check_security =		selinux_bprm_check_security,
 	.bprm_secureexec =		selinux_bprm_secureexec,
Index: linux-2.6.10-mm1/security/selinux/include/objsec.h
===================================================================
--- linux-2.6.10-mm1.orig/security/selinux/include/objsec.h	2005-01-04 12:15:45.000000000 -0600
+++ linux-2.6.10-mm1/security/selinux/include/objsec.h	2005-01-04 12:42:43.000000000 -0600
@@ -87,6 +87,12 @@ struct bprm_security_struct {
 	struct linux_binprm *bprm;     /* back pointer to bprm object */
 	u32 sid;                       /* SID for transformed process */
 	unsigned char set;
+
+	/*
+	 * unsafe is used to share failure information from bprm_apply_creds()
+	 * to bprm_post_apply_creds().
+	 */
+	char unsafe;
 };
 
 struct netif_security_struct {

Re: [PATCH] split bprm_apply_creds into two functions

[Stephen Smalley]

On Tue, 2005-01-04 at 13:30, Serge E. Hallyn wrote:
> The following patch splits bprm_apply_creds into two functions,
> bprm_apply_creds and bprm_post_apply_creds.  The latter is called
> after the task_lock has been dropped.  Without this patch, SELinux
> must drop the task_lock and re-acquire it during apply_creds, making
> the 'unsafe' flag meaningless to any later security modules.  Please
> apply.
> 
> thanks,
> -serge
> 
> Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Signed-off-by:  Stephen Smalley <sds@epoch.ncsc.mil>

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH] split bprm_apply_creds into two functions

[Chris Wright]

* Serge E. Hallyn (serue@us.ibm.com) wrote:
> The following patch splits bprm_apply_creds into two functions,
> bprm_apply_creds and bprm_post_apply_creds.  The latter is called
> after the task_lock has been dropped.  Without this patch, SELinux
> must drop the task_lock and re-acquire it during apply_creds, making
> the 'unsafe' flag meaningless to any later security modules.  Please
> apply.
> 
> Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Signed-off-by: Chris Wright <chrisw@osdl.org>

Re: [PATCH] split bprm_apply_creds into two functions

[Chris Wright]

* Serge E. Hallyn (serue@us.ibm.com) wrote:
> The following patch splits bprm_apply_creds into two functions,
> bprm_apply_creds and bprm_post_apply_creds.  The latter is called
> after the task_lock has been dropped.  Without this patch, SELinux
> must drop the task_lock and re-acquire it during apply_creds, making
> the 'unsafe' flag meaningless to any later security modules.  Please
> apply.
> 
> thanks,
> -serge
> 
> Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Signed-off-by: Chris Wright <chrisw@osdl.org>

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] split bprm_apply_creds into two functions

[Chris Wright]

* Chris Wright (chrisw@osdl.org) wrote:
> * Serge E. Hallyn (serue@us.ibm.com) wrote:
> > The following patch splits bprm_apply_creds into two functions,
> > bprm_apply_creds and bprm_post_apply_creds.  The latter is called
> > after the task_lock has been dropped.  Without this patch, SELinux
> > must drop the task_lock and re-acquire it during apply_creds, making
> > the 'unsafe' flag meaningless to any later security modules.  Please
> > apply.
> > 
> > thanks,
> > -serge
> > 
> > Signed-off-by: Serge Hallyn <serue@us.ibm.com>
> 
> Signed-off-by: Chris Wright <chrisw@osdl.org>

Err, replied to the wrong email.  Meant to reply to the 'resend'

ugh,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net