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

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

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

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

+   /* 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

+   /* 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?

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>

-- 
Kees Cook