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

[Michal Hocko <mhocko@kernel.org>]

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@google.com>

Acked-by: Michal Hocko <mhocko@suse.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