* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-19 21:21 UTC (permalink / raw)
To: Aleksa Sarai
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, viro,
linux-fsdevel, linux-api, dancol, timmurray, linux-man, Kees Cook
In-Reply-To: <20181119211810.73ptfhnwdmkngfi4@yavin>
On Tue, Nov 20, 2018 at 08:18:10AM +1100, Aleksa Sarai wrote:
> On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> > On Tue, Nov 20, 2018 at 07:28:57AM +1100, Aleksa Sarai wrote:
> > > On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> > > > + 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;
> > > > + } else {
> > > > + prepare_kill_siginfo(sig, &kinfo);
> > > > + }
> > >
> > > I wonder whether we should also have a pidns restriction here, since
> > > currently it isn't possible for a container process using a pidns to
> > > signal processes outside its pidns. AFAICS, this isn't done through an
> > > explicit check -- it's a side-effect of processes in a pidns not being
> > > able to address non-descendant-pidns processes.
> > >
> > > But maybe it's reasonable to allow sending a procfd to a different pidns
> > > and the same operations working on it? If we extend the procfd API to
> >
> > No, I don't think so. I really don't want any fancy semantics in here.
> > Fancy doesn't get merged and fancy is hard to maintain. So we should do
> > something like:
> >
> > if (proc_pid_ns() != current_pid_ns)
> > return EINVAL
>
> This isn't quite sufficient. The key thing is that you have to be in an
> *ancestor* (or same) pidns, not the *same* pidns. Ideally you can re-use
> the check already in pidns_get_parent, and expose it. It would be
> something as trivial as:
>
> bool pidns_is_descendant(struct pid_namespace *ns,
> struct pid_namespace *ancestor)
> {
> for (;;) {
> if (!ns)
> return false;
> if (ns == ancestor)
> break;
> ns = ns->parent;
> }
> return true;
> }
That can be done without a loop by comparing the level counter for the
two pid namespaces.
>
> And you can rewrite pidns_get_parent to use it. So you would instead be
> doing:
>
> if (pidns_is_descendant(proc_pid_ns, task_active_pid_ns(current)))
> return -EPERM;
>
> (Or you can just copy the 5-line loop into procfd_signal -- though I
> imagine we'll need this for all of the procfd_* APIs.)
>
> --
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-19 21:20 UTC (permalink / raw)
To: Aleksa Sarai
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, viro,
linux-fsdevel, linux-api, dancol, timmurray, linux-man, Kees Cook
In-Reply-To: <20181119211810.73ptfhnwdmkngfi4@yavin>
On Tue, Nov 20, 2018 at 08:18:10AM +1100, Aleksa Sarai wrote:
> On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> > On Tue, Nov 20, 2018 at 07:28:57AM +1100, Aleksa Sarai wrote:
> > > On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> > > > + 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;
> > > > + } else {
> > > > + prepare_kill_siginfo(sig, &kinfo);
> > > > + }
> > >
> > > I wonder whether we should also have a pidns restriction here, since
> > > currently it isn't possible for a container process using a pidns to
> > > signal processes outside its pidns. AFAICS, this isn't done through an
> > > explicit check -- it's a side-effect of processes in a pidns not being
> > > able to address non-descendant-pidns processes.
> > >
> > > But maybe it's reasonable to allow sending a procfd to a different pidns
> > > and the same operations working on it? If we extend the procfd API to
> >
> > No, I don't think so. I really don't want any fancy semantics in here.
> > Fancy doesn't get merged and fancy is hard to maintain. So we should do
> > something like:
> >
> > if (proc_pid_ns() != current_pid_ns)
> > return EINVAL
>
> This isn't quite sufficient. The key thing is that you have to be in an
> *ancestor* (or same) pidns, not the *same* pidns. Ideally you can re-use
See my next mail.
> the check already in pidns_get_parent, and expose it. It would be
> something as trivial as:
>
> bool pidns_is_descendant(struct pid_namespace *ns,
> struct pid_namespace *ancestor)
> {
> for (;;) {
> if (!ns)
> return false;
> if (ns == ancestor)
> break;
> ns = ns->parent;
> }
> return true;
> }
>
> And you can rewrite pidns_get_parent to use it. So you would instead be
> doing:
>
> if (pidns_is_descendant(proc_pid_ns, task_active_pid_ns(current)))
> return -EPERM;
>
> (Or you can just copy the 5-line loop into procfd_signal -- though I
> imagine we'll need this for all of the procfd_* APIs.)
>
> --
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Aleksa Sarai @ 2018-11-19 21:18 UTC (permalink / raw)
To: Christian Brauner
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, viro,
linux-fsdevel, linux-api, dancol, timmurray, linux-man, Kees Cook
In-Reply-To: <20181119205518.btew3vxwgva4w3zh@brauner.io>
[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]
On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> On Tue, Nov 20, 2018 at 07:28:57AM +1100, Aleksa Sarai wrote:
> > On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> > > + 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;
> > > + } else {
> > > + prepare_kill_siginfo(sig, &kinfo);
> > > + }
> >
> > I wonder whether we should also have a pidns restriction here, since
> > currently it isn't possible for a container process using a pidns to
> > signal processes outside its pidns. AFAICS, this isn't done through an
> > explicit check -- it's a side-effect of processes in a pidns not being
> > able to address non-descendant-pidns processes.
> >
> > But maybe it's reasonable to allow sending a procfd to a different pidns
> > and the same operations working on it? If we extend the procfd API to
>
> No, I don't think so. I really don't want any fancy semantics in here.
> Fancy doesn't get merged and fancy is hard to maintain. So we should do
> something like:
>
> if (proc_pid_ns() != current_pid_ns)
> return EINVAL
This isn't quite sufficient. The key thing is that you have to be in an
*ancestor* (or same) pidns, not the *same* pidns. Ideally you can re-use
the check already in pidns_get_parent, and expose it. It would be
something as trivial as:
bool pidns_is_descendant(struct pid_namespace *ns,
struct pid_namespace *ancestor)
{
for (;;) {
if (!ns)
return false;
if (ns == ancestor)
break;
ns = ns->parent;
}
return true;
}
And you can rewrite pidns_get_parent to use it. So you would instead be
doing:
if (pidns_is_descendant(proc_pid_ns, task_active_pid_ns(current)))
return -EPERM;
(Or you can just copy the 5-line loop into procfd_signal -- though I
imagine we'll need this for all of the procfd_* APIs.)
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-19 21:13 UTC (permalink / raw)
To: Aleksa Sarai
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, viro,
linux-fsdevel, linux-api, dancol, timmurray, linux-man, Kees Cook
In-Reply-To: <20181119205518.btew3vxwgva4w3zh@brauner.io>
On Mon, Nov 19, 2018 at 09:55:18PM +0100, Christian Brauner wrote:
> On Tue, Nov 20, 2018 at 07:28:57AM +1100, Aleksa Sarai wrote:
> > On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> > > + 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;
> > > + } else {
> > > + prepare_kill_siginfo(sig, &kinfo);
> > > + }
> >
> > I wonder whether we should also have a pidns restriction here, since
> > currently it isn't possible for a container process using a pidns to
> > signal processes outside its pidns. AFAICS, this isn't done through an
> > explicit check -- it's a side-effect of processes in a pidns not being
> > able to address non-descendant-pidns processes.
> >
> > But maybe it's reasonable to allow sending a procfd to a different pidns
> > and the same operations working on it? If we extend the procfd API to
>
> No, I don't think so. I really don't want any fancy semantics in here.
> Fancy doesn't get merged and fancy is hard to maintain. So we should do
> something like:
>
> if (proc_pid_ns() != current_pid_ns)
> return EINVAL
To be more precise, we need to detect if fd refers to an ancestor pidns
and if so return EINVAL.
>
> > allow process creation this would allow a container to create a process
> > outside its pidns.
> >
> > --
> > Aleksa Sarai
> > Senior Software Engineer (Containers)
> > SUSE Linux GmbH
> > <https://www.cyphar.com/>
>
>
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-19 20:55 UTC (permalink / raw)
To: Aleksa Sarai
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, viro,
linux-fsdevel, linux-api, dancol, timmurray, linux-man, Kees Cook
In-Reply-To: <20181119202857.k5zw742xjfrw677j@yavin>
On Tue, Nov 20, 2018 at 07:28:57AM +1100, Aleksa Sarai wrote:
> On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> > + 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;
> > + } else {
> > + prepare_kill_siginfo(sig, &kinfo);
> > + }
>
> I wonder whether we should also have a pidns restriction here, since
> currently it isn't possible for a container process using a pidns to
> signal processes outside its pidns. AFAICS, this isn't done through an
> explicit check -- it's a side-effect of processes in a pidns not being
> able to address non-descendant-pidns processes.
>
> But maybe it's reasonable to allow sending a procfd to a different pidns
> and the same operations working on it? If we extend the procfd API to
No, I don't think so. I really don't want any fancy semantics in here.
Fancy doesn't get merged and fancy is hard to maintain. So we should do
something like:
if (proc_pid_ns() != current_pid_ns)
return EINVAL
> allow process creation this would allow a container to create a process
> outside its pidns.
>
> --
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Aleksa Sarai @ 2018-11-19 20:28 UTC (permalink / raw)
To: Christian Brauner
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, viro,
linux-fsdevel, linux-api, dancol, timmurray, linux-man, Kees Cook
In-Reply-To: <20181119103241.5229-3-christian@brauner.io>
[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]
On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> + 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;
> + } else {
> + prepare_kill_siginfo(sig, &kinfo);
> + }
I wonder whether we should also have a pidns restriction here, since
currently it isn't possible for a container process using a pidns to
signal processes outside its pidns. AFAICS, this isn't done through an
explicit check -- it's a side-effect of processes in a pidns not being
able to address non-descendant-pidns processes.
But maybe it's reasonable to allow sending a procfd to a different pidns
and the same operations working on it? If we extend the procfd API to
allow process creation this would allow a container to create a process
outside its pidns.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Aleksa Sarai @ 2018-11-19 20:21 UTC (permalink / raw)
To: Daniel Colascione
Cc: Dmitry Safonov, Andy Lutomirski, Randy Dunlap, Christian Brauner,
Eric W. Biederman, open list, Serge Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Al Viro, Linux FS Devel, Linux API,
Tim Murray, Kees Cook, Jan Engelhardt, Andrei Vagin
In-Reply-To: <CAKOZueu78Urbc-Vt--ZntGAG_i-LrNsPjGmRi7F8zuTUQ=iHug@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]
On 2018-11-19, Daniel Colascione <dancol@google.com> wrote:
> > I wonder how fast it would be holding a pid with another open()ed fd.
> > And then you need to read comm (or how you filter whom to kill).
> > It seems to me that procfs will be even slower with this safe-way.
> > But I might misunderstand the idea, excuses.
> >
> > So, I just wanted to gently remind about procfs with netlink socket[1].
>
> We discussed netlink was extensively on the thread about
> /proc/pid/kill. For numerous reasons, it's not suitable for
> fundamental process management. We really need an FD-based interface
> to processes, just like we have FD-based interfaces to other resource
> types. We need something consistent and reliable, not an abuse of a
> monitoring interface.
Another significant problem with using netlink for something like this
is that (as its name suggest) it's tied to network namespaces and not
pid namespaces so you wouldn't reasonably be able to use the API inside
a container. Using an fd side-steps the problem somewhat (though this
just gave me an idea -- I will add it to the other thread).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Daniel Colascione @ 2018-11-19 19:39 UTC (permalink / raw)
To: Christian Brauner
Cc: Eric W. Biederman, linux-kernel, Serge E. Hallyn, Jann Horn,
Andy Lutomirski, Andrew Morton, Oleg Nesterov, Aleksa Sarai,
Al Viro, Linux FS Devel, Linux API, Tim Murray, linux-man,
Kees Cook
In-Reply-To: <20181119193128.hv7z4j52ajrue2jr@brauner.io>
Yep. That's also what I was talking about, FWIW.
On Mon, Nov 19, 2018 at 11:31 AM, Christian Brauner
<christian@brauner.io> wrote:
> On Mon, Nov 19, 2018 at 01:02:06PM -0600, Eric W. Biederman wrote:
>> Christian Brauner <christian@brauner.io> writes:
>>
>> > On Mon, Nov 19, 2018 at 07:59:24AM -0800, Daniel Colascione wrote:
>> >> You never addressed my comment on the previous patch about your use of
>> >
>> > Sorry, that thread exploded so quickly that I might have missed it.
>> >
>> >> private_data here. Why can't you use the struct pid reference that's
>> >> already in the inode?
>> >
>> > If that's what people prefer we can probably use that. There was
>> > precedent for stashing away such data in fs/proc/base.c already for
>> > various other things including user namespaces and struct mm so I
>> > followed this model. A prior version of my patch (I didn't send out) did
>> > retrive the inode via proc_pid() in .open() took an additional reference
>> > via get_pid() and dropped it in .release(). Do we prefer that?
>>
>> If you are using proc/<pid>/ directories as your file descriptors, you
>> don't need to add an open or a release method at all. The existing file
>> descriptors hold a reference to the inode which holds a reference the
>> the struct pid.
>>
>> The only time you need to get a reference is when you need a task
>> and kill_pid_info already performs that work for you.
>
> Oh, I see what you and Andy are saying now. Sweet, that means we can
> trim down the patch even more. Less code, less headache.
>
> Thanks!
>
>>
>> So using proc_pid is all you need to do to get the pid from the existing
>> file descriptors.
>>
>> Eric
>>
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-19 19:31 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Daniel Colascione, linux-kernel, Serge E. Hallyn, Jann Horn,
Andy Lutomirski, Andrew Morton, Oleg Nesterov, Aleksa Sarai,
Al Viro, Linux FS Devel, Linux API, Tim Murray, linux-man,
Kees Cook
In-Reply-To: <87o9ak28nl.fsf@xmission.com>
On Mon, Nov 19, 2018 at 01:02:06PM -0600, Eric W. Biederman wrote:
> Christian Brauner <christian@brauner.io> writes:
>
> > On Mon, Nov 19, 2018 at 07:59:24AM -0800, Daniel Colascione wrote:
> >> You never addressed my comment on the previous patch about your use of
> >
> > Sorry, that thread exploded so quickly that I might have missed it.
> >
> >> private_data here. Why can't you use the struct pid reference that's
> >> already in the inode?
> >
> > If that's what people prefer we can probably use that. There was
> > precedent for stashing away such data in fs/proc/base.c already for
> > various other things including user namespaces and struct mm so I
> > followed this model. A prior version of my patch (I didn't send out) did
> > retrive the inode via proc_pid() in .open() took an additional reference
> > via get_pid() and dropped it in .release(). Do we prefer that?
>
> If you are using proc/<pid>/ directories as your file descriptors, you
> don't need to add an open or a release method at all. The existing file
> descriptors hold a reference to the inode which holds a reference the
> the struct pid.
>
> The only time you need to get a reference is when you need a task
> and kill_pid_info already performs that work for you.
Oh, I see what you and Andy are saying now. Sweet, that means we can
trim down the patch even more. Less code, less headache.
Thanks!
>
> So using proc_pid is all you need to do to get the pid from the existing
> file descriptors.
>
> Eric
>
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Eric W. Biederman @ 2018-11-19 19:02 UTC (permalink / raw)
To: Christian Brauner
Cc: Daniel Colascione, linux-kernel, Serge E. Hallyn, Jann Horn,
Andy Lutomirski, Andrew Morton, Oleg Nesterov, Aleksa Sarai,
Al Viro, Linux FS Devel, Linux API, Tim Murray, linux-man,
Kees Cook
In-Reply-To: <20181119182902.fadw6qiu3eepndm3@brauner.io>
Christian Brauner <christian@brauner.io> writes:
> On Mon, Nov 19, 2018 at 07:59:24AM -0800, Daniel Colascione wrote:
>> You never addressed my comment on the previous patch about your use of
>
> Sorry, that thread exploded so quickly that I might have missed it.
>
>> private_data here. Why can't you use the struct pid reference that's
>> already in the inode?
>
> If that's what people prefer we can probably use that. There was
> precedent for stashing away such data in fs/proc/base.c already for
> various other things including user namespaces and struct mm so I
> followed this model. A prior version of my patch (I didn't send out) did
> retrive the inode via proc_pid() in .open() took an additional reference
> via get_pid() and dropped it in .release(). Do we prefer that?
If you are using proc/<pid>/ directories as your file descriptors, you
don't need to add an open or a release method at all. The existing file
descriptors hold a reference to the inode which holds a reference the
the struct pid.
The only time you need to get a reference is when you need a task
and kill_pid_info already performs that work for you.
So using proc_pid is all you need to do to get the pid from the existing
file descriptors.
Eric
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-19 18:39 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Daniel Colascione, Tim Murray,
linux-man, Kees Cook
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.
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-19 18:29 UTC (permalink / raw)
To: Daniel Colascione
Cc: Eric W. Biederman, linux-kernel, Serge E. Hallyn, Jann Horn,
Andy Lutomirski, Andrew Morton, Oleg Nesterov, Aleksa Sarai,
Al Viro, Linux FS Devel, Linux API, Tim Murray, linux-man,
Kees Cook
In-Reply-To: <CAKOZueshQ2DAwUYp+dLpmfYG6FtvCqeRQxH41SjNS+WqY3e0Fw@mail.gmail.com>
On Mon, Nov 19, 2018 at 07:59:24AM -0800, Daniel Colascione wrote:
> On Mon, Nov 19, 2018 at 2:32 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.
> >
> > With this patch a process can be killed via:
> >
> > #define _GNU_SOURCE
> > #include <errno.h>
> > #include <fcntl.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> > #include <signal.h>
> > #include <sys/stat.h>
> > #include <sys/syscall.h>
> > #include <sys/types.h>
> > #include <unistd.h>
> >
> > int main(int argc, char *argv[])
> > {
> > int ret;
> > char buf[1000];
> >
> > if (argc < 2)
> > exit(EXIT_FAILURE);
> >
> > ret = snprintf(buf, sizeof(buf), "/proc/%s", argv[1]);
> > if (ret < 0)
> > exit(EXIT_FAILURE);
> >
> > int fd = open(buf, O_DIRECTORY | O_CLOEXEC);
> > if (fd < 0) {
> > printf("%s - Failed to open \"%s\"\n", strerror(errno), buf);
> > exit(EXIT_FAILURE);
> > }
> >
> > ret = syscall(__NR_procfd_signal, fd, SIGKILL, NULL, 0);
> > if (ret < 0) {
> > printf("Failed to send SIGKILL \"%s\"\n", strerror(errno));
> > close(fd);
> > exit(EXIT_FAILURE);
> > }
> >
> > close(fd);
> >
> > exit(EXIT_SUCCESS);
> > }
> >
> > [1]: https://lkml.org/lkml/2018/11/18/130
> >
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > Cc: Serge Hallyn <serge@hallyn.com>
> > Cc: Jann Horn <jannh@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Andy Lutomirsky <luto@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> > Changelog:
> > v1:
> > - patch introduced
> > ---
> > arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> > arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> > fs/proc/base.c | 6 ++
> > include/linux/proc_fs.h | 1 +
> > include/linux/syscalls.h | 2 +
> > kernel/signal.c | 76 ++++++++++++++++++++++++--
> > 6 files changed, 81 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> > index 3cf7b533b3d1..e637eab883e9 100644
> > --- a/arch/x86/entry/syscalls/syscall_32.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> > @@ -398,3 +398,4 @@
> > 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> > 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> > 386 i386 rseq sys_rseq __ia32_sys_rseq
> > +387 i386 procfd_signal sys_procfd_signal __ia32_sys_procfd_signal
> > diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> > index f0b1709a5ffb..e95f6741ab42 100644
> > --- a/arch/x86/entry/syscalls/syscall_64.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> > @@ -343,6 +343,7 @@
> > 332 common statx __x64_sys_statx
> > 333 common io_pgetevents __x64_sys_io_pgetevents
> > 334 common rseq __x64_sys_rseq
> > +335 common procfd_signal __x64_sys_procfd_signal
> >
> > #
> > # x32-specific system call numbers start at 512 to avoid cache impact
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 6365a4fea314..a12c9de92bd0 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -3055,6 +3055,12 @@ static const struct file_operations proc_tgid_base_operations = {
> > .release = proc_tgid_release,
> > };
> >
> > +bool proc_is_procfd(const struct file *file)
> > +{
> > + return d_is_dir(file->f_path.dentry) &&
> > + (file->f_op == &proc_tgid_base_operations);
> > +}
> > +
> > static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> > {
> > return proc_pident_lookup(dir, dentry,
> > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > index d0e1f1522a78..2d53a47fba34 100644
> > --- a/include/linux/proc_fs.h
> > +++ b/include/linux/proc_fs.h
> > @@ -73,6 +73,7 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
> > int (*show)(struct seq_file *, void *),
> > proc_write_t write,
> > void *data);
> > +extern bool proc_is_procfd(const struct file *file);
> >
> > #else /* CONFIG_PROC_FS */
> >
> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index 2ac3d13a915b..a5ca8cb84566 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -907,6 +907,8 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
> > unsigned mask, struct statx __user *buffer);
> > asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
> > int flags, uint32_t sig);
> > +asmlinkage long sys_procfd_signal(int fd, int sig, siginfo_t __user *info,
> > + int flags);
> >
> > /*
> > * Architecture-specific system calls
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index 9a32bc2088c9..e8a8929de710 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -19,7 +19,9 @@
> > #include <linux/sched/task.h>
> > #include <linux/sched/task_stack.h>
> > #include <linux/sched/cputime.h>
> > +#include <linux/file.h>
> > #include <linux/fs.h>
> > +#include <linux/proc_fs.h>
> > #include <linux/tty.h>
> > #include <linux/binfmts.h>
> > #include <linux/coredump.h>
> > @@ -3286,6 +3288,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
> > }
> > #endif
> >
> > +static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> > +{
> > + clear_siginfo(info);
> > + info->si_signo = sig;
> > + info->si_errno = 0;
> > + info->si_code = SI_USER;
> > + info->si_pid = task_tgid_vnr(current);
> > + info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> > +}
> > +
> > /**
> > * sys_kill - send a signal to a process
> > * @pid: the PID of the process
> > @@ -3295,16 +3307,68 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> > {
> > struct kernel_siginfo info;
> >
> > - clear_siginfo(&info);
> > - info.si_signo = sig;
> > - info.si_errno = 0;
> > - info.si_code = SI_USER;
> > - info.si_pid = task_tgid_vnr(current);
> > - info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> > + prepare_kill_siginfo(sig, &info);
> >
> > return kill_something_info(sig, &info, pid);
> > }
> >
> > +/**
> > + * sys_procfd_signal - send a signal to a process through a process file
> > + * descriptor
> > + * @fd: the file descriptor of the process
> > + * @sig: signal to be sent
> > + * @info: the signal info
> > + * @flags: future flags to be passed
> > + */
> > +SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info,
> > + int, flags)
> > +{
> > + int ret;
> > + struct pid *pid;
> > + kernel_siginfo_t kinfo;
> > + struct fd f;
> > +
> > + /* Enforce flags be set to 0 until we add an extension. */
> > + if (flags)
> > + return -EINVAL;
> > +
> > + f = fdget_raw(fd);
> > + if (!f.file)
> > + return -EBADF;
> > +
> > + ret = -EINVAL;
> > + /* Is this a process file descriptor? */
> > + if (!proc_is_procfd(f.file))
> > + goto err;
> > +
> > + pid = f.file->private_data;
>
> You never addressed my comment on the previous patch about your use of
Sorry, that thread exploded so quickly that I might have missed it.
> private_data here. Why can't you use the struct pid reference that's
> already in the inode?
If that's what people prefer we can probably use that. There was
precedent for stashing away such data in fs/proc/base.c already for
various other things including user namespaces and struct mm so I
followed this model. A prior version of my patch (I didn't send out) did
retrive the inode via proc_pid() in .open() took an additional reference
via get_pid() and dropped it in .release(). Do we prefer that?
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-19 18:23 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man,
Kees Cook
In-Reply-To: <20181119171053.GA28067@asgard.redhat.com>
On Mon, Nov 19, 2018 at 06:10:53PM +0100, Eugene Syromiatnikov wrote:
> On Mon, Nov 19, 2018 at 11:32:39AM +0100, Christian Brauner wrote:
> > diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> > index 3cf7b533b3d1..e637eab883e9 100644
> > --- a/arch/x86/entry/syscalls/syscall_32.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> > @@ -398,3 +398,4 @@
> > 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> > 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> > 386 i386 rseq sys_rseq __ia32_sys_rseq
> > +387 i386 procfd_signal sys_procfd_signal __ia32_sys_procfd_signal
> > diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> > index f0b1709a5ffb..e95f6741ab42 100644
> > --- a/arch/x86/entry/syscalls/syscall_64.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> > @@ -343,6 +343,7 @@
> > 332 common statx __x64_sys_statx
> > 333 common io_pgetevents __x64_sys_io_pgetevents
> > 334 common rseq __x64_sys_rseq
> > +335 common procfd_signal __x64_sys_procfd_signal
>
> You have wired up the syscall on x86 but have not added the syscall number
> to the generic syscall header (include/uapi/asm-generic/unistd.h), why?
Oversight on my part, sorry.
^ permalink raw reply
* Re: [PATCH v1 1/2] proc: get process file descriptor from /proc/<pid>
From: Christian Brauner @ 2018-11-19 18:20 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Daniel Colascione, Tim Murray,
linux-man, Kees Cook
In-Reply-To: <CALCETrVvHu2dSJs1dpA-2ozGhZGxbGrnYbUuh98rkCdsz42rfg@mail.gmail.com>
On Mon, Nov 19, 2018 at 07:32:33AM -0800, Andy Lutomirski wrote:
> On Mon, Nov 19, 2018 at 2:33 AM Christian Brauner <christian@brauner.io> wrote:
> >
> > With this patch an open() call on /proc/<pid> will give userspace a handle
> > to struct pid of the process associated with /proc/<pid>. This allows to
> > maintain a stable handle on a process.
> > I have been discussing various approaches extensively during technical
> > conferences this year culminating in a long argument with Eric at Linux
> > Plumbers. The general consensus was that having a handle on a process
> > should be something that is very simple and easy to maintain with the
> > option of being extensible via a more advanced api if the need arises. I
> > believe that this patch is the most simple, dumb, and therefore
> > maintainable solution.
>
> How does the mechanism you're adding here differ from proc_pid()?
I'm sorry, I am missing the context around proc_pid()?. Are you talking
about the the proc_pid() function in "internal.h"? What exactly is the
proc_pid() mechanism?
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Eugene Syromiatnikov @ 2018-11-19 17:14 UTC (permalink / raw)
To: Christian Brauner
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man,
Kees Cook
In-Reply-To: <20181119103241.5229-3-christian@brauner.io>
On Mon, Nov 19, 2018 at 11:32:39AM +0100, Christian Brauner wrote:
> +/**
> + * sys_procfd_signal - send a signal to a process through a process file
> + * descriptor
> + * @fd: the file descriptor of the process
> + * @sig: signal to be sent
> + * @info: the signal info
> + * @flags: future flags to be passed
> + */
> +SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info,
> + int, flags)
It could be considered better to use an unsigned type for the "flags"
argument.
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Eugene Syromiatnikov @ 2018-11-19 17:10 UTC (permalink / raw)
To: Christian Brauner
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man,
Kees Cook
In-Reply-To: <20181119103241.5229-3-christian@brauner.io>
On Mon, Nov 19, 2018 at 11:32:39AM +0100, Christian Brauner wrote:
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 3cf7b533b3d1..e637eab883e9 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -398,3 +398,4 @@
> 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> 386 i386 rseq sys_rseq __ia32_sys_rseq
> +387 i386 procfd_signal sys_procfd_signal __ia32_sys_procfd_signal
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index f0b1709a5ffb..e95f6741ab42 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -343,6 +343,7 @@
> 332 common statx __x64_sys_statx
> 333 common io_pgetevents __x64_sys_io_pgetevents
> 334 common rseq __x64_sys_rseq
> +335 common procfd_signal __x64_sys_procfd_signal
You have wired up the syscall on x86 but have not added the syscall number
to the generic syscall header (include/uapi/asm-generic/unistd.h), why?
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Andy Lutomirski @ 2018-11-19 17:00 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andrew Lutomirski, Dave Hansen, Christopherson, Sean J,
Jethro Beekman, Florian Weimer, Linux API, Jann Horn,
Linus Torvalds, X86 ML, linux-arch, LKML, Peter Zijlstra,
Rich Felker, nhorman, npmccallum, Ayoun, Serge, shay.katz-zamir,
linux-sgx, Andy Shevchenko, Thomas Gleixner, Ingo Molnar
In-Reply-To: <20181119160204.GD13298@linux.intel.com>
On Mon, Nov 19, 2018 at 8:02 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, Nov 19, 2018 at 07:29:36AM -0800, Andy Lutomirski wrote:
> > 1. The kernel needs some way to know *when* to apply this fixup.
> > Decoding the instruction stream and doing it to all exceptions that
> > hit an ENCLU instruction seems like a poor design.
>
> I'm not sure why you would ever need to do any type of fixup as the idea
> is to just return to AEP i.e. from chosen exceptions (EPCM, #UD) the AEP
> would work the same way as for exceptions that the kernel can deal with
> except filling the exception information to registers.
Sure, but how does the kernel know when to do that and when to send a
signal? I don't really like decoding the instruction stream to figure
it out.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-19 16:27 UTC (permalink / raw)
To: Dmitry Safonov
Cc: Andy Lutomirski, Randy Dunlap, Christian Brauner,
Eric W. Biederman, open list, Serge Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook, Jan Engelhardt,
Andrei Vagin
In-Reply-To: <CAJwJo6aXQUtciO7kPSENoajV6JUqUGJxCh_0yPf5D8GfbLegzA@mail.gmail.com>
On Mon, Nov 19, 2018 at 8:13 AM, Dmitry Safonov <0x7f454c46@gmail.com> wrote:
> I wonder how fast it would be holding a pid with another open()ed fd.
> And then you need to read comm (or how you filter whom to kill).
> It seems to me that procfs will be even slower with this safe-way.
> But I might misunderstand the idea, excuses.
>
> So, I just wanted to gently remind about procfs with netlink socket[1].
We discussed netlink was extensively on the thread about
/proc/pid/kill. For numerous reasons, it's not suitable for
fundamental process management. We really need an FD-based interface
to processes, just like we have FD-based interfaces to other resource
types. We need something consistent and reliable, not an abuse of a
monitoring interface.
> Probably, if it's time to add a new API for procfs, netlink may be more
> desirable.
Definitely not.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors (Larger pids)
From: Eric W. Biederman @ 2018-11-19 16:26 UTC (permalink / raw)
To: Dmitry Safonov
Cc: Andy Lutomirski, dancol, rdunlap, christian, open list,
Serge Hallyn, jannh, Andrew Morton, Oleg Nesterov, cyphar,
Al Viro, linux-fsdevel, Linux API, timmurray, Kees Cook, jengelh,
Andrei Vagin
In-Reply-To: <CAJwJo6aXQUtciO7kPSENoajV6JUqUGJxCh_0yPf5D8GfbLegzA@mail.gmail.com>
Dmitry Safonov <0x7f454c46@gmail.com> writes:
>
> So, I just wanted to gently remind about procfs with netlink socket[1].
> It seems to me that whenever you receive() pid information, you
> can request some uniq 64(?) bit number and kill the process using it.
> Whenever uniqueness of 64-bit number to handle pids will be a question
> the netlink message might be painlessly extended to 128 or whatever.
No.
I have seen this requested twice in this thread now, and despite
understanding where everyone is coming from I do not believe it will
be wise to implement larger pids.
The problem is that we then have to make these larger pids live in
the pid namespace, make struct pid larger to hold them and update
CRIU to save and restore them.
All for a very small handful of processes that use this extended API.
Eric
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Dmitry Safonov @ 2018-11-19 16:13 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dancol, rdunlap, christian, Eric W. Biederman, open list,
Serge Hallyn, jannh, Andrew Morton, Oleg Nesterov, cyphar,
Al Viro, linux-fsdevel, Linux API, timmurray, Kees Cook, jengelh,
Andrei Vagin
In-Reply-To: <CALCETrVscRwQG55-j1SKc2TmSb1-=5861804ojUuviNzdyDOrA@mail.gmail.com>
On Sun, 18 Nov 2018 at 18:30, Andy Lutomirski <luto@kernel.org> wrote:
> Here's my point: if we're really going to make a new API to manipulate
> processes by their fd, I think we should have at least a decent idea
> of how that API will get extended in the future. Right now, we have
> an extremely awkward situation where opening an fd in /proc requires
> certain capabilities or uids, and using those fds often also checks
> current's capabilities, and the target process may have changed its
> own security context, including gaining privilege via SUID, SGID, or
> LSM transition rules in the mean time. This has been a huge source of
> security bugs. It would be nice to have a model for future APIs that
> avoids these problems.
>
> And I didn't say in my proposal that a process's identity should
> fundamentally change when it calls execve(). I'm suggesting that
> certain operations that could cause a process to gain privilege or
> otherwise require greater permission to introspect (mainly execve)
> could be handled by invalidating the new process management fds.
> Sure, if init re-execs itself, it's still PID 1, but that doesn't
> necessarily mean that:
>
> fd = process_open_management_fd(1);
> [init reexecs]
> process_do_something(fd);
>
> needs to work.
>
> >
> > > setresuid() has no effect
> > > here -- if you have W access to the process and the process calls
> > > setresuid(), you still have W access.
> >
> > Now you've created a situation in which an operation that security
> > policy previously blocked now becomes possible, invaliding previous
> > designs based on the old security invariant. That's the definition of
> > introducing a security hole.
>
> I think you're overstating your case. To a pretty good approximation,
> setresuid() allows the caller to remove elements from the set {ruid,
> suid, euid}, unless the caller has CAP_SETUID. If you could ptrace a
> process before it calls setresuid(), you might as well be able to
> ptrace() it after, since you could have just ptraced it and made it
> call setresuid() while still ptracing it. Similarly, it seems like
> it's probably safe to be able to open an fd that lets you watch the
> exit status of a process, have the process call setresuid(), and still
> see the exit status.
>
> Regardless of how you feel about these issues, if you're going to add
> an API by which you open an fd, wait for a process to exit, and read
> the exit status, you need to define the conditions under which you may
> open the fd and under which you may read the exit status once you have
> the fd. There are probably multiple valid answers, but the question
> still needs to be answered. My POLLERR hack, aside from being ugly,
> avoids this particular issue because it merely lets you wait for
> something you already could have observed using readdir().
Beg your pardon for hijacking the thread..
I wonder how fast it would be holding a pid with another open()ed fd.
And then you need to read comm (or how you filter whom to kill).
It seems to me that procfs will be even slower with this safe-way.
But I might misunderstand the idea, excuses.
So, I just wanted to gently remind about procfs with netlink socket[1].
It seems to me that whenever you receive() pid information, you
can request some uniq 64(?) bit number and kill the process using it.
Whenever uniqueness of 64-bit number to handle pids will be a question
the netlink message might be painlessly extended to 128 or whatever.
Also, it may provide the facilities to atomically kill process say by name
by adding another field to netlink message.
Probably, if it's time to add a new API for procfs, netlink may be more
desirable.
[1]: https://lwn.net/Articles/650243/
Thanks,
Dmitry
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Jarkko Sakkinen @ 2018-11-19 16:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Dave Hansen, Christopherson, Sean J, Jethro Beekman,
Florian Weimer, Linux API, Jann Horn, Linus Torvalds, X86 ML,
linux-arch, LKML, Peter Zijlstra, Rich Felker, nhorman,
npmccallum, Ayoun, Serge, shay.katz-zamir, linux-sgx,
Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov
In-Reply-To: <CALCETrXSpn7QGzPgO1BjMRQ75r2i0kmACrbUM+8PyT+xi3f3Hw@mail.gmail.com>
On Mon, Nov 19, 2018 at 07:29:36AM -0800, Andy Lutomirski wrote:
> 1. The kernel needs some way to know *when* to apply this fixup.
> Decoding the instruction stream and doing it to all exceptions that
> hit an ENCLU instruction seems like a poor design.
I'm not sure why you would ever need to do any type of fixup as the idea
is to just return to AEP i.e. from chosen exceptions (EPCM, #UD) the AEP
would work the same way as for exceptions that the kernel can deal with
except filling the exception information to registers.
> 2. It starts exposing what looks like a more generic exception
> handling mechanism to userspace, except that it's nonsensical for
> anything other than ENCLU.
Well, I see the user space and namely the run-time the host for the
enclave i.e. middle-man to provide services for emulating instructions
etc.
/Jarkko
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Daniel Colascione @ 2018-11-19 15:59 UTC (permalink / raw)
To: Christian Brauner
Cc: Eric W. Biederman, linux-kernel, Serge E. Hallyn, Jann Horn,
Andy Lutomirski, Andrew Morton, Oleg Nesterov, Aleksa Sarai,
Al Viro, Linux FS Devel, Linux API, Tim Murray, linux-man,
Kees Cook
In-Reply-To: <20181119103241.5229-3-christian@brauner.io>
On Mon, Nov 19, 2018 at 2:32 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.
>
> With this patch a process can be killed via:
>
> #define _GNU_SOURCE
> #include <errno.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <signal.h>
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <sys/types.h>
> #include <unistd.h>
>
> int main(int argc, char *argv[])
> {
> int ret;
> char buf[1000];
>
> if (argc < 2)
> exit(EXIT_FAILURE);
>
> ret = snprintf(buf, sizeof(buf), "/proc/%s", argv[1]);
> if (ret < 0)
> exit(EXIT_FAILURE);
>
> int fd = open(buf, O_DIRECTORY | O_CLOEXEC);
> if (fd < 0) {
> printf("%s - Failed to open \"%s\"\n", strerror(errno), buf);
> exit(EXIT_FAILURE);
> }
>
> ret = syscall(__NR_procfd_signal, fd, SIGKILL, NULL, 0);
> if (ret < 0) {
> printf("Failed to send SIGKILL \"%s\"\n", strerror(errno));
> close(fd);
> exit(EXIT_FAILURE);
> }
>
> close(fd);
>
> exit(EXIT_SUCCESS);
> }
>
> [1]: https://lkml.org/lkml/2018/11/18/130
>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Serge Hallyn <serge@hallyn.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirsky <luto@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> Changelog:
> v1:
> - patch introduced
> ---
> arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> fs/proc/base.c | 6 ++
> include/linux/proc_fs.h | 1 +
> include/linux/syscalls.h | 2 +
> kernel/signal.c | 76 ++++++++++++++++++++++++--
> 6 files changed, 81 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 3cf7b533b3d1..e637eab883e9 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -398,3 +398,4 @@
> 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> 386 i386 rseq sys_rseq __ia32_sys_rseq
> +387 i386 procfd_signal sys_procfd_signal __ia32_sys_procfd_signal
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index f0b1709a5ffb..e95f6741ab42 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -343,6 +343,7 @@
> 332 common statx __x64_sys_statx
> 333 common io_pgetevents __x64_sys_io_pgetevents
> 334 common rseq __x64_sys_rseq
> +335 common procfd_signal __x64_sys_procfd_signal
>
> #
> # x32-specific system call numbers start at 512 to avoid cache impact
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6365a4fea314..a12c9de92bd0 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -3055,6 +3055,12 @@ static const struct file_operations proc_tgid_base_operations = {
> .release = proc_tgid_release,
> };
>
> +bool proc_is_procfd(const struct file *file)
> +{
> + return d_is_dir(file->f_path.dentry) &&
> + (file->f_op == &proc_tgid_base_operations);
> +}
> +
> static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> {
> return proc_pident_lookup(dir, dentry,
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index d0e1f1522a78..2d53a47fba34 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -73,6 +73,7 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
> int (*show)(struct seq_file *, void *),
> proc_write_t write,
> void *data);
> +extern bool proc_is_procfd(const struct file *file);
>
> #else /* CONFIG_PROC_FS */
>
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 2ac3d13a915b..a5ca8cb84566 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -907,6 +907,8 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
> unsigned mask, struct statx __user *buffer);
> asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
> int flags, uint32_t sig);
> +asmlinkage long sys_procfd_signal(int fd, int sig, siginfo_t __user *info,
> + int flags);
>
> /*
> * Architecture-specific system calls
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9a32bc2088c9..e8a8929de710 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -19,7 +19,9 @@
> #include <linux/sched/task.h>
> #include <linux/sched/task_stack.h>
> #include <linux/sched/cputime.h>
> +#include <linux/file.h>
> #include <linux/fs.h>
> +#include <linux/proc_fs.h>
> #include <linux/tty.h>
> #include <linux/binfmts.h>
> #include <linux/coredump.h>
> @@ -3286,6 +3288,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
> }
> #endif
>
> +static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> +{
> + clear_siginfo(info);
> + info->si_signo = sig;
> + info->si_errno = 0;
> + info->si_code = SI_USER;
> + info->si_pid = task_tgid_vnr(current);
> + info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> +}
> +
> /**
> * sys_kill - send a signal to a process
> * @pid: the PID of the process
> @@ -3295,16 +3307,68 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> {
> struct kernel_siginfo info;
>
> - clear_siginfo(&info);
> - info.si_signo = sig;
> - info.si_errno = 0;
> - info.si_code = SI_USER;
> - info.si_pid = task_tgid_vnr(current);
> - info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> + prepare_kill_siginfo(sig, &info);
>
> return kill_something_info(sig, &info, pid);
> }
>
> +/**
> + * sys_procfd_signal - send a signal to a process through a process file
> + * descriptor
> + * @fd: the file descriptor of the process
> + * @sig: signal to be sent
> + * @info: the signal info
> + * @flags: future flags to be passed
> + */
> +SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info,
> + int, flags)
> +{
> + int ret;
> + struct pid *pid;
> + kernel_siginfo_t kinfo;
> + struct fd f;
> +
> + /* Enforce flags be set to 0 until we add an extension. */
> + if (flags)
> + return -EINVAL;
> +
> + f = fdget_raw(fd);
> + if (!f.file)
> + return -EBADF;
> +
> + ret = -EINVAL;
> + /* Is this a process file descriptor? */
> + if (!proc_is_procfd(f.file))
> + goto err;
> +
> + pid = f.file->private_data;
You never addressed my comment on the previous patch about your use of
private_data here. Why can't you use the struct pid reference that's
already in the inode?
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Daniel Colascione @ 2018-11-19 15:57 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Christian Brauner, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, linux-man, Kees Cook
In-Reply-To: <CALCETrUY=Hk6=BjgPuDBgj5J1T_b5Q5u1uVOWHjGWXwmHoZBEQ@mail.gmail.com>
On Mon, Nov 19, 2018 at 7:45 AM, Andy Lutomirski <luto@kernel.org> 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?
+1
>> + 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?
All the signal functions exempt the current process from access
checks. Whether that's useful or not (and I think it is), being
inconsistent here would be wrong.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Dave Martin @ 2018-11-19 15:49 UTC (permalink / raw)
To: Christian Brauner
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, Kees Cook
In-Reply-To: <20181118111751.6142-1-christian@brauner.io>
On Sun, Nov 18, 2018 at 12:17:51PM +0100, Christian Brauner wrote:
> With this patch an open() call on /proc/<pid> will give userspace a handle
> to struct pid of the process associated with /proc/<pid>. This allows to
> maintain a stable handle on a process.
> I have been discussing various approaches extensively during technical
> conferences this year culminating in a long argument with Eric at Linux
> Plumbers. The general consensus was that having a handle on a process
> will be something that is very simple and easy to maintain with the
> option of being extensible via a more advanced api if the need arises. I
> believe that this patch is the most simple, dumb, and therefore
> maintainable solution.
>
> The need for this has arisen in order to reliably kill a process without
> running into issues of the pid being recycled as has been described in the
> rejected patch [1]. To fulfill the need described in that patchset a new
It would certainly be good to fix this. IIUC, things like pkill(1) are
a gamble today and can probably kill a process that doesn't fulfil the
match criteria due to PID recycling. (If not, I'd certainly like to
understand how that is prevented.)
> ioctl() PROC_FD_SIGNAL is added. It can be used to send signals to a
> process via a file descriptor:
>
> int fd = open("/proc/1234", O_DIRECTORY | O_CLOEXEC);
> ioctl(fd, PROC_FD_SIGNAL, SIGKILL);
> close(fd);
>
> Note, the stable handle will allow us to carefully extend this feature in
> the future.
A concern here would be that an fd-based shadow API may need to be
created, duplicating the whole PID-based API that already exists.
However, so long as the PID is stabilised against recycling, an ordinary
kill() call seems fine as a way to kill the target process, and no new
API or permission model seems to be needed.
It occurs to me that a mechanism for holding a reference on a third
process already exists: ptrace.
Suppose we were to have something like
ptrace(PTRACE_MONITOR, pid, 0, 0);
that subscribes the caller for the same set of notifications via wait()
as the process' real parent gets, and prevents PID recycling until the
zombie is consumed be everyone who is subscribed.
Multiple PTRACE_MONITOR attachments could be allowed for a given target,
in addition to the real parent and regular ptrace-parent (if any).
There are a couple of wrinkles:
* ptrace() operates on tasks, not processes, so the precise semantics
of PTRACE_MONITOR attachment would need a bit of thought.
* Odd mechanisms for discovering PIDs, like the unix(7) SCM_CREDENTIALS
message might need a special variant to get a PTRACE_MONITOR attachment
along with the message. This variant behaviour would need to be opt-in
for the recipient.
OTOH, extending ptrace may bring problems of its own :/
Cheers
---Dave
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Andy Lutomirski @ 2018-11-19 15:45 UTC (permalink / raw)
To: Christian Brauner
Cc: Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Lutomirski, Andrew Morton, Oleg Nesterov, Aleksa Sarai,
Al Viro, Linux FS Devel, Linux API, Daniel Colascione, Tim Murray,
linux-man, Kees Cook
In-Reply-To: <20181119103241.5229-3-christian@brauner.io>
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?
> +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()?
> + 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?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox