On 05/07, James Morris wrote:
>
> On Thu, 7 May 2009, Ingo Molnar wrote:
>
> > > Boot tested with SELinux.
> >
> > thanks! Below are the two patches i wrote and tested.
>
> Do you need these in your tree, or are they ok going into linux-next via
> mine?
James, Ingo, what happend with these renames? I don't see them
in linux/kernel/git/jmorris/security-testing-2.6.git.
I'd like to do some further cleanups on top of these changes,
or if they were dropped, against Linus's tree.
Oleg.
Umm, I think I mentioned it before: if you really need to rename that
beast please kill the ptrace part as we use it for all kinds of other
things.
On 06/23, Christoph Hellwig wrote:
>
> Umm, I think I mentioned it before: if you really need to rename that
> beast please kill the ptrace part as we use it for all kinds of other
> things.
Agreed, it would be nice to rename (and move out of ptrace.c) but I am
not going to do this, I can't suggest the better naming.
But I think it would be nice to kill __ptrace_may_access() and use
ptrace_may_access() everywhere. ptrace_attach() is trivial, the only
other caller is mm_for_maps().
Oleg.
It would be nice to kill __ptrace_may_access(). It requires task_lock(),
but this lock is only needed to read mm->flags in the middle.
Convert mm_for_maps() to use ptrace_may_access(), this also simplifies
the code a little bit.
Also, we do not need to take ->mmap_sem in advance. In fact I think
mm_for_maps() should not play with ->mmap_sem at all, the caller should
take this lock.
With or without this patch, without ->cred_guard_mutex held we can race
with exec() and get the new ->mm but check old creds.
Signed-off-by: Oleg Nesterov <[email protected]>
--- WAIT/fs/proc/base.c~1_MM_FOR_MAPS 2009-06-17 14:11:26.000000000 +0200
+++ WAIT/fs/proc/base.c 2009-06-23 20:16:44.000000000 +0200
@@ -237,20 +237,19 @@ struct mm_struct *mm_for_maps(struct tas
struct mm_struct *mm = get_task_mm(task);
if (!mm)
return NULL;
+ if (mm != current->mm) {
+ /*
+ * task->mm can be changed before security check,
+ * in that case we must notice the change after.
+ */
+ if (!ptrace_may_access(task, PTRACE_MODE_READ) ||
+ mm != task->mm) {
+ mmput(mm);
+ return NULL;
+ }
+ }
down_read(&mm->mmap_sem);
- task_lock(task);
- if (task->mm != mm)
- goto out;
- if (task->mm != current->mm &&
- __ptrace_may_access(task, PTRACE_MODE_READ) < 0)
- goto out;
- task_unlock(task);
return mm;
-out:
- task_unlock(task);
- up_read(&mm->mmap_sem);
- mmput(mm);
- return NULL;
}
static int proc_pid_cmdline(struct task_struct *task, char * buffer)
On Tue, 23 Jun 2009, Oleg Nesterov wrote:
> On 05/07, James Morris wrote:
> >
> > On Thu, 7 May 2009, Ingo Molnar wrote:
> >
> > > > Boot tested with SELinux.
> > >
> > > thanks! Below are the two patches i wrote and tested.
> >
> > Do you need these in your tree, or are they ok going into linux-next via
> > mine?
>
> James, Ingo, what happend with these renames? I don't see them
> in linux/kernel/git/jmorris/security-testing-2.6.git.
The context was probably lost for some reason, e.g. waiting on acks or for
reviewer questions to be resolved. Please resend if you don't see them
applied and also always indicate any depdendencies on other patches.
It is unliklely that I'd apply security-related patches of any kind of
non-trivial nature without reviewed-by or acked-by lines from other
developers, and you may need to engage reviewers on an individual basis to
make that happen.
- James
--
James Morris
<[email protected]>
Quoting Oleg Nesterov ([email protected]):
> It would be nice to kill __ptrace_may_access(). It requires task_lock(),
> but this lock is only needed to read mm->flags in the middle.
>
> Convert mm_for_maps() to use ptrace_may_access(), this also simplifies
> the code a little bit.
>
> Also, we do not need to take ->mmap_sem in advance. In fact I think
> mm_for_maps() should not play with ->mmap_sem at all, the caller should
> take this lock.
Yeah I think that makes sense.
> With or without this patch, without ->cred_guard_mutex held we can race
> with exec() and get the new ->mm but check old creds.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
reasoning on the security check also makes sense.
Reviewed-by: Serge Hallyn <[email protected]>
> --- WAIT/fs/proc/base.c~1_MM_FOR_MAPS 2009-06-17 14:11:26.000000000 +0200
> +++ WAIT/fs/proc/base.c 2009-06-23 20:16:44.000000000 +0200
> @@ -237,20 +237,19 @@ struct mm_struct *mm_for_maps(struct tas
> struct mm_struct *mm = get_task_mm(task);
> if (!mm)
> return NULL;
> + if (mm != current->mm) {
> + /*
> + * task->mm can be changed before security check,
> + * in that case we must notice the change after.
> + */
> + if (!ptrace_may_access(task, PTRACE_MODE_READ) ||
> + mm != task->mm) {
> + mmput(mm);
> + return NULL;
> + }
> + }
> down_read(&mm->mmap_sem);
> - task_lock(task);
> - if (task->mm != mm)
> - goto out;
> - if (task->mm != current->mm &&
> - __ptrace_may_access(task, PTRACE_MODE_READ) < 0)
> - goto out;
> - task_unlock(task);
> return mm;
> -out:
> - task_unlock(task);
> - up_read(&mm->mmap_sem);
> - mmput(mm);
> - return NULL;
> }
>
> static int proc_pid_cmdline(struct task_struct *task, char * buffer)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Also, we do not need to take ->mmap_sem in advance. In fact I think
> mm_for_maps() should not play with ->mmap_sem at all, the caller should
> take this lock.
Agreed. It has only one caller (though two forks of it) in
fs/proc/task_{no,}mmu.c and it looks easy to change.
> With or without this patch, without ->cred_guard_mutex held we can race
> with exec() and get the new ->mm but check old creds.
It looks safe and proper for mm_for_maps() to take that mutex around its
check. Your patch looks good to me as it is, and taking cred_guard_mutex
can be another security fix patch on top.
Thanks,
Roland
On Tue, 23 Jun 2009, Serge E. Hallyn wrote:
> Quoting Oleg Nesterov ([email protected]):
> > It would be nice to kill __ptrace_may_access(). It requires task_lock(),
> > but this lock is only needed to read mm->flags in the middle.
> >
> > Convert mm_for_maps() to use ptrace_may_access(), this also simplifies
> > the code a little bit.
> >
> > Also, we do not need to take ->mmap_sem in advance. In fact I think
> > mm_for_maps() should not play with ->mmap_sem at all, the caller should
> > take this lock.
>
> Yeah I think that makes sense.
>
> > With or without this patch, without ->cred_guard_mutex held we can race
> > with exec() and get the new ->mm but check old creds.
> >
> > Signed-off-by: Oleg Nesterov <[email protected]>
>
> reasoning on the security check also makes sense.
>
> Reviewed-by: Serge Hallyn <[email protected]>
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next
--
James Morris
<[email protected]>
On 06/24, Roland McGrath wrote:
>
> > Also, we do not need to take ->mmap_sem in advance. In fact I think
> > mm_for_maps() should not play with ->mmap_sem at all, the caller should
> > take this lock.
>
> Agreed. It has only one caller (though two forks of it) in
> fs/proc/task_{no,}mmu.c and it looks easy to change.
>
> > With or without this patch, without ->cred_guard_mutex held we can race
> > with exec() and get the new ->mm but check old creds.
>
> It looks safe and proper for mm_for_maps() to take that mutex around its
> check. Your patch looks good to me as it is, and taking cred_guard_mutex
> can be another security fix patch on top.
Agreed, will do.
Oleg.