lkml.org 
[lkml]   [2018]   [Jul]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 20/20] signal: Don't restart fork when signals come in.
Linus Torvalds <torvalds@linux-foundation.org> writes:

> This is completely broken.
>
> On Mon, Jul 23, 2018 at 8:27 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index 6c358846a8b8..6ee5822f0085 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -1602,6 +1603,24 @@ static __latent_entropy struct task_struct *copy_process(
>> {
>> int retval;
>> struct task_struct *p;
>> + struct multiprocess_signals delayed;
>> +
>> + /*
>> + * Force any signals received before this point to be delivered
>> + * before the fork happens. Collect up signals sent to multiple
>> + * processes that happen during the fork and delay them so that
>> + * they appear to happen after the fork.
>> + */
>> + sigemptyset(&delayed.signal);
>> + INIT_HLIST_NODE(&delayed.node);
>> +
>> + spin_lock_irq(&current->sighand->siglock);
>> + if (!(clone_flags & CLONE_THREAD))
>> + hlist_add_head(&delayed.node, &current->signal->multiprocess);
>
> Here you add the entry to the multiprocess list.
>
>> + recalc_sigpending();
>> + spin_unlock_irq(&current->sighand->siglock);
>> + if (signal_pending(current))
>> + return ERR_PTR(restart_syscall());
>
> .. and here you return with the list entry still there, pointing to
> the stack that you now no longer use.
>
> The same is true of *all* the error cases, because the only point you
> remove it is for the success case:

Yes you are quite right. Easy enough to fix, but it definitely needs
to be fixed.

I will respin.

Eric


>> @@ -1979,6 +1982,8 @@ static __latent_entropy struct task_struct *copy_process(
>> attach_pid(p, PIDTYPE_TGID);
>> attach_pid(p, PIDTYPE_PGID);
>> attach_pid(p, PIDTYPE_SID);
>> + p->signal->shared_pending.signal = delayed.signal;
>> + hlist_del(&delayed.node);
>
> So for all the error cases, you leave a dangling pointer to the
> current stack in that signal handler, and then return an error.
>
> Guaranteed stack and list corruption.
>
> Linus

\
 
 \ /
  Last update: 2018-07-24 19:59    [W:0.090 / U:2.412 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site