linux-kernel - Re: [PATCH] fork: don't copy inconsistent signal handler state to child

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] [day] [month] [year] [list]

Message-ID: <20180702151346.GD19043@dhcp22.suse.cz>
Date:   Mon, 2 Jul 2018 17:13:46 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Jann Horn <jannh@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, Rik van Riel <riel@...hat.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] fork: don't copy inconsistent signal handler state to
 child

On Mon 02-07-18 16:51:08, Jann Horn wrote:
> Before this change, if a multithreaded process forks while one of its
> threads is changing a signal handler using sigaction(), the memcpy() in
> copy_sighand() can race with the struct assignment in do_sigaction().
> It isn't clear whether this can cause corruption of the userspace signal
> handler pointer, but it definitely can cause inconsistency between
> different fields of struct sigaction.
> 
> Take the appropriate spinlock to avoid this.
> 
> I have tested that this patch prevents inconsistency between sa_sigaction
> and sa_flags, which is possible before this patch.

Looks sensible to me.

> Signed-off-by: Jann Horn <jannh@...gle.com>

Acked-by: Michal Hocko <mhocko@...e.com>

> ---
> I haven't added a CC stable line to this patch because I feel that it
> doesn't quite meet the bar; if anyone disagrees, please say so.
> 
> 
>  kernel/fork.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 9440d61b925c..4c86e8daae53 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1394,7 +1394,9 @@ static int copy_sighand(unsigned long clone_flags, struct task_struct *tsk)
>  		return -ENOMEM;
>  
>  	atomic_set(&sig->count, 1);
> +	spin_lock_irq(&current->sighand->siglock);
>  	memcpy(sig->action, current->sighand->action, sizeof(sig->action));
> +	spin_unlock_irq(&current->sighand->siglock);
>  	return 0;
>  }
>  
> -- 
> 2.18.0.399.gad0ab374a1-goog
> 

-- 
Michal Hocko
SUSE Labs