[PATCH 1/2] CRED: Rename cred_exec_mutex to reflect that it's a guard against ptrace

[PATCH 1/2] CRED: Rename cred_exec_mutex to reflect that it's a guard against ptrace

[David Howells]

Rename cred_exec_mutex to reflect that it's a guard against foreign
intervention on a process's credential state, such as is made by ptrace().  The
attachment of a debugger to a process affects execve()'s calculation of the new
credential state - _and_ also setprocattr()'s calculation of that state.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/compat.c               |    6 +++---
 fs/exec.c                 |   10 +++++-----
 include/linux/init_task.h |    4 ++--
 include/linux/sched.h     |    4 +++-
 kernel/cred.c             |    4 ++--
 kernel/ptrace.c           |    9 +++++----
 6 files changed, 20 insertions(+), 17 deletions(-)


diff --git a/fs/compat.c b/fs/compat.c
index 681ed81..bb2a9b2 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1488,7 +1488,7 @@ int compat_do_execve(char * filename,
 	if (!bprm)
 		goto out_files;
 
-	retval = mutex_lock_interruptible(&current->cred_exec_mutex);
+	retval = mutex_lock_interruptible(&current->cred_guard_mutex);
 	if (retval < 0)
 		goto out_free;
 	current->in_execve = 1;
@@ -1550,7 +1550,7 @@ int compat_do_execve(char * filename,
 	/* execve succeeded */
 	current->fs->in_exec = 0;
 	current->in_execve = 0;
-	mutex_unlock(&current->cred_exec_mutex);
+	mutex_unlock(&current->cred_guard_mutex);
 	acct_update_integrals(current);
 	free_bprm(bprm);
 	if (displaced)
@@ -1573,7 +1573,7 @@ out_unmark:
 
 out_unlock:
 	current->in_execve = 0;
-	mutex_unlock(&current->cred_exec_mutex);
+	mutex_unlock(&current->cred_guard_mutex);
 
 out_free:
 	free_bprm(bprm);
diff --git a/fs/exec.c b/fs/exec.c
index 639177b..998e856 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1045,7 +1045,7 @@ void install_exec_creds(struct linux_binprm *bprm)
 	commit_creds(bprm->cred);
 	bprm->cred = NULL;
 
-	/* cred_exec_mutex must be held at least to this point to prevent
+	/* cred_guard_mutex must be held at least to this point to prevent
 	 * ptrace_attach() from altering our determination of the task's
 	 * credentials; any time after this it may be unlocked */
 
@@ -1055,7 +1055,7 @@ EXPORT_SYMBOL(install_exec_creds);
 
 /*
  * determine how safe it is to execute the proposed program
- * - the caller must hold current->cred_exec_mutex to protect against
+ * - the caller must hold current->cred_guard_mutex to protect against
  *   PTRACE_ATTACH
  */
 int check_unsafe_exec(struct linux_binprm *bprm)
@@ -1297,7 +1297,7 @@ int do_execve(char * filename,
 	if (!bprm)
 		goto out_files;
 
-	retval = mutex_lock_interruptible(&current->cred_exec_mutex);
+	retval = mutex_lock_interruptible(&current->cred_guard_mutex);
 	if (retval < 0)
 		goto out_free;
 	current->in_execve = 1;
@@ -1360,7 +1360,7 @@ int do_execve(char * filename,
 	/* execve succeeded */
 	current->fs->in_exec = 0;
 	current->in_execve = 0;
-	mutex_unlock(&current->cred_exec_mutex);
+	mutex_unlock(&current->cred_guard_mutex);
 	acct_update_integrals(current);
 	free_bprm(bprm);
 	if (displaced)
@@ -1383,7 +1383,7 @@ out_unmark:
 
 out_unlock:
 	current->in_execve = 0;
-	mutex_unlock(&current->cred_exec_mutex);
+	mutex_unlock(&current->cred_guard_mutex);
 
 out_free:
 	free_bprm(bprm);
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index d87247d..7f54ba9 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -145,8 +145,8 @@ extern struct cred init_cred;
 	.group_leader	= &tsk,						\
 	.real_cred	= &init_cred,					\
 	.cred		= &init_cred,					\
-	.cred_exec_mutex =						\
-		 __MUTEX_INITIALIZER(tsk.cred_exec_mutex),		\
+	.cred_guard_mutex =						\
+		 __MUTEX_INITIALIZER(tsk.cred_guard_mutex),		\
 	.comm		= "swapper",					\
 	.thread		= INIT_THREAD,					\
 	.fs		= &init_fs,					\
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 3fa82b3..5932ace 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1247,7 +1247,9 @@ struct task_struct {
 					 * credentials (COW) */
 	const struct cred *cred;	/* effective (overridable) subjective task
 					 * credentials (COW) */
-	struct mutex cred_exec_mutex;	/* execve vs ptrace cred calculation mutex */
+	struct mutex cred_guard_mutex;	/* guard against foreign influences on
+					 * credential calculations
+					 * (notably. ptrace) */
 
 	char comm[TASK_COMM_LEN]; /* executable name excluding path
 				     - access with [gs]et_task_comm (which lock
diff --git a/kernel/cred.c b/kernel/cred.c
index 3a03918..1bb4d7e 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -167,7 +167,7 @@ EXPORT_SYMBOL(prepare_creds);
 
 /*
  * Prepare credentials for current to perform an execve()
- * - The caller must hold current->cred_exec_mutex
+ * - The caller must hold current->cred_guard_mutex
  */
 struct cred *prepare_exec_creds(void)
 {
@@ -276,7 +276,7 @@ int copy_creds(struct task_struct *p, unsigned long clone_flags)
 	struct cred *new;
 	int ret;
 
-	mutex_init(&p->cred_exec_mutex);
+	mutex_init(&p->cred_guard_mutex);
 
 	if (
 #ifdef CONFIG_KEYS
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 0692ab5..27ac802 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -185,10 +185,11 @@ int ptrace_attach(struct task_struct *task)
 	if (same_thread_group(task, current))
 		goto out;
 
-	/* Protect exec's credential calculations against our interference;
-	 * SUID, SGID and LSM creds get determined differently under ptrace.
+	/* Protect the target's credential calculations against our
+	 * interference; SUID, SGID and LSM creds get determined differently
+	 * under ptrace.
 	 */
-	retval = mutex_lock_interruptible(&task->cred_exec_mutex);
+	retval = mutex_lock_interruptible(&task->cred_guard_mutex);
 	if (retval  < 0)
 		goto out;
 
@@ -232,7 +233,7 @@ repeat:
 bad:
 	write_unlock_irqrestore(&tasklist_lock, flags);
 	task_unlock(task);
-	mutex_unlock(&task->cred_exec_mutex);
+	mutex_unlock(&task->cred_guard_mutex);
 out:
 	return retval;
 }

[PATCH 2/2] CRED: Guard the setprocattr security hook against ptrace

[David Howells]

Guard the setprocattr security hook against ptrace by taking the target task's
cred_guard_mutex around it.  The problem is that setprocattr() may otherwise
note the lack of a debugger, and then perform an action on that basis whilst
letting a debugger attach between the two points.  Holding cred_guard_mutex
across the test and the action prevents ptrace_attach() from doing that.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/proc/base.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)


diff --git a/fs/proc/base.c b/fs/proc/base.c
index fb45615..23342e1 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2128,9 +2128,15 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 	if (copy_from_user(page, buf, count))
 		goto out_free;
 
+	/* Guard against adverse ptrace interaction */
+	length = mutex_lock_interruptible(&task->cred_guard_mutex);
+	if (length < 0)
+		goto out_free;
+
 	length = security_setprocattr(task,
 				      (char*)file->f_path.dentry->d_name.name,
 				      (void*)page, count);
+	mutex_unlock(&task->cred_guard_mutex);
 out_free:
 	free_page((unsigned long) page);
 out:

Re: [PATCH 2/2] CRED: Guard the setprocattr security hook against ptrace

[James Morris]

On Fri, 8 May 2009, David Howells wrote:

> Guard the setprocattr security hook against ptrace by taking the target task's
> cred_guard_mutex around it.  The problem is that setprocattr() may otherwise
> note the lack of a debugger, and then perform an action on that basis whilst
> letting a debugger attach between the two points.  Holding cred_guard_mutex
> across the test and the action prevents ptrace_attach() from doing that.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

Applied to 
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/2] CRED: Rename cred_exec_mutex to reflect that it's a guard against ptrace

[Oleg Nesterov]

Sorry for delay,

On 05/08, David Howells wrote:
>
> @@ -185,10 +185,11 @@ int ptrace_attach(struct task_struct *task)
>  	if (same_thread_group(task, current))
>  		goto out;
>
> -	/* Protect exec's credential calculations against our interference;
> -	 * SUID, SGID and LSM creds get determined differently under ptrace.
> +	/* Protect the target's credential calculations against our
> +	 * interference; SUID, SGID and LSM creds get determined differently
> +	 * under ptrace.
>  	 */
> -	retval = mutex_lock_interruptible(&task->cred_exec_mutex);
> +	retval = mutex_lock_interruptible(&task->cred_guard_mutex);
>  	if (retval  < 0)
>  		goto out;
>
> @@ -232,7 +233,7 @@ repeat:
>  bad:
>  	write_unlock_irqrestore(&tasklist_lock, flags);
>  	task_unlock(task);
> -	mutex_unlock(&task->cred_exec_mutex);
> +	mutex_unlock(&task->cred_guard_mutex);

This rename is obviously fine, but conflicts with
ptrace-do-not-use-task_lock-for-attach.patch in -mm tree:

-bad:
-       write_unlock_irqrestore(&tasklist_lock, flags);
-       task_unlock(task);
+unlock_tasklist:
+       write_unlock_irq(&tasklist_lock);
+unlock_creds:
        mutex_unlock(&task->cred_exec_mutex);
 out:
        return retval;


Hmm. Ingo's "rename ptrace_may_access => ptrace_access_check" conflicts
with my patch too.


Andrew, Roland, I guess I should re-send
	
	ptrace-ptrace_attach-check-pf_kthread-exit_state-instead-of-mm.patch
	ptrace-cleanup-check-set-of-pt_ptraced-during-attach.patch
	ptrace-do-not-use-task_lock-for-attach.patch

patches?

Oleg.

Re: [PATCH 1/2] CRED: Rename cred_exec_mutex to reflect that it's a guard against ptrace

[Roland McGrath]

> Hmm. Ingo's "rename ptrace_may_access => ptrace_access_check" conflicts
> with my patch too.

Andrew seemed to want a different name choice too, so that will have to be
resolved before we worry about patch conflicts.

> Andrew, Roland, I guess I should re-send
> 	
> 	ptrace-ptrace_attach-check-pf_kthread-exit_state-instead-of-mm.patch
> 	ptrace-cleanup-check-set-of-pt_ptraced-during-attach.patch
> 	ptrace-do-not-use-task_lock-for-attach.patch
> 
> patches?

I guess so too.  Your series of changes is more substantial and potentially
controversial or problematic than the various renamings, so I think it
makes sense to settle and merge the renamings first.


Thanks,
Roland

Re: [PATCH 1/2] CRED: Rename cred_exec_mutex to reflect that it's a guard against ptrace

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> wrote:

> > Hmm. Ingo's "rename ptrace_may_access => ptrace_access_check" conflicts
> > with my patch too.
> 
> Andrew seemed to want a different name choice too, so that will have to be
> resolved before we worry about patch conflicts.
> 
> > Andrew, Roland, I guess I should re-send
> > 	
> > 	ptrace-ptrace_attach-check-pf_kthread-exit_state-instead-of-mm.patch
> > 	ptrace-cleanup-check-set-of-pt_ptraced-during-attach.patch
> > 	ptrace-do-not-use-task_lock-for-attach.patch
> > 
> > patches?
> 
> I guess so too.  Your series of changes is more substantial and 
> potentially controversial or problematic than the various 
> renamings, so I think it makes sense to settle and merge the 
> renamings first.

yeah, cleanups first is generally the better strategy. Not only does 
it make reverts easier, it also makes it easier to review (and 
potentially fix) later patches.

	Ingo

Re: [PATCH 1/2] CRED: Rename cred_exec_mutex to reflect that it's a guard against ptrace

[James Morris]

On Fri, 8 May 2009, David Howells wrote:

> Rename cred_exec_mutex to reflect that it's a guard against foreign
> intervention on a process's credential state, such as is made by ptrace().  The
> attachment of a debugger to a process affects execve()'s calculation of the new
> credential state - _and_ also setprocattr()'s calculation of that state.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

Applied to 
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next


-- 
James Morris
<jmorris@namei.org>