Re: [PATCH 20/20] signal: Don't restart fork when signals come in.

From: Linus Torvalds
Date: Tue Jul 24 2018 - 13:28:13 EST


This is completely broken.

On Mon, Jul 23, 2018 at 8:27 PM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> 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:

> @@ -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