[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 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

[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>

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

[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