Re: [PATCH] Split bprm_apply_creds into two functions

[Serge E. Hallyn]

Quoting Chris Wright (chrisw@xxxxxxxx):
> * Serge E. Hallyn (serue@xxxxxxxxxx) 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@xxxxxxxxxx>

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;
 	}
 }
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/