From: Christian Brauner <christian@brauner.io> To: Andy Lutomirski <luto@kernel.org> Cc: "Eric W. Biederman" <ebiederm@xmission.com>, LKML <linux-kernel@vger.kernel.org>, "Serge E. Hallyn" <serge@hallyn.com>, Jann Horn <jannh@google.com>, Andrew Morton <akpm@linux-foundation.org>, Oleg Nesterov <oleg@redhat.com>, Aleksa Sarai <cyphar@cyphar.com>, Al Viro <viro@zeniv.linux.org.uk>, Linux FS Devel <linux-fsdevel@vger.kernel.org>, Linux API <linux-api@vger.kernel.org>, Daniel Colascione <dancol@google.com>, Tim Murray <timmurray@google.com>, linux-man <linux-man@vger.kernel.org>, Kees Cook <keescook@chromium.org> Subject: Re: [PATCH v1 2/2] signal: add procfd_signal() syscall Date: Mon, 19 Nov 2018 19:39:36 +0100 [thread overview] Message-ID: <20181119183931.tkz7hfruw2ekqh62@brauner.io> (raw) In-Reply-To: <CALCETrUY=Hk6=BjgPuDBgj5J1T_b5Q5u1uVOWHjGWXwmHoZBEQ@mail.gmail.com> On Mon, Nov 19, 2018 at 07:45:04AM -0800, Andy Lutomirski wrote: > On Mon, Nov 19, 2018 at 2:33 AM Christian Brauner <christian@brauner.io> wrote: > > > > The kill() syscall operates on process identifiers. After a process has > > exited its pid can be reused by another process. If a caller sends a signal > > to a reused pid it will end up signaling the wrong process. This issue has > > often surfaced and there has been a push [1] to address this problem. > > > > A prior patch has introduced the ability to get a file descriptor > > referencing struct pid by opening /proc/<pid>. This guarantees a stable > > handle on a process which can be used to send signals to the referenced > > process. Discussion has shown that a dedicated syscall is preferable over > > ioctl()s. Thus, the new syscall procfd_signal() is introduced to solve > > this problem. It operates on a process file descriptor. > > The syscall takes an additional siginfo_t and flags argument. If siginfo_t > > is NULL then procfd_signal() behaves like kill() if it is not NULL it > > behaves like rt_sigqueueinfo. > > The flags argument is added to allow for future extensions of this syscall. > > It currently needs to be passed as 0. > > A few questions. First: you've made this work on /proc/PID, but > should it also work on /proc/PID/task/TID to send signals to a > specific thread? Yeah, so I thought about that. Your point being to combine: kill(), tgkill() aka rt_sigqueueinfo() and rt_tg_sigqueueinfo(). If I understand this correctly the implication is to also get file descriptors to /proc/PID/task/TID and pass them to procfd_signal()? Can we hold of on that one? Adding this in the future should be easily doable by simply getting /proc/PID/task/TID file descriptors but I would like this patchset to be as small as possible. > > > +bool proc_is_procfd(const struct file *file) > > +{ > > + return d_is_dir(file->f_path.dentry) && > > + (file->f_op == &proc_tgid_base_operations); > > +} > > Maybe rename to proc_is_tgid_procfd() to leave room for proc_is_tid_procfd()? Yes, good idea! > > > + if (info) { > > + ret = __copy_siginfo_from_user(sig, &kinfo, info); > > + if (unlikely(ret)) > > + goto err; > > + /* > > + * Not even root can pretend to send signals from the kernel. > > + * Nor can they impersonate a kill()/tgkill(), which adds > > + * source info. > > + */ > > + ret = -EPERM; > > + if ((kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL) && > > + (task_pid(current) != pid)) > > + goto err; > > Is the exception for signaling yourself actually useful here? I tried to strictly follow the sigqueue-based permission checks. I'm not comfortable removing this check without signal-experts telling me that it is safe to do.
next prev parent reply other threads:[~2018-11-19 18:39 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-19 10:32 [PATCH v1 0/2] proc: allow signaling processes via file descriptors Christian Brauner 2018-11-19 10:32 ` [PATCH v1 1/2] proc: get process file descriptor from /proc/<pid> Christian Brauner 2018-11-19 15:32 ` Andy Lutomirski 2018-11-19 18:20 ` Christian Brauner 2018-11-19 10:32 ` [PATCH v1 2/2] signal: add procfd_signal() syscall Christian Brauner 2018-11-19 15:45 ` Andy Lutomirski 2018-11-19 15:57 ` Daniel Colascione 2018-11-19 18:39 ` Christian Brauner [this message] 2018-11-19 15:59 ` Daniel Colascione 2018-11-19 18:29 ` Christian Brauner 2018-11-19 19:02 ` Eric W. Biederman 2018-11-19 19:31 ` Christian Brauner 2018-11-19 19:39 ` Daniel Colascione 2018-11-19 17:10 ` Eugene Syromiatnikov 2018-11-19 18:23 ` Christian Brauner 2018-11-19 17:14 ` Eugene Syromiatnikov 2018-11-19 20:28 ` Aleksa Sarai 2018-11-19 20:55 ` Christian Brauner 2018-11-19 21:13 ` Christian Brauner 2018-11-19 21:18 ` Aleksa Sarai 2018-11-19 21:20 ` Christian Brauner 2018-11-19 21:21 ` Christian Brauner 2018-11-19 21:25 ` Aleksa Sarai 2018-11-19 21:26 ` Daniel Colascione 2018-11-19 21:36 ` Aleksa Sarai 2018-11-19 21:37 ` Christian Brauner 2018-11-19 21:41 ` Daniel Colascione 2018-11-20 4:59 ` Eric W. Biederman 2018-11-20 10:31 ` Christian Brauner 2018-11-21 21:39 ` Serge E. Hallyn 2018-11-19 21:23 ` Aleksa Sarai 2018-11-22 7:41 ` Serge E. Hallyn 2018-11-19 22:39 ` Tycho Andersen 2018-11-19 22:49 ` Daniel Colascione 2018-11-19 23:07 ` Tycho Andersen 2018-11-20 0:27 ` Andy Lutomirski 2018-11-20 0:32 ` Christian Brauner 2018-11-20 0:34 ` Andy Lutomirski 2018-11-20 0:49 ` Daniel Colascione 2018-11-22 7:48 ` Serge E. Hallyn 2018-11-19 23:35 ` kbuild test robot 2018-11-19 23:37 ` kbuild test robot 2018-11-19 23:45 ` Christian Brauner 2018-11-28 21:45 ` Joey Pabalinas 2018-11-28 22:05 ` Christian Brauner 2018-11-28 23:02 ` Joey Pabalinas 2018-11-19 10:32 ` [PATCH] procfd_signal.2: document procfd_signal syscall Christian Brauner 2018-11-20 13:29 ` Michael Kerrisk (man-pages) 2018-11-28 20:59 ` Florian Weimer 2018-11-28 21:12 ` Christian Brauner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20181119183931.tkz7hfruw2ekqh62@brauner.io \ --to=christian@brauner.io \ --cc=akpm@linux-foundation.org \ --cc=cyphar@cyphar.com \ --cc=dancol@google.com \ --cc=ebiederm@xmission.com \ --cc=jannh@google.com \ --cc=keescook@chromium.org \ --cc=linux-api@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-man@vger.kernel.org \ --cc=luto@kernel.org \ --cc=oleg@redhat.com \ --cc=serge@hallyn.com \ --cc=timmurray@google.com \ --cc=viro@zeniv.linux.org.uk \ --subject='Re: [PATCH v1 2/2] signal: add procfd_signal() syscall' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).