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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFxNijeenFd6o7fg3iJFqAjdTZLaFV2=Eaqbg1fDOzx26A@mail.gmail.com>
Date:   Tue, 24 Jul 2018 10:27:58 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Oleg Nesterov <oleg@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Wen Yang <wen.yang99@....com.cn>, majiang <ma.jiang@....com.cn>
Subject: Re: [PATCH 20/20] signal: Don't restart fork when signals come in.

This is completely broken.

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

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