Re: [PATCH] fork: don't copy inconsistent signal handler state to child
From: Michal Hocko
Date: Mon Jul 02 2018 - 11:13:56 EST
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@xxxxxxxxxx>
Acked-by: Michal Hocko <mhocko@xxxxxxxx>
> ---
> 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(¤t->sighand->siglock);
> memcpy(sig->action, current->sighand->action, sizeof(sig->action));
> + spin_unlock_irq(¤t->sighand->siglock);
> return 0;
> }
>
> --
> 2.18.0.399.gad0ab374a1-goog
>
--
Michal Hocko
SUSE Labs