[PATCH] secure unlock_task_sighand() call

[PATCH] secure unlock_task_sighand() call

[naveen yadav]

Dear All,


When check code , we found below issue in zap_thread function().


>From 57bf616d0e20086d73122373baf799c675f4e3d5 Mon Sep 17 00:00:00 2001
From: Ajeet Yadav <ajeet.yadav.77@gmail.com>
Date: Sat, 21 Dec 2013 14:45:48 +0530
Subject: [PATCH] secure unlock_task_sighand() call

Since the return value was not checked for lock_task_sighand(),
there was a chance that spin_unlock_irqrestore being called
from unlock_task_sighand gets called wihout actually acquire
the lock, which inturn can lead to kernel crash.

Signed-off-by: Ajeet Yadav <ajeet.yadav.77@gmail.com>
Signed-off-by: Vaibhav Shinde <v.bhav.shinde@gmail.com>

---
 fs/coredump.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 6d8b4cd..447b02c 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -357,9 +357,10 @@ static inline int zap_threads(struct task_struct
*tsk, struct mm_struct *mm,
                do {
                        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);
+                                               unlock_task_sighand(p, &flags);
+                                       }
                                }
                                break;
                        }
--
1.7.9.5

Re: [PATCH] secure unlock_task_sighand() call

[Linus Torvalds]

Did you actually *see* the problem, or was this just from looking at the code?

I don't hate the patch, and it might be the right thing to do in any
case (just to avoid depending on subtle things), but this really *is*
subtle, and I'm adding Oleg to the participants since it is his code
(going back to 2006, no less).

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. But I might miss
something, and as mentioned the patch might be a good idea regardless
just to avoid overly subtle rules that can confuse people.

Oleg?

                   Linus

On Sat, Dec 21, 2013 at 1:55 AM, naveen yadav <yad.naveen@gmail.com> wrote:
>
> When check code , we found below issue in zap_thread function().
>
> From 57bf616d0e20086d73122373baf799c675f4e3d5 Mon Sep 17 00:00:00 2001
> From: Ajeet Yadav <ajeet.yadav.77@gmail.com>
> Date: Sat, 21 Dec 2013 14:45:48 +0530
> Subject: [PATCH] secure unlock_task_sighand() call
>
> Since the return value was not checked for lock_task_sighand(),
> there was a chance that spin_unlock_irqrestore being called
> from unlock_task_sighand gets called wihout actually acquire
> the lock, which inturn can lead to kernel crash.
>
> Signed-off-by: Ajeet Yadav <ajeet.yadav.77@gmail.com>
> Signed-off-by: Vaibhav Shinde <v.bhav.shinde@gmail.com>
>
> ---
>  fs/coredump.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 6d8b4cd..447b02c 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -357,9 +357,10 @@ static inline int zap_threads(struct task_struct
> *tsk, struct mm_struct *mm,
>                 do {
>                         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);
> +                                               unlock_task_sighand(p, &flags);
> +                                       }
>                                 }
>                                 break;
>                         }
> --
> 1.7.9.5

Re: [PATCH] secure unlock_task_sighand() call

[Oleg Nesterov]

On 12/21, Linus Torvalds wrote:
>
> We have coredump serialization in exit_mm() that I think *should* make
> this all ok - if we still see p->mm matching our mm,

Yes. And the comment says:

	lock_task_sighand(p)
	must be used. Since p->mm != NULL and we hold ->mmap_sem
	it can't fail.

IOW, this task can't pass exit_mm() and thus lock_task_sighand() can't
fail.

> >                 do {
> >                         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);
> > +                                               unlock_task_sighand(p, &flags);
> > +                                       }

I too do not think this is needed. But perhaps BUG_ON() make sense.

Note: just in case, this has another problem: while_each_thread() is racy.
We already have the initial fixes in -mm, this code (as other users)
should be converted to use for_each_thread(), I'll send the patch(es).

Oleg.

Re: [PATCH] secure unlock_task_sighand() call

[Oleg Nesterov]

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.

Re: [PATCH] secure unlock_task_sighand() call

[naveen yadav]

Happy Christmas !!!


Thanks Oleg and Linus for your reply.


We are facing OOPS during core dump on kernel 3.8.x on ARM target.

So we were doing core review and found this. We do not know whether
its big issue but thought to share this considering the problem


 Also I think in zap_process() there is no need to send SIGKILL to
ZOMBIE or DEAD process.

--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -271,17 +271,19 @@ static int zap_process(struct task_struct
*start, int exit_code)

-               if (t != current && t->mm) {
+               if (t->exit_state) {
+                       nr++;
+               } else if (t != current && t->mm) {
                        sigaddset(&t->pending.signal, SIGKILL);
                        signal_wake_up(t, 1);

Regards
Naveen



On Sun, Dec 22, 2013 at 8:04 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> 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.
>

Re: [PATCH] secure unlock_task_sighand() call

[Oleg Nesterov]

On 12/23, naveen yadav wrote:
>
> Happy Christmas !!!

Thanks, the same to you ;)

> We are facing OOPS during core dump on kernel 3.8.x on ARM target.

Do you have any traces? Any additional info?

Can you try the fresh kernels?

Not that I can recall any change in this area which could help, but
perhaps this is arm specific...

> So we were doing core review and found this.

Do you mean that with this patch the kernel doesn't crash?

>  Also I think in zap_process() there is no need to send SIGKILL to
> ZOMBIE or DEAD process.

Yes, it would be very wrong to account a zombie, but:

> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -271,17 +271,19 @@ static int zap_process(struct task_struct
> *start, int exit_code)
>
> -               if (t != current && t->mm) {
> +               if (t->exit_state) {
> +                       nr++;
> +               } else if (t != current && t->mm) {

This change adds no harm, but it is misleading and unneeded. Please note
that t->mm != NULL && t->exit_state != 0 is not possible, exit_mm() is
called before exit_notify(). IOW, a zombie thread can't have ->mm.

Oleg.

Re: [PATCH] secure unlock_task_sighand() call

[Linus Torvalds]

On Mon, Dec 23, 2013 at 4:29 AM, naveen yadav <yad.naveen@gmail.com> wrote:
>
> We are facing OOPS during core dump on kernel 3.8.x on ARM target.
>
> So we were doing core review and found this. We do not know whether
> its big issue but thought to share this considering the problem

Can you please post the oops, just in case that has any more hints.
Maybe the serialization is broken somehow on ARM, or there is some
ARM-specific bug. This code _is_ fairly subtle, but I don't think any
of the patches I've seen so far should really fix anything.

              Linus