Re: [PATCH] signal: don't silently convert SI_USER signals to non-current pidfd
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
Re: [PATCH] signal: don't silently convert SI_USER signals to non-current pidfd
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
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
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
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
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
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
Reviewed-by: Christian Brauner
As discussed in
https://lore.kernel.org/lkml/20190330012229.yt3hecmgaj2r6...@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
>
[PATCH] signal: don't silently convert SI_USER signals to non-current pidfd
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
---
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
