Received-SPF: pass (google.com: domain of linux-kernel+bounces-72684-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 20 Feb 2024 10:22:59 +0100
From: Christian Brauner <brauner@kernel.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Andy Lutomirski <luto@amacapital.net>, 
	"Eric W. Biederman" <ebiederm@xmission.com>, Tycho Andersen <tycho@tycho.pizza>, linux-api@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect
 PIDFD_THREAD
Message-ID: <20240220-pragmatisch-parzelle-8a1d10a94fae@brauner>
References: <20240210131518.GC27557@redhat.com>
 <20240210-chihuahua-hinzog-3945b6abd44a@brauner>
 <20240210165133.GD27557@redhat.com>
 <20240214123655.GB16265@redhat.com>
 <20240216-albern-aufwiegen-1de327c7dafd@brauner>
 <20240216130625.GA8723@redhat.com>
 <20240216-ohnedies-improvisieren-58edcc102b6a@brauner>
 <20240216181214.GA10393@redhat.com>
 <20240220-einwurf-depesche-d8682be0370c@brauner>
 <20240220090255.GA7783@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240220090255.GA7783@redhat.com>

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