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

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

[Jann Horn]

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>
---
 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

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

[Christian Brauner]

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
> 

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

[Oleg Nesterov]

On 03/30, Jann Horn wrote:
>
> --- 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;

ACK.


but perhaps it should always fail, even if task_pid(current) == pid.

sys_rt_sigqueueinfo() allows to send any siginfo to yourself, but this is only needed
for checkpoint/restart.

Oleg.

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

[Christian Brauner]

On Mon, Apr 08, 2019 at 03:13:16PM +0200, Oleg Nesterov wrote:
> On 03/30, Jann Horn wrote:
> >
> > --- 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;
> 
> ACK.
> 

Sorry Oleg. I missed that message somehow.
> 
> but perhaps it should always fail, even if task_pid(current) == pid.
> 
> sys_rt_sigqueueinfo() allows to send any siginfo to yourself, but this is only needed
> for checkpoint/restart.

Yes, that's why this was added. I would leave it in exactly because of
checkpoint/restart. I have sympathies for "this [...] project by various
mad Russians" [1] and it doesn't really hurt us. :)

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=099469502f62fbe0d7e4f0b83a2f22538367f734

Christian

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

[Oleg Nesterov]

On 04/17, Christian Brauner wrote:
>
> On Mon, Apr 08, 2019 at 03:13:16PM +0200, Oleg Nesterov wrote:
> >
> > but perhaps it should always fail, even if task_pid(current) == pid.
> >
> > sys_rt_sigqueueinfo() allows to send any siginfo to yourself, but this is only needed
> > for checkpoint/restart.
>
> Yes, that's why this was added. I would leave it in exactly because of
> checkpoint/restart.

I don't understand...

c/r doesn't need this "feature" in pidfd_send_signal(), so it can be removed.
But,

> I have sympathies for "this [...] project by various
> mad Russians" [1] and it doesn't really hurt us. :)

I won't argue, I agree it doesn't hurt and looks consistent with sys_rt_sigqueueinfo.

Oleg.

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

[Christian Brauner]

On Wed, Apr 17, 2019 at 03:16:03PM +0200, Oleg Nesterov wrote:
> On 04/17, Christian Brauner wrote:
> >
> > On Mon, Apr 08, 2019 at 03:13:16PM +0200, Oleg Nesterov wrote:
> > >
> > > but perhaps it should always fail, even if task_pid(current) == pid.
> > >
> > > sys_rt_sigqueueinfo() allows to send any siginfo to yourself, but this is only needed
> > > for checkpoint/restart.
> >
> > Yes, that's why this was added. I would leave it in exactly because of
> > checkpoint/restart.
> 
> I don't understand...
> 
> c/r doesn't need this "feature" in pidfd_send_signal(), so it can be removed.
> But,

Just out of curiosity: in what sense? They don't need it since they have
other ways of doing this or they *can't* use it for some other reason
I'm not seeing?

Christian

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

[Oleg Nesterov]

On 04/17, Christian Brauner wrote:
>
> On Wed, Apr 17, 2019 at 03:16:03PM +0200, Oleg Nesterov wrote:
> > On 04/17, Christian Brauner wrote:
> > >
> > > On Mon, Apr 08, 2019 at 03:13:16PM +0200, Oleg Nesterov wrote:
> > > >
> > > > but perhaps it should always fail, even if task_pid(current) == pid.
> > > >
> > > > sys_rt_sigqueueinfo() allows to send any siginfo to yourself, but this is only needed
> > > > for checkpoint/restart.
> > >
> > > Yes, that's why this was added. I would leave it in exactly because of
> > > checkpoint/restart.
> >
> > I don't understand...
> >
> > c/r doesn't need this "feature" in pidfd_send_signal(), so it can be removed.
> > But,
>
> Just out of curiosity: in what sense? They don't need it since they have
> other ways of doing this

Yes. The restarting process needs to "restore" the pending signals, including the
signals with si_code >= 0. It does this using tgsigqueueinfo() and that is why we
allow this if the signal sent to itself.

So criu simply doesn't need pidfd_send_signal() to do this. And at the same time,

> or they *can't* use it for some other reason

Yes again. pidfd_send_signal() does kill_pid_info(), so it can't be used to restore
the "per-thread" task->pending signals.

Oleg.

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

[Christian Brauner]

On Wed, Apr 17, 2019 at 03:50:03PM +0200, Oleg Nesterov wrote:
> On 04/17, Christian Brauner wrote:
> >
> > On Wed, Apr 17, 2019 at 03:16:03PM +0200, Oleg Nesterov wrote:
> > > On 04/17, Christian Brauner wrote:
> > > >
> > > > On Mon, Apr 08, 2019 at 03:13:16PM +0200, Oleg Nesterov wrote:
> > > > >
> > > > > but perhaps it should always fail, even if task_pid(current) == pid.
> > > > >
> > > > > sys_rt_sigqueueinfo() allows to send any siginfo to yourself, but this is only needed
> > > > > for checkpoint/restart.
> > > >
> > > > Yes, that's why this was added. I would leave it in exactly because of
> > > > checkpoint/restart.
> > >
> > > I don't understand...
> > >
> > > c/r doesn't need this "feature" in pidfd_send_signal(), so it can be removed.
> > > But,
> >
> > Just out of curiosity: in what sense? They don't need it since they have
> > other ways of doing this
> 
> Yes. The restarting process needs to "restore" the pending signals, including the
> signals with si_code >= 0. It does this using tgsigqueueinfo() and that is why we
> allow this if the signal sent to itself.
> 
> So criu simply doesn't need pidfd_send_signal() to do this. And at the same time,
> 
> > or they *can't* use it for some other reason
> 
> Yes again. pidfd_send_signal() does kill_pid_info(), so it can't be used to restore
> the "per-thread" task->pending signals.

In the future pidfd_send_signal() will gain the flag
PIDFD_SIGNAL_THREAD. Jann and I already have a patch for this but we're
holding off until the need arises.

Christian