[PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment

[PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment

[Oleg Nesterov]

tracehook_unsafe_exec() doesn't need task_lock(), remove the old comment.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- PTRACE/include/linux/tracehook.h~	2009-04-06 00:03:42.000000000 +0200
+++ PTRACE/include/linux/tracehook.h	2009-04-23 23:20:20.000000000 +0200
@@ -142,8 +142,6 @@ static inline void tracehook_report_sysc
  * @task:		current task doing exec
  *
  * Return %LSM_UNSAFE_* bits applied to an exec because of tracing.
- *
- * Called with task_lock() held on @task.
  */
 static inline int tracehook_unsafe_exec(struct task_struct *task)
 {

Re: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment

[Roland McGrath]

> tracehook_unsafe_exec() doesn't need task_lock(), remove the old comment.

Please make it instead say that cred_exec_mutex is held by the caller
through the exec.  That locking takes the place of what task_lock() was
doing before (e.g. interlock with PTRACE_ATTACH).


Thanks,
Roland

ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment)

[Oleg Nesterov]

(change subject, add more CCs)

On 04/23, Roland McGrath wrote:
>
> > tracehook_unsafe_exec() doesn't need task_lock(), remove the old comment.
>
> Please make it instead say that cred_exec_mutex is held by the caller
> through the exec.

Yes. Except it looks like ->cred_exec_mutex is never used in fact.

ptrace_attach() takes current->cred_exec_mutex ? iow, ptrace and exec
use different locks ?

Oleg.

Re: ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment)

[Roland McGrath]

> (change subject, add more CCs)
> 
> On 04/23, Roland McGrath wrote:
> >
> > > tracehook_unsafe_exec() doesn't need task_lock(), remove the old comment.
> >
> > Please make it instead say that cred_exec_mutex is held by the caller
> > through the exec.
> 
> Yes. Except it looks like ->cred_exec_mutex is never used in fact.
> 
> ptrace_attach() takes current->cred_exec_mutex ? iow, ptrace and exec
> use different locks ?

Good point!  That sure looks broken to me.  David?

Either that should be task->cred_exec_mutex, or Oleg and I
are completely confused about what cred_exec_mutex is meant for.


Thanks,
Roland

Re: ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment)

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> Yes. Except it looks like ->cred_exec_mutex is never used in fact.

I must to be missing something...  I see that:

	int ptrace_attach(struct task_struct *task)
	{
	...
		/* Protect exec's credential calculations against our interference;
		 * SUID, SGID and LSM creds get determined differently under ptrace.
		 */
		retval = mutex_lock_interruptible(&current->cred_exec_mutex);
	...
	}

And:

	int do_execve(...)
	{
	...
		retval = mutex_lock_interruptible(&current->cred_exec_mutex);
		if (retval < 0)
			goto out_free;
	...
	}

So how is it not used?

David

Re: ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment)

[Oleg Nesterov]

On 04/25, David Howells wrote:
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Yes. Except it looks like ->cred_exec_mutex is never used in fact.
>
> I must to be missing something...  I see that:
>
> 	int ptrace_attach(struct task_struct *task)
> 	{
> 	...
> 		/* Protect exec's credential calculations against our interference;
> 		 * SUID, SGID and LSM creds get determined differently under ptrace.
> 		 */
> 		retval = mutex_lock_interruptible(&current->cred_exec_mutex);
> 	...
> 	}
>
> And:
>
> 	int do_execve(...)
> 	{
> 	...
> 		retval = mutex_lock_interruptible(&current->cred_exec_mutex);
> 		if (retval < 0)
> 			goto out_free;
> 	...
> 	}

Sorry David, I was very unclear.

These 2 current's are different tasks, and hence we take to unrelated locks.

We can never block taking current->cred_exec_mutex because nobody else
touches this mutex, we always use current. This means this lock is "nop".

Unless I missed something, ptrace_attach() should take task->cred_exec_mutex.

Oleg.

Re: ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment)

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> Unless I missed something, ptrace_attach() should take task->cred_exec_mutex.

Bah!  Yes.

David

[PATCH] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex

[Oleg Nesterov]

ptrace_attach() needs task->cred_exec_mutex, not current->cred_exec_mutex.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- PTRACE/kernel/ptrace.c~	2009-04-15 12:53:58.000000000 +0200
+++ PTRACE/kernel/ptrace.c	2009-04-27 01:22:51.000000000 +0200
@@ -188,7 +188,7 @@ int ptrace_attach(struct task_struct *ta
 	/* Protect exec's credential calculations against our interference;
 	 * SUID, SGID and LSM creds get determined differently under ptrace.
 	 */
-	retval = mutex_lock_interruptible(&current->cred_exec_mutex);
+	retval = mutex_lock_interruptible(&task->cred_exec_mutex);
 	if (retval  < 0)
 		goto out;
 
@@ -232,7 +232,7 @@ repeat:
 bad:
 	write_unlock_irqrestore(&tasklist_lock, flags);
 	task_unlock(task);
-	mutex_unlock(&current->cred_exec_mutex);
+	mutex_unlock(&task->cred_exec_mutex);
 out:
 	return retval;
 }

Re: [PATCH] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex

[Roland McGrath]

Acked-by: Roland McGrath <roland@redhat.com>

Re: [PATCH] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> ptrace_attach() needs task->cred_exec_mutex, not current->cred_exec_mutex.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

[GIT] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex

[James Morris]

Here's the patch ready to pull for 2.6.30.

The following changes since commit ce8a7424d23a36f043d0de8484f888971c831119:
  Tim Abbott (1):
        sparc: convert to use __HEAD and HEAD_TEXT macros.

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6 for-linus

Oleg Nesterov (1):
      ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex

 kernel/ptrace.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


-- 
James Morris
<jmorris@namei.org>

[PATCH] ptrace-tracehook_unsafe_exec-remove-the-stale-comment-fix

[Oleg Nesterov]

(on top of ptrace-tracehook_unsafe_exec-remove-the-stale-comment.patch)

On 04/23, Roland McGrath wrote:
>
> > tracehook_unsafe_exec() doesn't need task_lock(), remove the old comment.
>
> Please make it instead say that cred_exec_mutex is held by the caller
> through the exec.

Thanks.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- PTRACE/include/linux/tracehook.h~COMMENT_FIX	2009-04-23 23:20:20.000000000 +0200
+++ PTRACE/include/linux/tracehook.h	2009-04-27 21:59:54.000000000 +0200
@@ -142,6 +142,8 @@ static inline void tracehook_report_sysc
  * @task:		current task doing exec
  *
  * Return %LSM_UNSAFE_* bits applied to an exec because of tracing.
+ *
+ * task->cred_exec_mutex is held by the caller through the do_execve().
  */
 static inline int tracehook_unsafe_exec(struct task_struct *task)
 {