On Tue, Feb 20, 2024 at 10:02:56AM +0100, Oleg Nesterov wrote:
> On 02/20, Christian Brauner wrote:
> >
> > On Fri, Feb 16, 2024 at 07:12:14PM +0100, Oleg Nesterov wrote:
> > > On 02/16, Christian Brauner wrote:
> > > >
> > > > > SI_USER means that the target can trust the values of si_pid/si_uid
> > > > > in siginfo.
> > > >
> > > > Bah, what an annoying nonsense. I see that this can be used to emulate
> > > > stuff like SI_TIMER and SI_ASYNCIO. But I very much doubt the value of
> > > > e.g., emulating SI_DETHREAD. Maybe I'm missing something very obvious.
> > >
> > > I don't understand...
> >
> > My question was what the purpose of being able to to set si_code to
> > e.g., SI_DETHREAD is and then to send a signal to yourself? Because it
> > looks like that's what rt_{tg}sigqueueinfo() and pidfd_send_signal()
> > allows the caller to do. I'm just trying to understand use-cases for
> > this.
>
> Ah. IIRC criu uses this hack to restore the pending (arbitrary) signals
> collected at dump time.
>
> I was a bit surprise sys_pidfd_send_signal() allows this hack too, I don't
I think that we simply mirrored the restrictions in the other system
calls.
> think that criu uses pidfd at restore time, but I do not know.
Hm, I just checked and it doesn't use pidfd_send_signal(). It uses
pidfds but only for pid reuse detection for RPC clients.
So right now si_code is blocked for >= 0 and for SI_TKILL. If we were to
simply ensure that si_code can't be < 0 then this amounts to effectively
blocking @info from being filled in by userspace at all. Because 0 is a
valid value.
So could we just _try_ and either ignore the @info argument completely
or consistenly report EINVAL when @info is non-NULL and see if anyone
reports a regression? If there ever is a valid use-case then we can just
add a flag argument PIDFD_SIGNAL_INFO to indicate that @info should be
taken into account.
So something like the completely untested?
diff --git a/kernel/signal.c b/kernel/signal.c
index cf6539a6b1cb..2cca42175480 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3849,22 +3849,6 @@ static bool access_pidfd_pidns(struct pid *pid)
return true;
}
-static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo,
- siginfo_t __user *info)
-{
-#ifdef CONFIG_COMPAT
- /*
- * Avoid hooking up compat syscalls and instead handle necessary
- * conversions here. Note, this is a stop-gap measure and should not be
- * considered a generic solution.
- */
- if (in_compat_syscall())
- return copy_siginfo_from_user32(
- kinfo, (struct compat_siginfo __user *)info);
-#endif
- return copy_siginfo_from_user(kinfo, info);
-}
-
static struct pid *pidfd_to_pid(const struct file *file)
{
struct pid *pid;
@@ -3911,6 +3895,10 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
return -EINVAL;
+ /* Currently unused. */
+ if (info)
+ return -EINVAL;
+
f = fdget(pidfd);
if (!f.file)
return -EBADF;
@@ -3945,23 +3933,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
break;
}
- if (info) {
- ret = copy_siginfo_from_user_any(&kinfo, info);
- if (unlikely(ret))
- goto err;
-
- ret = -EINVAL;
- 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))
- goto err;
- } else {
- prepare_kill_siginfo(sig, &kinfo, type);
- }
+ prepare_kill_siginfo(sig, &kinfo, type);
if (type == PIDTYPE_PGID)
ret = kill_pgrp_info(sig, &kinfo, pid);
