Messages in this thread | | | From | (Eric W. Biederman) | Date | Tue, 24 Jul 2018 12:58:35 -0500 | Subject | Re: [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(¤t->sighand->siglock); >> + if (!(clone_flags & CLONE_THREAD)) >> + hlist_add_head(&delayed.node, ¤t->signal->multiprocess); > > Here you add the entry to the multiprocess list. > >> + recalc_sigpending(); >> + spin_unlock_irq(¤t->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
| |