tracehook_unsafe_exec() doesn't need task_lock(), remove the old comment.
Signed-off-by: Oleg Nesterov <[email protected]>
--- 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)
{
> 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
(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.
> (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
Oleg Nesterov <[email protected]> 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(¤t->cred_exec_mutex);
...
}
And:
int do_execve(...)
{
...
retval = mutex_lock_interruptible(¤t->cred_exec_mutex);
if (retval < 0)
goto out_free;
...
}
So how is it not used?
David
On 04/25, David Howells wrote:
>
> Oleg Nesterov <[email protected]> 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(¤t->cred_exec_mutex);
> ...
> }
>
> And:
>
> int do_execve(...)
> {
> ...
> retval = mutex_lock_interruptible(¤t->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.
Oleg Nesterov <[email protected]> wrote:
> Unless I missed something, ptrace_attach() should take task->cred_exec_mutex.
Bah! Yes.
David
ptrace_attach() needs task->cred_exec_mutex, not current->cred_exec_mutex.
Signed-off-by: Oleg Nesterov <[email protected]>
--- 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(¤t->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(¤t->cred_exec_mutex);
+ mutex_unlock(&task->cred_exec_mutex);
out:
return retval;
}
Acked-by: Roland McGrath <[email protected]>
Oleg Nesterov <[email protected]> wrote:
> ptrace_attach() needs task->cred_exec_mutex, not current->cred_exec_mutex.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: David Howells <[email protected]>
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
<[email protected]>
(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 <[email protected]>
--- 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)
{