Re: [PATCH v3] signal: add procfd_send_signal() syscall

On Wed, Dec 05, 2018 at 03:24:08PM -0800, Kees Cook wrote:

On Wed, Dec 5, 2018 at 12:53 PM Christian Brauner [off-list ref] wrote:

quoted

On Wed, Dec 05, 2018 at 12:20:43PM -0600, Eric W. Biederman wrote:

quoted

Christian Brauner [off-list ref] writes:

quoted

[1]:  https://lkml.org/lkml/2018/11/18/130
[2]:  https://lore.kernel.org/lkml/874lbtjvtd.fsf@oldenburg2.str.redhat.com/ (local)
[3]:  https://lore.kernel.org/lkml/20181204132604.aspfupwjgjx6fhva@brauner.io/ (local)
[4]:  https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/ (local)
[5]:  https://lore.kernel.org/lkml/20181121213946.GA10795@mail.hallyn.com/ (local)
[6]:  https://lore.kernel.org/lkml/20181120103111.etlqp7zop34v6nv4@brauner.io/ (local)
[7]:  https://lore.kernel.org/lkml/36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net/ (local)
[8]:  https://lore.kernel.org/lkml/87tvjxp8pc.fsf@xmission.com/ (local)
[9]:  https://asciinema.org/a/X1J8eGhe3vCfBE2b9TXtTaSJ7
[10]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/ (local)
[11]: https://lore.kernel.org/lkml/F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net/ (local)

I nominate this for 2018's most-well-documented syscall commit log award. ;)

Hahaha. If I win can I get my price in beer(s)? :)

quoted

quoted

quoted

+   /*
+    * Give userspace a way to detect whether /proc/<pid>/task/<tid> fds
+    * are supported.
+    */
+   ret = -EOPNOTSUPP;
+   if (proc_is_tid_procfd(f.file))
+           goto err;

      -EBADF is the proper error code.

This is done so that userspace has a way of figuring out that tid fds
are not yet supported. This has been discussed with Florian (see commit
message).

Right, we should keep this -EOPNOTSUPP.

quoted

quoted

quoted

+   /* Is this a procfd? */
+   ret = -EINVAL;
+   if (!proc_is_tgid_procfd(f.file))
+           goto err;

      -EBADF is the proper error code.

Yeah, EINVAL tends to be used for bad flags... this is more about an
improper fd.

quoted

quoted

quoted

+   /* Without CONFIG_PROC_FS proc_pid() returns NULL. */
+   pid = proc_pid(file_inode(f.file));
+   if (!pid)
+           goto err;

Perhaps you want to fold the proc_pid into the proc_is_tgid_procfd
call.  That way proc_pid can stay private to proc.

Hm, I guess we can do that for now. My intention was to have reuseable
helpers but I guess it would be fine for now.

quoted

quoted

+   if (!may_signal_procfd(pid))
+           goto err;
+

Does the ns parent checking in may_signal_procfd need any locking or
RCU? I know pid and current namespaces are "pinned", but I don't know
how parent ns works here. I'm assuming the parents are stuck until all
children go away?

Yeah, since they are hierarchical killing an ancestor means killing the
children. Also, in case you're interested, there's precedent for that:
kernel/pid_namespace.c:static struct ns_common *pidns_get_parent(struct ns_common *ns)
I'm not using this function because a) I would have to special case the
initial test-case and b) it takes a get() on the pid ns which would
force us to use another put which is unnecessary.

quoted

quoted

quoted

+   ret = kill_pid_info(sig, &kinfo, pid);

Just double-checking for myself: this does not bypass
security_task_kill(), so no problem there AFAIK.

Reviewed-by: Kees Cook <redacted>

Thanks! :)
As a sidenote I'm switching the name from procfd_send_signal() to
taskfd_send_signal(). It seems to me the best way to handle Eric's
request to reflect that we can eventually both signal tgids and tids.

-- 
Kees Cook