lkml.org 
[lkml]   [2013]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] secure unlock_task_sighand() call
Naveen,

sorry for the terse and neglectful reply yesterday.

Actually, when I re-read the Linus's email, I think he already explained
everything, so let me repeat:

On 12/21, Linus Torvalds wrote:
>
> Did you actually *see* the problem, or was this just from looking at the code?

Yes. Because this code assumes that lock_task_sighand() must not fail.
If it fails, we have a problem which should be fixed.

> We have coredump serialization in exit_mm() that I think *should* make
> this all ok - if we still see p->mm matching our mm, I don't think it
> should be able to get to __exit_signal() and make the sighand go away,
> so the lock_task_sighand() shouldn't ever fail.

Yes, exactly.

Note that if we ignore exec, we do not need lock_task_sighand() at all,
we could simply do spin_lock_irq(p->sighand->siglock).

The caller holds mm->mmap_sem for writing, if we see p->mm == mm it
simply can not pass exit_mm() which does down_read(&mm->mmap_sem), so
this task can not exit.

The problem is, this task can change its ->sighand in de_thread(), that
is why we need lock_task_sighand(). But if it does exec, it can't pass
exec_mmap() by the same reason, we hold mmap_sem.

> > if (p->mm) {
> > if (unlikely(p->mm == mm)) {
> > - lock_task_sighand(p, &flags);
> > - nr += zap_process(p, exit_code);
> > - unlock_task_sighand(p, &flags);
> > + if (lock_task_sighand(p, &flags) {
> > + nr += zap_process(p, exit_code);

But we can't silently skip a process with the same ->mm. We can't even
skip the execing thread task if it is going to change its ->mm, even if
it is single-threaded. Note that exec_mmap() will notice mm->core_state
and fail. So every task with the same mm should be accounted because it
will play with core_state->nr_threads in exit_mm(). And it should be
killed because otherwise coredump_wait() can sleep "forever".

So this is not the right change in any case. If lock_task_sighand() can
fail we should fix something else.

Oleg.



\
 
 \ /
  Last update: 2013-12-22 16:01    [W:0.102 / U:0.736 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site