linux-kernel - Re: [PATCH] signal: don't silently convert SI

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: <20190330021729.3utry4va44jt4ib6@brauner.io>
Date:   Sat, 30 Mar 2019 03:17:30 +0100
From:   Christian Brauner <christian@...uner.io>
To:     Jann Horn <jannh@...gle.com>
Cc:     Oleg Nesterov <oleg@...hat.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Arnd Bergmann <arnd@...db.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] signal: don't silently convert SI_USER signals to
 non-current pidfd

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

Reviewed-by: Christian Brauner <christian@...uner.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
>