Re: [PATCH] signal: don't silently convert SI_USER signals to non-current pidfd

[Christian Brauner <christian@brauner.io>]

On Sat, Mar 30, 2019 at 03:12:32AM +0100, Jann Horn wrote:
> The current sys_pidfd_send_signal() silently turns signals with explicit
> SI_USER context that are sent to non-current tasks into signals with
> kernel-generated siginfo.
> This is unlike do_rt_sigqueueinfo(), which returns -EPERM in this case.
> If a user actually wants to send a signal with kernel-provided siginfo,
> they can do that with pidfd_send_signal(pidfd, sig, NULL, 0); so allowing
> this case is unnecessary.
> 
> Instead of silently replacing the siginfo, just bail out with an error;
> this is consistent with other interfaces and avoids special-casing behavior
> based on security checks.
> 
> Fixes: 3eb39f47934f ("signal: add pidfd_send_signal() syscall")
> Signed-off-by: Jann Horn <jannh@google.com>

Reviewed-by: Christian Brauner <christian@brauner.io>

As discussed in
https://lore.kernel.org/lkml/20190330012229.yt3hecmgaj2r6vp7@brauner.io
targeting this for a 5.1 rc.

> ---
>  kernel/signal.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index b7953934aa99..f98448cf2def 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3605,16 +3605,11 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
>  		if (unlikely(sig != kinfo.si_signo))
>  			goto err;
>  
> +		/* Only allow sending arbitrary signals to yourself. */
> +		ret = -EPERM;
>  		if ((task_pid(current) != pid) &&
> -		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) {
> -			/* Only allow sending arbitrary signals to yourself. */
> -			ret = -EPERM;
> -			if (kinfo.si_code != SI_USER)
> -				goto err;
> -
> -			/* Turn this into a regular kill signal. */
> -			prepare_kill_siginfo(sig, &kinfo);
> -		}
> +		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
> +			goto err;
>  	} else {
>  		prepare_kill_siginfo(sig, &kinfo);
>  	}
> -- 
> 2.21.0.392.gf8f6787159e-goog
>