* Re: [PATCH v2 4/5] signal: PIDFD_SIGNAL_TID threads via pidfds
From: Jann Horn @ 2019-03-30 1:06 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, David Howells, Serge E. Hallyn, Linux API,
kernel list, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <20190329155425.26059-5-christian@brauner.io>
On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner <christian@brauner.io> wrote:
> 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:
[...]
> 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
[...]
> +/* 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`
> #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
[...]
> +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
[...]
> + * 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
[...]
> @@ -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?
Something that was already kinda weird in the existing code, but is
getting worse with TIDs is the handling of SI_USER with siginfo.
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
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
can use info==NULL, right? That should create wrongness parity with
do_rt_sigqueueinfo().
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?
> err:
> fdput(f);
> return ret;
> }
[...]
^ permalink raw reply
* Re: [PATCH v2 4/5] signal: PIDFD_SIGNAL_TID threads via pidfds
From: Christian Brauner @ 2019-03-30 1:22 UTC (permalink / raw)
To: Jann Horn
Cc: Andy Lutomirski, David Howells, Serge E. Hallyn, Linux API,
kernel list, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <CAG48ez0=Q2CzgTGLiEU7F9B=gS-58VrpKz2XQ369cemo2FJdbQ@mail.gmail.com>
On Sat, Mar 30, 2019 at 02:06:34AM +0100, Jann Horn wrote:
> On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner <christian@brauner.io> wrote:
> > 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:
> [...]
> > 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
> [...]
> > +/* 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.
>
> > #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
> [...]
> > +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.
>
> [...]
> > + * 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.
>
> [...]
> > @@ -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.
>
> 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.
> 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?
> 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.
> 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;
}
> 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.
^ permalink raw reply
* Re: [PATCH v2 4/5] signal: PIDFD_SIGNAL_TID threads via pidfds
From: Christian Brauner @ 2019-03-30 1:34 UTC (permalink / raw)
To: Jann Horn
Cc: Andy Lutomirski, David Howells, Serge E. Hallyn, Linux API,
kernel list, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <20190330012229.yt3hecmgaj2r6vp7@brauner.io>
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:
> > On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner <christian@brauner.io> wrote:
> > > 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:
> > [...]
> > > 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
> > [...]
> > > +/* 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.
>
> >
> > > #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
> > [...]
> > > +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.
>
> >
> > [...]
> > > + * 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.
>
> >
> > [...]
> > > @@ -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.
>
> >
> > 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.
>
> > 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?
>
> > 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.
>
> > 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;
> }
>
> > 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.
^ permalink raw reply
* Re: [PATCH v2 4/5] signal: PIDFD_SIGNAL_TID threads via pidfds
From: Christian Brauner @ 2019-03-30 1:42 UTC (permalink / raw)
To: Jann Horn
Cc: Andy Lutomirski, David Howells, Serge E. Hallyn, Linux API,
kernel list, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <20190330013416.vnidfkjbsxxhzslm@brauner.io>
On Sat, Mar 30, 2019 at 02:34:16AM +0100, Christian Brauner wrote:
> 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:
> > > On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner <christian@brauner.io> wrote:
> > > > 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:
> > > [...]
> > > > 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
> > > [...]
> > > > +/* 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.
> >
> > >
> > > > #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
> > > [...]
> > > > +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.
> >
> > >
> > > [...]
> > > > + * 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.
> >
> > >
> > > [...]
> > > > @@ -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.
>
> >
> > >
> > > 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.
> >
> > > 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?
> >
> > > 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.
> >
> > > 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;
> > }
> >
> > > 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.
Sorry, that was probably unclear. I was referring to:
> > > 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
This makes perfect sense as a bugfix for 5.1 imo.
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Daniel Colascione @ 2019-03-30 5:35 UTC (permalink / raw)
To: Christian Brauner
Cc: Jonathan Kowalski, Jann Horn, Konstantin Khlebnikov,
Andy Lutomirski, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai
In-Reply-To: <20190328103813.eogszrqbitw3e7k7@brauner.io>
On Thu, Mar 28, 2019 at 3:38 AM Christian Brauner <christian@brauner.io> wrote:
>
> > All that said, thanks for the work on this once again. My intention is
> > just that we don't end up with an API that could have been done better
> > and be cleaner to use for potential users in the coming years.
>
> Thanks for your input on all of this. I still don't find multiplexers in
> the style of seccomp()/fsconfig()/keyctl() to be a problem since they
> deal with a specific task. They are very much different from ioctl()s in
> that regard. But since Joel, you, and Daniel found the pidctl() approach
> not very nice I dropped it. The interface needs to be satisfactory for
> all of us especially since Android and other system managers will be the
> main consumers.
Thanks.
> So let's split this into pidfd_open(pid_t pid, unsigned int flags) which
> allows to cleanly get pidfds independent procfs and do the translation
> to procpidfds in an ioctl() as we've discussed in prior threads. This
I sustain my objection to adding an ioctl. Compared to a system call,
an ioctl has a more rigid interface, greater susceptibility to
programmer error (due to the same ioctl control code potentially doing
different things for different file types), longer path length, and
more awkward filtering/monitoring/auditing/tracing. We've discussed
this issue at length before, and I thought we all agreed to use system
calls, not ioctl, for core kernel functionality. So why is an ioctl
suddenly back on the table? The way I see it, an ioctl has no
advantages except for 1) conserving system call numbers, which are not
scarce, and 2) avoiding the system call number coordination problem
(and the coordination problem isn't a factor for core kernel code). I
don't understand everyone's reluctance to add new system calls. What
am I missing? Why would we give up all the advantages that a system
call gives us?
I also don't understand Andy's argument on the other thread that an
ioctl is okay if it's an "operation on an FD" --- *most* system calls
are operations on FDs. We don't have an ioctl for sendmsg(2) and it's
an "operation on an FD".
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Jonathan Kowalski @ 2019-03-30 6:25 UTC (permalink / raw)
To: Daniel Colascione
Cc: Christian Brauner, Jann Horn, Konstantin Khlebnikov,
Andy Lutomirski, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai
In-Reply-To: <CAKOZueusdFurOb_uFZ3PDmZeJvYvHVKMx6=TpiWPERkKRiHfiw@mail.gmail.com>
On Sat, Mar 30, 2019 at 5:35 AM Daniel Colascione <dancol@google.com> wrote:
>
> On Thu, Mar 28, 2019 at 3:38 AM Christian Brauner <christian@brauner.io> wrote:
> >
> > > All that said, thanks for the work on this once again. My intention is
> > > just that we don't end up with an API that could have been done better
> > > and be cleaner to use for potential users in the coming years.
> >
> > Thanks for your input on all of this. I still don't find multiplexers in
> > the style of seccomp()/fsconfig()/keyctl() to be a problem since they
> > deal with a specific task. They are very much different from ioctl()s in
> > that regard. But since Joel, you, and Daniel found the pidctl() approach
> > not very nice I dropped it. The interface needs to be satisfactory for
> > all of us especially since Android and other system managers will be the
> > main consumers.
>
> Thanks.
>
> > So let's split this into pidfd_open(pid_t pid, unsigned int flags) which
> > allows to cleanly get pidfds independent procfs and do the translation
> > to procpidfds in an ioctl() as we've discussed in prior threads. This
>
> I sustain my objection to adding an ioctl. Compared to a system call,
> an ioctl has a more rigid interface, greater susceptibility to
> programmer error (due to the same ioctl control code potentially doing
> different things for different file types), longer path length, and
> more awkward filtering/monitoring/auditing/tracing. We've discussed
> this issue at length before, and I thought we all agreed to use system
> calls, not ioctl, for core kernel functionality. So why is an ioctl
> suddenly back on the table? The way I see it, an ioctl has no
> advantages except for 1) conserving system call numbers, which are not
> scarce, and 2) avoiding the system call number coordination problem
> (and the coordination problem isn't a factor for core kernel code). I
> don't understand everyone's reluctance to add new system calls. What
> am I missing? Why would we give up all the advantages that a system
> call gives us?
>
I agree in general, but in this particular case a system call or an
ioctl doesn't matter much, all it does is take the pidfd, the command,
and /proc's dir fd.
If you start adding a system call for every specific operation on file
descriptors, it *will* become a problem. Besides, the translation is
just there because it is racy to do in userspace, it is not some well
defined core kernel functionality. Therefore, it is just a way to
enter the kernel to do the openat in a race free and safe manner.
As is, the facility being provided through an ioctl on the pidfd is
not something I'd consider a problem. I think the translation stuff
should also probably be an extension of ioctl_ns(2) (but I wouldn't be
opposed if translate_pid is resurrected as is).
For anything more involved than ioctl(pidfd, PIDFD_TO_PROCFD,
procrootfd), I'd agree that a system call would be a cleaner
interface, otherwise, if you cannot generalise it, using ioctls as a
command interface is probably the better tradeoff here.
> I also don't understand Andy's argument on the other thread that an
> ioctl is okay if it's an "operation on an FD" --- *most* system calls
> are operations on FDs. We don't have an ioctl for sendmsg(2) and it's
> an "operation on an FD".
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Daniel Colascione @ 2019-03-30 7:39 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Christian Brauner, Jann Horn, Konstantin Khlebnikov,
Andy Lutomirski, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai
In-Reply-To: <CAGLj2rEtgiJfmKgo4bJvmBYp1Y5f1SELVfEu7Ayfi62zOAxjGQ@mail.gmail.com>
On Fri, Mar 29, 2019 at 11:25 PM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
>
> On Sat, Mar 30, 2019 at 5:35 AM Daniel Colascione <dancol@google.com> wrote:
> >
> > On Thu, Mar 28, 2019 at 3:38 AM Christian Brauner <christian@brauner.io> wrote:
> > >
> > > > All that said, thanks for the work on this once again. My intention is
> > > > just that we don't end up with an API that could have been done better
> > > > and be cleaner to use for potential users in the coming years.
> > >
> > > Thanks for your input on all of this. I still don't find multiplexers in
> > > the style of seccomp()/fsconfig()/keyctl() to be a problem since they
> > > deal with a specific task. They are very much different from ioctl()s in
> > > that regard. But since Joel, you, and Daniel found the pidctl() approach
> > > not very nice I dropped it. The interface needs to be satisfactory for
> > > all of us especially since Android and other system managers will be the
> > > main consumers.
> >
> > Thanks.
> >
> > > So let's split this into pidfd_open(pid_t pid, unsigned int flags) which
> > > allows to cleanly get pidfds independent procfs and do the translation
> > > to procpidfds in an ioctl() as we've discussed in prior threads. This
> >
> > I sustain my objection to adding an ioctl. Compared to a system call,
> > an ioctl has a more rigid interface, greater susceptibility to
> > programmer error (due to the same ioctl control code potentially doing
> > different things for different file types), longer path length, and
> > more awkward filtering/monitoring/auditing/tracing. We've discussed
> > this issue at length before, and I thought we all agreed to use system
> > calls, not ioctl, for core kernel functionality. So why is an ioctl
> > suddenly back on the table? The way I see it, an ioctl has no
> > advantages except for 1) conserving system call numbers, which are not
> > scarce, and 2) avoiding the system call number coordination problem
> > (and the coordination problem isn't a factor for core kernel code). I
> > don't understand everyone's reluctance to add new system calls. What
> > am I missing? Why would we give up all the advantages that a system
> > call gives us?
> >
>
> I agree in general, but in this particular case a system call or an
> ioctl doesn't matter much, all it does is take the pidfd, the command,
> and /proc's dir fd.
Thanks again.
I agree that the operation we're discussing has a simple signature,
but signature flexibility isn't the only reason to prefer a system
call over an ioctl. There are other reasons for preferring system
calls to ioctls (safety, tracing, etc.) that apply even if the
operation we're discussing has a relatively simple signature: for
example, every system call has a distinct and convenient ftrace event,
but ioctls don't; strace filtering Just Works on a
system-call-by-system-call basis, but it doesn't for ioctls; and
documentation for system calls is much more discoverable (e.g., man
-k) than documentation for ioctls. Even if the distinction doesn't
matter much, IMHO, it still matters a little, enough to favor a system
call without an offsetting advantage for the ioctl option.
> If you start adding a system call for every specific operation on file
> descriptors, it *will* become a problem.
I'm not sure what you mean. Do you mean that adding a top-level system
call for every operation that might apply to one specific kind of file
descriptor would lead, as the overall result, to the kernel having
enough system calls to cause negative consequences? I'm not sure I
agree, but accepting this idea for the sake of discussion: shouldn't
we be more okay with system calls for features present on almost all
systems --- like procfs --- even if we punt to ioctls very rarely-used
functionality, e.g., some hypothetical special squeak noise that you
could get some specific 1995 adlib clone to make?
> Besides, the translation is
> just there because it is racy to do in userspace, it is not some well
> defined core kernel functionality.
> Therefore, it is just a way to
> enter the kernel to do the openat in a race free and safe manner.
I agree that the translation has to be done in the kernel, not
userspace, and that the kernel must provide to userspace some
interface for requesting that the translation happen: we're just
discussing the shape of this interface. Shouldn't all interfaces
provided by the kernel to userspace be equally well defined? I'm not
sure that the internal simplicity of the operation matters much
either. There are already explicit system calls for some
simple-to-implement things, e.g., timerfd_gettime. It's worth noting
that timerfd is (IIRC), like procfs, a feature that's both ubiquitous
and optional.
> As is, the facility being provided through an ioctl on the pidfd is
> not something I'd consider a problem.
You're right that from a signature perspective, using an ioctl isn't a
problem. I just want to make sure we take into account the other,
non-signature advantages that system calls have over ioctls.
> I think the translation stuff
> should also probably be an extension of ioctl_ns(2) (but I wouldn't be
> opposed if translate_pid is resurrected as is).
> For anything more involved than ioctl(pidfd, PIDFD_TO_PROCFD,
> procrootfd), I'd agree that a system call would be a cleaner
> interface, otherwise, if you cannot generalise it, using ioctls as a
> command interface is probably the better tradeoff here.
Sure: I just want to better understand everyone else's thought process
here, having been frustrated with things like the termios ioctls being
ioctls.
^ permalink raw reply
* Re: [PATCH v2 2/5] pid: add pidfd_open()
From: Jürg Billeter @ 2019-03-30 11:53 UTC (permalink / raw)
To: Christian Brauner, jannh, luto, dhowells, serge, linux-api,
linux-kernel
Cc: arnd, ebiederm, khlebnikov, keescook, adobriyan, tglx,
mtk.manpages, bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy,
cyphar, viro, joel, dancol
In-Reply-To: <20190329155425.26059-3-christian@brauner.io>
On Fri, 2019-03-29 at 16:54 +0100, Christian Brauner wrote:
> diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h
> index ac49a220cf2a..d6c7c0701997 100644
> --- a/include/uapi/linux/wait.h
> +++ b/include/uapi/linux/wait.h
> @@ -18,5 +18,7 @@
> #define P_PID 1
> #define P_PGID 2
>
> +/* Get a file descriptor for /proc/<pid> of the corresponding pidfd
> */
> +#define PIDFD_GET_PROCFD _IOR('p', 1, int)
>
> #endif /* _UAPI_LINUX_WAIT_H */
This is missing an entry in Documentation/ioctl/ioctl-number.txt and is
actually conflicting with existing entries.
However, I'd actually prefer a syscall to allow strict whitelisting via
seccomp and avoid the other ioctl disadvantages that Daniel has already
mentioned.
Cheers,
Jürg
^ permalink raw reply
* Re: [PATCH ghak90 V5 05/10] audit: add containerid support for ptrace and signals
From: Richard Guy Briggs @ 2019-03-30 12:55 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, Steve Grubb,
David Howells, Simo Sorce, Eric Paris, Serge E. Hallyn,
Eric W . Biederman, nhorman
In-Reply-To: <20190328020410.pl7odjknw7robdk3@madcap2.tricolour.ca>
On 2019-03-27 22:04, Richard Guy Briggs wrote:
> On 2019-03-27 22:17, Ondrej Mosnacek wrote:
> > On Fri, Mar 15, 2019 at 7:34 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Add audit container identifier support to ptrace and signals. In
> > > particular, the "ref" field provides a way to label the auxiliary record
> > > to which it is associated.
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > include/linux/audit.h | 1 +
> > > kernel/audit.c | 2 ++
> > > kernel/audit.h | 2 ++
> > > kernel/auditsc.c | 23 +++++++++++++++++------
> > > 4 files changed, 22 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index 43438192ca2a..ebd6625ca80e 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -35,6 +35,7 @@ struct audit_sig_info {
> > > uid_t uid;
> > > pid_t pid;
> > > char ctx[0];
> > > + u64 cid;
> > > };
> >
> > It seems like this structure implicitly defines the format of some
> > message that is sent to userspace... If so, how will userspace detect
> > that a new format (including the cid) is being used? Even assuming the
> > fixed order as pointed out by Neil, the message still seems to be
> > variable-sized so userspace cannot even use the length to infer that.
> > Am I missing something here? (I hope I am :)
>
> How humble of you again. No, you're not missing something. This ends
> up being an api change... That can be fixed in userspace by checking
> for AUDIT_FEATURE_BITMAP_CONTAINERID, but how do we make a newer kernel
> not break an older userspace... I think this was the original rationale
> for adding it after the ctx but totally missing the fact that the latter
> is a variable-length field.
The way to address this on Steve Grubb's advice is to create a new
message type that incorporates a new struct with the structure above,
leaving the old one with the old message type deprecated. I've coded
this up along with userspace support.
> This patch really should be split into audit_sig_cid changes in a patch
> by itself and target_cid changes which could go with the second and
> fourth patches.
I've also done this which Paul had already asked for, not quite in this
form.
I believe this addresses all the outstanding issues.
> > > struct audit_buffer;
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 8cc0e88d7f2a..cfa659b3f6c4 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -138,6 +138,7 @@ struct audit_net {
> > > kuid_t audit_sig_uid = INVALID_UID;
> > > pid_t audit_sig_pid = -1;
> > > u32 audit_sig_sid = 0;
> > > +u64 audit_sig_cid = AUDIT_CID_UNSET;
> > >
> > > /* Records can be lost in several ways:
> > > 0) [suppressed in audit_alloc]
> > > @@ -1515,6 +1516,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > > memcpy(sig_data->ctx, ctx, len);
> > > security_release_secctx(ctx, len);
> > > }
> > > + sig_data->cid = audit_sig_cid;
> > > audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> > > sig_data, sizeof(*sig_data) + len);
> > > kfree(sig_data);
> > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > index c00e2ee3c6b3..c5ac6436317e 100644
> > > --- a/kernel/audit.h
> > > +++ b/kernel/audit.h
> > > @@ -148,6 +148,7 @@ struct audit_context {
> > > kuid_t target_uid;
> > > unsigned int target_sessionid;
> > > u32 target_sid;
> > > + u64 target_cid;
> > > char target_comm[TASK_COMM_LEN];
> > >
> > > struct audit_tree_refs *trees, *first_trees;
> > > @@ -344,6 +345,7 @@ extern void audit_filter_inodes(struct task_struct *tsk,
> > > extern pid_t audit_sig_pid;
> > > extern kuid_t audit_sig_uid;
> > > extern u32 audit_sig_sid;
> > > +extern u64 audit_sig_cid;
> > >
> > > extern int audit_filter(int msgtype, unsigned int listtype);
> > >
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index a8c8b44b954d..f04e115df5dc 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -113,6 +113,7 @@ struct audit_aux_data_pids {
> > > kuid_t target_uid[AUDIT_AUX_PIDS];
> > > unsigned int target_sessionid[AUDIT_AUX_PIDS];
> > > u32 target_sid[AUDIT_AUX_PIDS];
> > > + u64 target_cid[AUDIT_AUX_PIDS];
> > > char target_comm[AUDIT_AUX_PIDS][TASK_COMM_LEN];
> > > int pid_count;
> > > };
> > > @@ -1514,7 +1515,7 @@ static void audit_log_exit(void)
> > > for (aux = context->aux_pids; aux; aux = aux->next) {
> > > struct audit_aux_data_pids *axs = (void *)aux;
> > >
> > > - for (i = 0; i < axs->pid_count; i++)
> > > + for (i = 0; i < axs->pid_count; i++) {
> > > if (audit_log_pid_context(context, axs->target_pid[i],
> > > axs->target_auid[i],
> > > axs->target_uid[i],
> > > @@ -1522,14 +1523,20 @@ static void audit_log_exit(void)
> > > axs->target_sid[i],
> > > axs->target_comm[i]))
> > > call_panic = 1;
> > > + audit_log_contid(context, axs->target_cid[i]);
> > > + }
> > > }
> > >
> > > - if (context->target_pid &&
> > > - audit_log_pid_context(context, context->target_pid,
> > > - context->target_auid, context->target_uid,
> > > - context->target_sessionid,
> > > - context->target_sid, context->target_comm))
> > > + if (context->target_pid) {
> > > + if (audit_log_pid_context(context, context->target_pid,
> > > + context->target_auid,
> > > + context->target_uid,
> > > + context->target_sessionid,
> > > + context->target_sid,
> > > + context->target_comm))
> > > call_panic = 1;
> > > + audit_log_contid(context, context->target_cid);
> > > + }
> > >
> > > if (context->pwd.dentry && context->pwd.mnt) {
> > > ab = audit_log_start(context, GFP_KERNEL, AUDIT_CWD);
> > > @@ -2360,6 +2367,7 @@ void __audit_ptrace(struct task_struct *t)
> > > context->target_uid = task_uid(t);
> > > context->target_sessionid = audit_get_sessionid(t);
> > > security_task_getsecid(t, &context->target_sid);
> > > + context->target_cid = audit_get_contid(t);
> > > memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
> > > }
> > >
> > > @@ -2387,6 +2395,7 @@ int audit_signal_info(int sig, struct task_struct *t)
> > > else
> > > audit_sig_uid = uid;
> > > security_task_getsecid(current, &audit_sig_sid);
> > > + audit_sig_cid = audit_get_contid(current);
> > > }
> > >
> > > if (!audit_signals || audit_dummy_context())
> > > @@ -2400,6 +2409,7 @@ int audit_signal_info(int sig, struct task_struct *t)
> > > ctx->target_uid = t_uid;
> > > ctx->target_sessionid = audit_get_sessionid(t);
> > > security_task_getsecid(t, &ctx->target_sid);
> > > + ctx->target_cid = audit_get_contid(t);
> > > memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
> > > return 0;
> > > }
> > > @@ -2421,6 +2431,7 @@ int audit_signal_info(int sig, struct task_struct *t)
> > > axp->target_uid[axp->pid_count] = t_uid;
> > > axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
> > > security_task_getsecid(t, &axp->target_sid[axp->pid_count]);
> > > + axp->target_cid[axp->pid_count] = audit_get_contid(t);
> > > memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
> > > axp->pid_count++;
> > >
> >
> > Ondrej Mosnacek <omosnace at redhat dot com>
>
> - RGB
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Jonathan Kowalski @ 2019-03-30 14:30 UTC (permalink / raw)
To: Daniel Colascione
Cc: Christian Brauner, Jann Horn, Konstantin Khlebnikov,
Andy Lutomirski, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai
In-Reply-To: <CAKOZuesjdfNptM9Pt_uzsDVefW6LXhHPHG2a3rJRAe0rpeWX6g@mail.gmail.com>
On Sat, Mar 30, 2019 at 7:39 AM Daniel Colascione <dancol@google.com> wrote:
>
> [SNIP]
>
> Thanks again.
>
> I agree that the operation we're discussing has a simple signature,
> but signature flexibility isn't the only reason to prefer a system
> call over an ioctl. There are other reasons for preferring system
> calls to ioctls (safety, tracing, etc.) that apply even if the
> operation we're discussing has a relatively simple signature: for
> example, every system call has a distinct and convenient ftrace event,
> but ioctls don't; strace filtering Just Works on a
> system-call-by-system-call basis, but it doesn't for ioctls; and
It does for those with a unique number.
> documentation for system calls is much more discoverable (e.g., man
> -k) than documentation for ioctls. Even if the distinction doesn't
> matter much, IMHO, it still matters a little, enough to favor a system
> call without an offsetting advantage for the ioctl option.
It will be alongside other I/O operations in the pidfd man page.
>
> > If you start adding a system call for every specific operation on file
> > descriptors, it *will* become a problem.
>
> I'm not sure what you mean. Do you mean that adding a top-level system
> call for every operation that might apply to one specific kind of file
> descriptor would lead, as the overall result, to the kernel having
> enough system calls to cause negative consequences? I'm not sure I
> agree, but accepting this idea for the sake of discussion: shouldn't
> we be more okay with system calls for features present on almost all
> systems --- like procfs --- even if we punt to ioctls very rarely-used
> functionality, e.g., some hypothetical special squeak noise that you
> could get some specific 1995 adlib clone to make?
Consider if we were to do:
int procpidfd_open(int pidfd, int procrootfd);
This system call is useless besides a very specific operation for a
very specific usecase on a file descriptor. Then, you figure you might
want to support procpidfd back to pidfd (although YAGNI), so you will
add a CMD or flags argument now,
int procpidfd_open(int pidfd, int procrootfd/procpidfd, unsigned int flags);
int procpidfd = procpidfd_open(fd, procrootfd, PIDFD_TO_PROCPIDFD);
int pidfd = procpidfd_open(-1, procpidfd, PROCPIDFD_TO_PIDFD);
vs procpidfd2pidfd(int procpidfd); if you did not foresee the need.
Then, you want pidfd2pid, and so on.
If you don't, you will have to add a command interface in the new
system call, which changes parameters depending on the flags. This is
already starting to get ugly. In the end, it's a matter of taste, this
pattern if exploited leads to endless proliferation of the system call
interface, often ridden with short-sighted APIs, because you cannot
know if you want a focused call or a command style call. ioctl is
already a command interface, and 10 system calls for each command is
_NOT_ nice, from a user perspective.
>
> > Besides, the translation is
> > just there because it is racy to do in userspace, it is not some well
> > defined core kernel functionality.
> > Therefore, it is just a way to
> > enter the kernel to do the openat in a race free and safe manner.
>
> I agree that the translation has to be done in the kernel, not
> userspace, and that the kernel must provide to userspace some
> interface for requesting that the translation happen: we're just
> discussing the shape of this interface. Shouldn't all interfaces
> provided by the kernel to userspace be equally well defined? I'm not
> sure that the internal simplicity of the operation matters much
> either. There are already explicit system calls for some
> simple-to-implement things, e.g., timerfd_gettime. It's worth noting
> that timerfd is (IIRC), like procfs, a feature that's both ubiquitous
> and optional.
It is well defined, it has a well defined signature, and it will error
out if you don't use it properly. Again, what it does is very limited
and niche. I am not sure it warrants a system call of its own.
timerfd_gettime was an afterthought anyway, that's probably not a good
example (it was more to just match the POSIX timers interface as the
original timerfd never had support for querying, so the split into two
steps, create and initialize, you could argue one could do it without
a syscall, but it still has a well defined argument list, and
accepting elaborate data structures into an ioctl is not a good
interface to plumb, so that's easily justified).
It's much like socket and setsockopt/getsockopt in nature. I would
even say APIs separating creation and configuration age well and are
better, but a process doesn't fit such a model cleanly.
>
> > As is, the facility being provided through an ioctl on the pidfd is
> > not something I'd consider a problem.
>
> You're right that from a signature perspective, using an ioctl isn't a
> problem. I just want to make sure we take into account the other,
> non-signature advantages that system calls have over ioctls.
>
> > I think the translation stuff
> > should also probably be an extension of ioctl_ns(2) (but I wouldn't be
> > opposed if translate_pid is resurrected as is).
> > For anything more involved than ioctl(pidfd, PIDFD_TO_PROCFD,
> > procrootfd), I'd agree that a system call would be a cleaner
> > interface, otherwise, if you cannot generalise it, using ioctls as a
> > command interface is probably the better tradeoff here.
>
> Sure: I just want to better understand everyone else's thought process
> here, having been frustrated with things like the termios ioctls being
> ioctls.
On that note, I don't buy the safety argument here, or the seccomp argument.
You need to consider what this is, on a case by case basis.
ioctl(pidfd, PIDFD_TO_PROCFD, procrootfd);
See? You need /proc's root fd to be able to get to the /proc/<PID> dir
fd, in a race free manner. You don't need to have the overhead of
filtering to contain access, as privilege is bound to descriptors.
However, an ioctl to do pidfd_send_signal would have indeed been
problematic, it takes in structures, it has a very core use case, and
a very compelling advantage over existing signal sending APIs.
On the point, using file descriptors, you can safely delegate metadata
access to a process it has a pidfd open for, without /proc being in
its mount namespace (which was what Andy was after). File descriptors
are acting as capabilities, so if you don't leak the /proc's root fd
to the process, you can be assured it wouldn't be able to do any
metadata access (and you don't have /proc mounted).
*No need* to blacklist the ioctl or the system call, as it wouldn't
work anyway. *This* is a good interface, with little to no overhead
for security. So whether this is an ioctl or a system call doesn't
matter at all, because it doesn't suffer from all those cases where
you really want to avoid ioctls and do a well scoped system call as an
entrypoint instead.
There is not much incentive to it, really.
^ permalink raw reply
* Re: [PATCH v2 2/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-30 14:37 UTC (permalink / raw)
To: Jürg Billeter, torvalds
Cc: jannh, luto, dhowells, serge, linux-api, linux-kernel, arnd,
ebiederm, khlebnikov, keescook, adobriyan, tglx, mtk.manpages,
bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy, cyphar, viro,
joel, dancol
In-Reply-To: <f0d590b96b7d11ae21a9a06040b8ffa85fa74e2a.camel@bitron.ch>
On Sat, Mar 30, 2019 at 12:53:57PM +0100, Jürg Billeter wrote:
> On Fri, 2019-03-29 at 16:54 +0100, Christian Brauner wrote:
> > diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h
> > index ac49a220cf2a..d6c7c0701997 100644
> > --- a/include/uapi/linux/wait.h
> > +++ b/include/uapi/linux/wait.h
> > @@ -18,5 +18,7 @@
> > #define P_PID 1
> > #define P_PGID 2
> >
> > +/* Get a file descriptor for /proc/<pid> of the corresponding pidfd
> > */
> > +#define PIDFD_GET_PROCFD _IOR('p', 1, int)
> >
> > #endif /* _UAPI_LINUX_WAIT_H */
>
> This is missing an entry in Documentation/ioctl/ioctl-number.txt and is
> actually conflicting with existing entries.
Thanks. Yes, Jann mentioned this too.
>
> However, I'd actually prefer a syscall to allow strict whitelisting via
> seccomp and avoid the other ioctl disadvantages that Daniel has already
> mentioned.
You can filter ioctls with seccomp.
I have compromised quite a bit now and I think what we have is perfectly
fine. a single clean syscalls pidfd_open() that lets you get pidfds for
threads and thread-group leaders independent of procfs and a clean,
simple fd->fd converstion ioctl() that is a property of the f_ops of the
pidfd to get an fd to /proc/<pid> for metadata access. Btw, this being a
part of the pidfd f_ops seems strikingly elegant to me. Because it
expresses the notion that the metadata is implicitly part of the pidfd
nicely. But I might just be dumb.
I do not see the need to add another syscall that is conditional on
CONFIG_PROC_FS and only does a pidfd to /proc/<pid>-fd conversion.
That's almost the definition of what an ioctl() is most suited for.
I get the opposition to multiplexers but consider if we where to oppose
all of them. Let's leave ioctls out and just look at a few widely used
multiplexer syscalls:
1. seccomp()
- number of supported commands: 4
2. prctl()
- number of supported commands: 45
3. keyctl()
- number of supported commands: 25
4. bpf()
- number of supported commands: 18
5. proposed fsconfig()
- number of supported commands: 8
Total Number of required syscalls: 100
That means for bpf() alone Linux would have had to gain *18* additional
single syscalls and for the new mount api only for configuring a mount
context 8 additional syscalls would need to be pulled.
That all hinges on the argument that "syscalls are cheap" and that
running out of syscall numbers is not a real problem because there is a
patchset that lifts this restriction _eventually_. That patchset hasn't
been merged yet and I have not even seen it sent out yet. So we're still
short of syscall numbers. _Even_ if this patchset would have landed,
adding 26 syscalls for two apis seems excessive.
So unless Linus jumps in here (Cced) and says that he's fine that the
pidfd to /proc/<pid>-fd conversion is suited for yet another syscalls
what we have here is perfectly acceptable.
Again, as I've said before I don't see the point in sending piles of
syscalls when it is not really justified and I find none of the
arguments against this implementation we have here right now very
convincing.
Christian
^ permalink raw reply
* Re: [PATCH v2 2/5] pid: add pidfd_open()
From: Jonathan Kowalski @ 2019-03-30 14:51 UTC (permalink / raw)
To: Christian Brauner
Cc: Jürg Billeter, torvalds, Jann Horn, Andy Lutomirski,
David Howells, Serge E. Hallyn, Linux API, linux-kernel,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy
In-Reply-To: <20190330143726.6aaxz4sctu3pzpyx@brauner.io>
On Sat, Mar 30, 2019 at 2:37 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Sat, Mar 30, 2019 at 12:53:57PM +0100, Jürg Billeter wrote:
> > On Fri, 2019-03-29 at 16:54 +0100, Christian Brauner wrote:
> > > diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h
> > > index ac49a220cf2a..d6c7c0701997 100644
> > > --- a/include/uapi/linux/wait.h
> > > +++ b/include/uapi/linux/wait.h
> > > @@ -18,5 +18,7 @@
> > > #define P_PID 1
> > > #define P_PGID 2
> > >
> > > +/* Get a file descriptor for /proc/<pid> of the corresponding pidfd
> > > */
> > > +#define PIDFD_GET_PROCFD _IOR('p', 1, int)
> > >
> > > #endif /* _UAPI_LINUX_WAIT_H */
> >
> > This is missing an entry in Documentation/ioctl/ioctl-number.txt and is
> > actually conflicting with existing entries.
>
> Thanks. Yes, Jann mentioned this too.
>
> >
> > However, I'd actually prefer a syscall to allow strict whitelisting via
> > seccomp and avoid the other ioctl disadvantages that Daniel has already
> > mentioned.
>
> You can filter ioctls with seccomp.
>
You probably wouldn't even need to, because the only way the ioctl
would be useful is to have a dir fd to the procfs root. As such, the
pidfd file descriptor itself is useless with the ioctl. There's also
no filtering to be done, as one pidfd strictly maps to a specific
task, so it's not that you get access to other things than what you
weren't permitted to, and that's pretty neat the way it is. If /proc
is not mounted in its namespace, you'd need to pass it to the process
explicitly, and if it is, then it doesn't matter anyway (even if it
can open /proc, hidepid based restrictions still work -- it's
essentially a race free openat).
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Daniel Colascione @ 2019-03-30 16:08 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Christian Brauner, Jann Horn, Konstantin Khlebnikov,
Andy Lutomirski, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai
In-Reply-To: <CAGLj2rGFOv-AcHSuxF2YBz=WKNi7iLGp3i5KzAq7E1fo14B9Lg@mail.gmail.com>
On Sat, Mar 30, 2019 at 7:30 AM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
>
> On Sat, Mar 30, 2019 at 7:39 AM Daniel Colascione <dancol@google.com> wrote:
> >
> > [SNIP]
> >
> > Thanks again.
> >
> > I agree that the operation we're discussing has a simple signature,
> > but signature flexibility isn't the only reason to prefer a system
> > call over an ioctl. There are other reasons for preferring system
> > calls to ioctls (safety, tracing, etc.) that apply even if the
> > operation we're discussing has a relatively simple signature: for
> > example, every system call has a distinct and convenient ftrace event,
> > but ioctls don't; strace filtering Just Works on a
> > system-call-by-system-call basis, but it doesn't for ioctls; and
>
> It does for those with a unique number.
There's no such thing as a unique ioctl number though. Anyone is free
to create an ioctl with a conflicting number. Sure, you can guarantee
ioctl request number uniqueness within things that ship with the
kernel, but you can also guarantee system call number uniqueness, so
what do we gain by using an ioctl? And yes, you can do *some*
filtering based on ioctl command, but it's frequently more awkward
than the system call equivalent and sometimes doesn't work at all.
What's the strace -e pidfd_open equivalent for an ioctl? Grep isn't
quite equivalent.
> > documentation for system calls is much more discoverable (e.g., man
> > -k) than documentation for ioctls. Even if the distinction doesn't
> > matter much, IMHO, it still matters a little, enough to favor a system
> > call without an offsetting advantage for the ioctl option.
>
> It will be alongside other I/O operations in the pidfd man page.
>
> >
> > > If you start adding a system call for every specific operation on file
> > > descriptors, it *will* become a problem.
> >
> > I'm not sure what you mean. Do you mean that adding a top-level system
> > call for every operation that might apply to one specific kind of file
> > descriptor would lead, as the overall result, to the kernel having
> > enough system calls to cause negative consequences? I'm not sure I
> > agree, but accepting this idea for the sake of discussion: shouldn't
> > we be more okay with system calls for features present on almost all
> > systems --- like procfs --- even if we punt to ioctls very rarely-used
> > functionality, e.g., some hypothetical special squeak noise that you
> > could get some specific 1995 adlib clone to make?
>
> Consider if we were to do:
>
> int procpidfd_open(int pidfd, int procrootfd);
>
> This system call is useless besides a very specific operation for a
> very specific usecase on a file descriptor.
I don't understand this argument based on an operation being "very
specific". Are you suggesting that epoll_wait should have been an
ioctl? I don't see how an operation being specific to a type of file
descriptor is an argument for making that operation an ioctl instead
of a system call.
> Then, you figure you might
> want to support procpidfd back to pidfd (although YAGNI), so you will
> add a CMD or flags argument now,
Why would we need a system call at all? And why are we invoking
hypothetical argument B in an argument against adding operation A as a
system call? B doesn't exist yet. As Christian mentioned, we could
create a named /proc/pid/handle file which, when opened, would yield a
pidfd. More generally, though: if we expose new functionality, we can
make a new system call. Why is that worse than adding a new ioctl
code?
> int procpidfd_open(int pidfd, int procrootfd/procpidfd, unsigned int flags);
>
> int procpidfd = procpidfd_open(fd, procrootfd, PIDFD_TO_PROCPIDFD);
> int pidfd = procpidfd_open(-1, procpidfd, PROCPIDFD_TO_PIDFD);
>
> vs procpidfd2pidfd(int procpidfd); if you did not foresee the need.
> Then, you want pidfd2pid, and so on.
>
> If you don't, you will have to add a command interface in the new
> system call, which changes parameters depending on the flags. This is
> already starting to get ugly.
> In the end, it's a matter of taste,
I don't think it's quite a matter of taste. That idiom suggests that
the two options are roughly functionally equivalent. In this case,
there are clear and specific technical reasons to prefer system calls.
We're not talking about two equivalent approaches. Making the
operation a system call gives users more power, and we shouldn't
deprive them of this power without a good reason. So far, I haven't
seen such a reason.
> this
> pattern if exploited leads to endless proliferation of the system call
> interface, often ridden with short-sighted APIs, because you cannot
> know if you want a focused call or a command style call.
Bad interfaces proliferate no matter what, and there's no difference
in support burden between an ioctl and a system call: both need to
work indefinitely. Are you saying that it's easier to remove a bad
interface if it's an ioctl instead of a system call? I don't recall
any precedent, but maybe I'm wrong.
> ioctl is
> already a command interface, and 10 system calls for each command is
> _NOT_ nice, from a user perspective.
syscall(2) is already a "command" interface, just with more automatic
features than ioctl. What do we gain by pushing the "command" down a
level from the syscall(2) multiplexer to the ioctl(2) multiplexer?
> > > Besides, the translation is
> > > just there because it is racy to do in userspace, it is not some well
> > > defined core kernel functionality.
> > > Therefore, it is just a way to
> > > enter the kernel to do the openat in a race free and safe manner.
> >
> > I agree that the translation has to be done in the kernel, not
> > userspace, and that the kernel must provide to userspace some
> > interface for requesting that the translation happen: we're just
> > discussing the shape of this interface. Shouldn't all interfaces
> > provided by the kernel to userspace be equally well defined? I'm not
> > sure that the internal simplicity of the operation matters much
> > either. There are already explicit system calls for some
> > simple-to-implement things, e.g., timerfd_gettime. It's worth noting
> > that timerfd is (IIRC), like procfs, a feature that's both ubiquitous
> > and optional.
>
> It is well defined, it has a well defined signature, and it will error
> out if you don't use it properly. Again, what it does is very limited
> and niche. I am not sure it warrants a system call of its own.
Do we need to think in terms of whether a bit of functionality
"warrants" a system call? A system call is not expensive to add.
Instead of thinking of whether a function meets some bar for promotion
to a system call, we should be thinking of whether a function meets
the bar for demotion to ioctl. System calls are generally more useful
to users than ioctls, and all things being equal, creating an
interface more pleasant for users should be the priority.
> timerfd_gettime was an afterthought anyway, that's probably not a good
> example (it was more to just match the POSIX timers interface as the
> original timerfd never had support for querying, so the split into two
> steps, create and initialize, you could argue one could do it without
> a syscall, but it still has a well defined argument list, and
> accepting elaborate data structures into an ioctl is not a good
> interface to plumb, so that's easily justified).
TCGETS deals with an elaborate data structure.
In any case, I don't think the elaborate data structure argument is
relevant. The things that the system call mechanism lets you do that
the ioctl mechanism does not are independent of whether a system call
accepts simple or complex argument types. And ioctl and a system call
both specify a bit of kernel mode code to run: the difference is in
identifying that bit of kernel-side code to run and in the features
you get automatically through the call mechanism.
> It's much like socket and setsockopt/getsockopt in nature. I would
> even say APIs separating creation and configuration age well and are
> better, but a process doesn't fit such a model cleanly.
>
> > > As is, the facility being provided through an ioctl on the pidfd is
> > > not something I'd consider a problem.
> >
> > You're right that from a signature perspective, using an ioctl isn't a
> > problem. I just want to make sure we take into account the other,
> > non-signature advantages that system calls have over ioctls.
> >
> > > I think the translation stuff
> > > should also probably be an extension of ioctl_ns(2) (but I wouldn't be
> > > opposed if translate_pid is resurrected as is).
> > > For anything more involved than ioctl(pidfd, PIDFD_TO_PROCFD,
> > > procrootfd), I'd agree that a system call would be a cleaner
> > > interface, otherwise, if you cannot generalise it, using ioctls as a
> > > command interface is probably the better tradeoff here.
> >
> > Sure: I just want to better understand everyone else's thought process
> > here, having been frustrated with things like the termios ioctls being
> > ioctls.
>
> On that note, I don't buy the safety argument here, or the seccomp argument.
>
> You need to consider what this is, on a case by case basis.
>
> ioctl(pidfd, PIDFD_TO_PROCFD, procrootfd);
>
> See? You need /proc's root fd to be able to get to the /proc/<PID> dir
> fd, in a race free manner. You don't need to have the overhead of
> filtering to contain access, as privilege is bound to descriptors.
There's no filtering overhead when you haven't enabled filtering, right?
You seem to be making an argument that we should make this facility an
ioctl because we don't need the features that the system call
mechanism provides --- correct me if I'm wrong. Even if we can't see a
case for filtering or containment or tracing _today_, such a case
might arise in the future, as has happened many times before. Besides,
we haven't touched on the other features we get "for free" when we
make an operation a system call, e.g., explicit tracing support. I
don't understand why we should forestall these uses cases, both
present and future, by using an ioctl instead of a system call,
especially when we get so little in return. I think there's a lot of
value in regularity and consistency and that making some operations
ioctls and some syscalls based on details like whether an operation
accepts a structure or a primitive makes the system less flexible and
understandable.
> [snip further discussion along the same lines]
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-03-30 16:09 UTC (permalink / raw)
To: Christian Brauner
Cc: Jann Horn, Andrew Lutomirski, David Howells, serge, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, bl0pbl33p,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
nagarathnam.muthusamy, Aleksa Sarai, Al Viro,
Joel Fernandes <joel@
In-Reply-To: <20190329155425.26059-1-christian@brauner.io>
On Fri, Mar 29, 2019 at 8:54 AM Christian Brauner <christian@brauner.io> wrote:
>
> /* Introduction */
> This adds the pidfd_open() syscall.
> pidfd_open() allows to retrieve file descriptors for a given pid. This
> includes both file descriptors for processes and file descriptors for
> threads.
I'm ok with the pidfd_open() call to just get a pidfd, but that
"pidfd_to_profs()" needs to go away.
If you want to open the /proc/<pid>/status file, then just do that.
This whole "do one and convert to the other" needs to die. No, no, no.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Daniel Colascione @ 2019-03-30 16:11 UTC (permalink / raw)
To: Linus Torvalds
Cc: Christian Brauner, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy <nag>
In-Reply-To: <CAHk-=wjq4dTPcz-qsvhpm5F42SDHCoqEWv1V=rs_kt6MC=ZThA@mail.gmail.com>
On Sat, Mar 30, 2019 at 9:09 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Mar 29, 2019 at 8:54 AM Christian Brauner <christian@brauner.io> wrote:
> >
> > /* Introduction */
> > This adds the pidfd_open() syscall.
> > pidfd_open() allows to retrieve file descriptors for a given pid. This
> > includes both file descriptors for processes and file descriptors for
> > threads.
>
> I'm ok with the pidfd_open() call to just get a pidfd, but that
> "pidfd_to_profs()" needs to go away.
>
> If you want to open the /proc/<pid>/status file, then just do that.
> This whole "do one and convert to the other" needs to die. No, no, no.
How do you propose that someone open the /proc/<pid>/status file in a
race-free manner starting with the result of calling pidfd_open?
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-03-30 16:16 UTC (permalink / raw)
To: Daniel Colascione
Cc: Christian Brauner, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy <nag>
In-Reply-To: <CAKOZueuDWWYtg9gUpUgTG2k-gVPE1KBEkstr5eP=FC2EzdmaoA@mail.gmail.com>
On Sat, Mar 30, 2019 at 9:11 AM Daniel Colascione <dancol@google.com> wrote:
>
> How do you propose that someone open the /proc/<pid>/status file in a
> race-free manner starting with the result of calling pidfd_open?
CXhrist.
If you want to open a /proc file, then open a /proc file. Don't use
pidfd_open().
Just open /proc/<pid>/
Then you can use openat() to open whatever sub-files you want.
You have two choices: either you use /proc, or you don't. Don't mix the two.
And no, we are *NOT* making pidfd_open() into some "let's expose /proc
even when it's not mounted" horror. Seriously. That's disgusting,
wrong, insecure and stupid.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-03-30 16:18 UTC (permalink / raw)
To: Daniel Colascione
Cc: Christian Brauner, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy <nag>
In-Reply-To: <CAHk-=wjka0W1g3asQp2H=5ot=kkS-Mp+Cx6KHr0rNkGFfCOLYA@mail.gmail.com>
On Sat, Mar 30, 2019 at 9:16 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And no, we are *NOT* making pidfd_open() into some "let's expose /proc
> even when it's not mounted" horror. Seriously. That's disgusting,
> wrong, insecure and stupid.
And btw, this is not debatable. In fact, this whole discussion is
making me feel like I should just revert pidfd, not because the code I
merged is wrong, but because people are clearly intending to do
completely inappropriate things with this.
Get your act together. Stop hacking up garbage.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-30 16:19 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jann Horn, Andrew Lutomirski, David Howells, serge, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, bl0pbl33p,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
nagarathnam.muthusamy, Aleksa Sarai, Al Viro,
Joel Fernandes <joel@
In-Reply-To: <CAHk-=wjq4dTPcz-qsvhpm5F42SDHCoqEWv1V=rs_kt6MC=ZThA@mail.gmail.com>
On March 30, 2019 5:09:10 PM GMT+01:00, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Fri, Mar 29, 2019 at 8:54 AM Christian Brauner
><christian@brauner.io> wrote:
>>
>> /* Introduction */
>> This adds the pidfd_open() syscall.
>> pidfd_open() allows to retrieve file descriptors for a given pid.
>This
>> includes both file descriptors for processes and file descriptors for
>> threads.
>
>I'm ok with the pidfd_open() call to just get a pidfd, but that
From pure API perspective that's all I care about: independence of procfs.
Once we have pidfd_open() we can cleanly signal threads etc.
>"pidfd_to_profs()" needs to go away.
>
>If you want to open the /proc/<pid>/status file, then just do that.
>This whole "do one and convert to the other" needs to die. No, no, no.
>
> Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-03-30 16:24 UTC (permalink / raw)
To: Christian Brauner
Cc: Jann Horn, Andrew Lutomirski, David Howells, Serge E. Hallyn,
Linux API, Linux List Kernel Mailing, Arnd Bergmann,
Eric W. Biederman, Konstantin Khlebnikov, Kees Cook,
Alexey Dobriyan, Thomas Gleixner, Michael Kerrisk-manpages,
Jonathan Kowalski, Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy, Aleksa Sarai <cyph>
In-Reply-To: <BCD99947-5F55-4116-8F74-D6832E50340B@brauner.io>
On Sat, Mar 30, 2019 at 9:19 AM Christian Brauner <christian@brauner.io> wrote:
>
> From pure API perspective that's all I care about: independence of procfs.
> Once we have pidfd_open() we can cleanly signal threads etc.
But "independence from procfs" means that you damn well don't then do
"oh, now I have a pidfd, I want to turn it into a /proc fd and then
munge around there".
So I'm literally saying that it had better really *be* independent
from /proc. It is the standalone version, but it's most definitely
also the version that doesn't then give you secret access to /proc.
And it weorries me a lot that people are trying to play these kinds of
games. I'm just seeing some android patch that adds this horror and
then starts using it.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Daniel Colascione @ 2019-03-30 16:34 UTC (permalink / raw)
To: Linus Torvalds
Cc: Christian Brauner, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy <nag>
In-Reply-To: <CAHk-=whcA0kAWGqet9oJdyttjhP7J4PcU2Ly=n_Qyo6rMJNxRg@mail.gmail.com>
On Sat, Mar 30, 2019 at 9:24 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sat, Mar 30, 2019 at 9:19 AM Christian Brauner <christian@brauner.io> wrote:
> >
> > From pure API perspective that's all I care about: independence of procfs.
> > Once we have pidfd_open() we can cleanly signal threads etc.
>
> But "independence from procfs" means that you damn well don't then do
> "oh, now I have a pidfd, I want to turn it into a /proc fd and then
> munge around there".
>
> So I'm literally saying that it had better really *be* independent
> from /proc. It is the standalone version, but it's most definitely
> also the version that doesn't then give you secret access to /proc.
Just to be clear, I'm not proposing granting secret access to procfs,
and as far as I can see, nobody else is either. We've been talking
about making it easier to avoid races when you happen to want a pidfd
and a procfs fd that point to the same process, not granting access
that you didn't have before. If you'd rather not connect procfs and
pidfds, we can take this functionality off the table.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-30 16:37 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jann Horn, Andrew Lutomirski, David Howells, Serge E. Hallyn,
Linux API, Linux List Kernel Mailing, Arnd Bergmann,
Eric W. Biederman, Konstantin Khlebnikov, Kees Cook,
Alexey Dobriyan, Thomas Gleixner, Michael Kerrisk-manpages,
Jonathan Kowalski, Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy, Aleksa Sarai <cyph>
In-Reply-To: <CAHk-=whcA0kAWGqet9oJdyttjhP7J4PcU2Ly=n_Qyo6rMJNxRg@mail.gmail.com>
On Sat, Mar 30, 2019 at 09:24:23AM -0700, Linus Torvalds wrote:
> On Sat, Mar 30, 2019 at 9:19 AM Christian Brauner <christian@brauner.io> wrote:
> >
> > From pure API perspective that's all I care about: independence of procfs.
> > Once we have pidfd_open() we can cleanly signal threads etc.
>
> But "independence from procfs" means that you damn well don't then do
> "oh, now I have a pidfd, I want to turn it into a /proc fd and then
> munge around there".
>
> So I'm literally saying that it had better really *be* independent
> from /proc. It is the standalone version, but it's most definitely
> also the version that doesn't then give you secret access to /proc.
>
> And it weorries me a lot that people are trying to play these kinds of
> games. I'm just seeing some android patch that adds this horror and
> then starts using it.
The original need for this conversion thing came from Android indeed. I
won't go forward with any such patches.
Christian
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-30 16:38 UTC (permalink / raw)
To: Daniel Colascione
Cc: Linus Torvalds, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam
In-Reply-To: <CAKOZuevv_QgvzmusA+yey76vo4hn9bEVwud2RumuOr0K4++15A@mail.gmail.com>
On Sat, Mar 30, 2019 at 09:34:02AM -0700, Daniel Colascione wrote:
> On Sat, Mar 30, 2019 at 9:24 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Sat, Mar 30, 2019 at 9:19 AM Christian Brauner <christian@brauner.io> wrote:
> > >
> > > From pure API perspective that's all I care about: independence of procfs.
> > > Once we have pidfd_open() we can cleanly signal threads etc.
> >
> > But "independence from procfs" means that you damn well don't then do
> > "oh, now I have a pidfd, I want to turn it into a /proc fd and then
> > munge around there".
> >
> > So I'm literally saying that it had better really *be* independent
> > from /proc. It is the standalone version, but it's most definitely
> > also the version that doesn't then give you secret access to /proc.
>
> Just to be clear, I'm not proposing granting secret access to procfs,
> and as far as I can see, nobody else is either. We've been talking
> about making it easier to avoid races when you happen to want a pidfd
> and a procfs fd that point to the same process, not granting access
> that you didn't have before. If you'd rather not connect procfs and
> pidfds, we can take this functionality off the table.
This is dead! Nothing like this will make it through this tree. I have
no intention of endangering pidfd_send_signal().
Christian
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-03-30 17:04 UTC (permalink / raw)
To: Daniel Colascione
Cc: Christian Brauner, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy <nag>
In-Reply-To: <CAKOZuevv_QgvzmusA+yey76vo4hn9bEVwud2RumuOr0K4++15A@mail.gmail.com>
On Sat, Mar 30, 2019 at 9:34 AM Daniel Colascione <dancol@google.com> wrote:
>
> Just to be clear, I'm not proposing granting secret access to procfs,
> and as far as I can see, nobody else is either. We've been talking
> about making it easier to avoid races when you happen to want a pidfd
> and a procfs fd that point to the same process
So I thought that was the whole point of just opening /proc/<pid>.
Exactly because that way you can then use openat() from there on.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-30 17:12 UTC (permalink / raw)
To: Linus Torvalds
Cc: Daniel Colascione, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy <nagara>
In-Reply-To: <CAHk-=wj0o8BiUrYiM5ZcTS6JxBh1M60O36Ms8-EqLQ139=1L4g@mail.gmail.com>
On Sat, Mar 30, 2019 at 10:04:33AM -0700, Linus Torvalds wrote:
> On Sat, Mar 30, 2019 at 9:34 AM Daniel Colascione <dancol@google.com> wrote:
> >
> > Just to be clear, I'm not proposing granting secret access to procfs,
> > and as far as I can see, nobody else is either. We've been talking
> > about making it easier to avoid races when you happen to want a pidfd
> > and a procfs fd that point to the same process
>
> So I thought that was the whole point of just opening /proc/<pid>.
> Exactly because that way you can then use openat() from there on.
To clarify, what the Android guys really wanted to be part of the api is
a way to get race-free access to metadata associated with a given pidfd.
And the idea was that *if and only if procfs is mounted* you could do:
int pidfd = pidfd_open(1234, 0);
int procfd = open("/proc", O_RDONLY | O_CLOEXEC);
int procpidfd = ioctl(pidfd, PIDFD_TO_PROCFD, procfd);
and then we internally verify that the struct pid that the pidfd is
refering to, is still the same as the one that /proc/<pid> is refering
to and only then do we return an fd for the process /proc/<pid>
directory which would then allow you to do, e.g.:
int statusfd = openat(procpidfd, "status", O_RDONLY | O_CLOEXEC);
this would provide race-free access to metadat but again, only if /proc
is mounted and available to the user. But if that's an instant NAK we
will definitely *not* do this.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-03-30 17:24 UTC (permalink / raw)
To: Christian Brauner
Cc: Daniel Colascione, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy <nagara>
In-Reply-To: <20190330171215.3yrfxwodstmgzmxy@brauner.io>
On Sat, Mar 30, 2019 at 10:12 AM Christian Brauner <christian@brauner.io> wrote:
>
>
> To clarify, what the Android guys really wanted to be part of the api is
> a way to get race-free access to metadata associated with a given pidfd.
> And the idea was that *if and only if procfs is mounted* you could do:
>
> int pidfd = pidfd_open(1234, 0);
>
> int procfd = open("/proc", O_RDONLY | O_CLOEXEC);
> int procpidfd = ioctl(pidfd, PIDFD_TO_PROCFD, procfd);
And my claim is that this is three system calls - one of them very
hacky - to just do
int pidfd = open("/proc/%d", O_PATH);
and you're done. It acts as the pidfd _and_ the way to get the
associated status files etc.
So there is absolutely zero advantage to going through pidfd_open().
No. No. No.
So the *only* reason for "pidfd_open()" is if you don't have /proc in
the first place. In which case the whole PIDFD_TO_PROCFD is bogus.
Yeah, yeah, if you want to avoid going through the pathname
translation, that's one thing, but if that's your aim, then you again
should also just admit that PIDFD_TO_PROCFD is disgusting and wrong,
and you're basically saying "ok, I'm not going to do /proc at all".
So I'm ok with the whole "simpler, faster, no-proc pidfd", but then it
really has to be *SIMPLER* and *NO PROCFS*.
PIDFD_TO_PROCFD violates *everything*.
Linus
^ 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