Re: [PATCH v2 4/5] signal: PIDFD_SIGNAL_TID threads via pidfds

On Sat, Mar 30, 2019 at 02:22:29AM +0100, Christian Brauner wrote:

On Sat, Mar 30, 2019 at 02:06:34AM +0100, Jann Horn wrote:

quoted

On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner [off-list ref] wrote:

quoted

With the addition of pidfd_open() it is possible for users to reference a
specific thread by doing:

int pidfd = pidfd_open(<tid>, 0);

This means we can extend pidfd_send_signal() to signal a specific thread.
As promised in the commit for pidfd_send_signal() [1] the extension is
based on a flag argument, i.e. the scope of the signal delivery is based on
the flag argument, not on the type of file descriptor.
To this end the flag PIDFD_SIGNAL_TID is added. With this change we now
cover most of the functionality of all the other signal sending functions
combined:

[...]

quoted

diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h
index d6c7c0701997..b72f0ef84fe5 100644
--- a/include/uapi/linux/wait.h
+++ b/include/uapi/linux/wait.h

[...]

quoted

+/* Flags to pass to pidfd_send_signal */
+#define PIDFD_SIGNAL_TID 1 /* Send signal to specific thread */

nit: s/1/1U/; the flags argument is an `unsigned int`

Will change.

quoted

quoted

 #endif /* _UAPI_LINUX_WAIT_H */

diff --git a/kernel/signal.c b/kernel/signal.c
index eb97d0cc6ef7..9f93da85b2b9 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c

[...]

quoted

+static int pidfd_send_signal_specific(struct pid *pid, int sig,
+                                     struct kernel_siginfo *info)
+{
+       struct task_struct *p;
+       int error = -ESRCH;
+
+       rcu_read_lock();
+       p = pid_task(pid, PIDTYPE_PID);
+       if (p)
+               error = __do_send_specific(p, sig, info);
+       rcu_read_unlock();
+
+       return error;
+}
+
 /**
- * sys_pidfd_send_signal - send a signal to a process through a task file
- *                          descriptor
+ * sys_pidfd_send_signal - send a signal to a process through a pidfd
+
  * @pidfd:  the file descriptor of the process
  * @sig:    signal to be sent
  * @info:   the signal info
  * @flags:  future flags to be passed

nit: comment is outdated, it isn't "future flags" anymore

Will remove.

quoted

[...]

quoted

+ *   rt_tgsigqueueinfo(<tgid>, <tid>, <sig>, <uinfo>)
+ * - pidfd_send_signal(<pidfd>, <sig>, <info>, PIDFD_SIGNAL_TID);
+ *   which is equivalent to
+ *   rt_tgsigqueueinfo(<tgid>, <tid>, <sig>, <uinfo>)
+ *
  * In order to extend the syscall to threads and process groups the @flags
  * argument should be used. In essence, the @flags argument will determine
  * what is signaled and not the file descriptor itself. Put in other words,

nit: again, outdated comment about @flags

Will update.

quoted

[...]

quoted

@@ -3626,43 +3695,16 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
                prepare_kill_siginfo(sig, &kinfo);
        }

-       ret = kill_pid_info(sig, &kinfo, pid);
+       if (flags & PIDFD_SIGNAL_TID)
+               ret = pidfd_send_signal_specific(pid, sig, &kinfo);
+       else
+               ret = kill_pid_info(sig, &kinfo, pid);

nit: maybe give pidfd_send_signal_specific() and kill_pid_info() the
same signatures, since they perform similar operations with the same
argument types?

Yes, let's do
pidfd_send_signal_specific.(pid, sig, &kinfo);
kill_pid_info..............(pid, sig, &kinfo);

so it matches the argument order of the syscalls itself too.

Strike that. We should do:
pidfd_send_signal_specific.(sig, &kinfo, pid);
kill_pid_info..............(sig, &kinfo, pid);

because kill_pid_info() is called in multiple places so we would
needlessly shovle code around.

quoted

Something that was already kinda weird in the existing code, but is
getting worse with TIDs is the handling of SI_USER with siginfo.

Right, that's what we discussed earlier.

quoted

Copying context lines from above here:

        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;
                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);
                }
        } else {
                prepare_kill_siginfo(sig, &kinfo);
        }

So for signals to PIDs, the rule is that if you send siginfo with
SI_USER to yourself, the siginfo is preserved; otherwise the kernel
silently clobbers it. That's already kind of weird - silent behavior

Clobbers as in "silently replaces it whatever it seems fit?

quoted

difference depending on a security check. But now, for signals to
threads, I think the result is going to be that signalling the thread
group leader preserves information, and signalling any other thread
clobbers it? If so, that seems bad.

do_rt_sigqueueinfo() seems to have the same issue, from a glance - but
there, at least the error case is just a -EPERM, not a silent behavior
difference.

Would it make sense to refuse sending siginfo with SI_USER to
non-current? If you actually want to send a normal SI_USER signal, you

Yeah.

quoted

can use info==NULL, right? That should create wrongness parity with
do_rt_sigqueueinfo().

So you'd just do (just doing it non-elegantly rn):
if ((task_pid(current) != pid) &&
    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) {
        ret = -EPERM;
        goto err;
}

quoted

To improve things further, I guess you'd have to move the comparison
against current into pidfd_send_signal_specific(), or move the task
lookup out of it, or something like that?

Looks like a sane suggestion to me. Would you care to send a patch for
that? This is clearly a bugfix suitable for 5.1 so I'd rather not wait
until 5.2.