* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-12-01 16:27 UTC (permalink / raw)
To: Andy Lutomirski, Eric W. Biederman
Cc: Arnd Bergmann, Andy Lutomirski, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net>
On December 2, 2018 4:52:37 AM GMT+13:00, Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>> On Dec 1, 2018, at 7:28 AM, Eric W. Biederman <ebiederm@xmission.com>
>wrote:
>>
>>
>> It just occurs to me that the simple way to implement
>> procfd_sigqueueinfo info is like:
>>
>> int copy_siginfo_from_user_any(kernel_siginfo_t *info, siginfo_t
>*uinfo)
>> {
>> #ifdef CONFIG_COMPAT
>> if (in_compat_syscall)
>> return copy_siginfo_from_user32(info, uinfo);
>> #endif
>> return copy_siginfo_from_user(info, uinfo);
>
>> }
>>
>> long procfd_sigqueueinfo(int fd, siginfo_t *uinfo)
>> {
>> kernel_siginfo info;
>>
>> if (copy_siginfo_from_user_any(&info, uinfo))
>> return -EFAULT;
>> ...;
>> }
>>
>> It looks like there is already a place in ptrace.c that already
>> hand rolls copy_siginfo_from_user_any.
>>
>> So while I would love to figure out the subset of siginfo_t tha we
>can
>> just pass through, as I think that would make a better more forward
>> compatible copy_siginfo_from_user32.
>
>Seems reasonable to me. It’s less code overall than any other
>suggestion, too.
Thanks everyone, that was super helpful!
All things equal I'm going to send out an
updated version of the patch latest next week!
>
>> I think for this use case we just
>> add the in_compat_syscall test and then we just need to ensure this
>new
>> system call is placed in the proper places in the syscall table.
>>
>> Because we will need 3 call sights: x86_64, x32 and ia32. As the
>layout
>> changes between those three subarchitecuters.
>>
>>
>
>If it’s done this way, it can just be “common” in the 64-bit table. And
>we kick the can a bit farther down the road :)
>
>I’m working on patches to clean up x86’s syscall mess. It’s slow
>because I keep finding new messes. So far I have rt_sigreturn working
>like every other syscall — whee.
>
>Also, Eric, for your edification, I have a draft patch set to radically
>simplify x86’s signal delivery and return. Once that’s done, I can
>trivially speed up delivery by a ton by using sysret.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Andy Lutomirski @ 2018-12-01 15:52 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Arnd Bergmann, christian, Andy Lutomirski, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <87tvjxp8pc.fsf@xmission.com>
> On Dec 1, 2018, at 7:28 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
>
> It just occurs to me that the simple way to implement
> procfd_sigqueueinfo info is like:
>
> int copy_siginfo_from_user_any(kernel_siginfo_t *info, siginfo_t *uinfo)
> {
> #ifdef CONFIG_COMPAT
> if (in_compat_syscall)
> return copy_siginfo_from_user32(info, uinfo);
> #endif
> return copy_siginfo_from_user(info, uinfo);
> }
>
> long procfd_sigqueueinfo(int fd, siginfo_t *uinfo)
> {
> kernel_siginfo info;
>
> if (copy_siginfo_from_user_any(&info, uinfo))
> return -EFAULT;
> ...;
> }
>
> It looks like there is already a place in ptrace.c that already
> hand rolls copy_siginfo_from_user_any.
>
> So while I would love to figure out the subset of siginfo_t tha we can
> just pass through, as I think that would make a better more forward
> compatible copy_siginfo_from_user32.
Seems reasonable to me. It’s less code overall than any other suggestion, too.
> I think for this use case we just
> add the in_compat_syscall test and then we just need to ensure this new
> system call is placed in the proper places in the syscall table.
>
> Because we will need 3 call sights: x86_64, x32 and ia32. As the layout
> changes between those three subarchitecuters.
>
>
If it’s done this way, it can just be “common” in the 64-bit table. And we kick the can a bit farther down the road :)
I’m working on patches to clean up x86’s syscall mess. It’s slow because I keep finding new messes. So far I have rt_sigreturn working like every other syscall — whee.
Also, Eric, for your edification, I have a draft patch set to radically simplify x86’s signal delivery and return. Once that’s done, I can trivially speed up delivery by a ton by using sysret.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Eric W. Biederman @ 2018-12-01 15:28 UTC (permalink / raw)
To: Arnd Bergmann
Cc: christian, Andy Lutomirski, Andy Lutomirski, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <87y399s3sc.fsf@xmission.com>
It just occurs to me that the simple way to implement
procfd_sigqueueinfo info is like:
int copy_siginfo_from_user_any(kernel_siginfo_t *info, siginfo_t *uinfo)
{
#ifdef CONFIG_COMPAT
if (in_compat_syscall)
return copy_siginfo_from_user32(info, uinfo);
#endif
return copy_siginfo_from_user(info, uinfo);
}
long procfd_sigqueueinfo(int fd, siginfo_t *uinfo)
{
kernel_siginfo info;
if (copy_siginfo_from_user_any(&info, uinfo))
return -EFAULT;
...;
}
It looks like there is already a place in ptrace.c that already
hand rolls copy_siginfo_from_user_any.
So while I would love to figure out the subset of siginfo_t tha we can
just pass through, as I think that would make a better more forward
compatible copy_siginfo_from_user32. I think for this use case we just
add the in_compat_syscall test and then we just need to ensure this new
system call is placed in the proper places in the syscall table.
Because we will need 3 call sights: x86_64, x32 and ia32. As the layout
changes between those three subarchitecuters.
Eric
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Eric W. Biederman @ 2018-12-01 14:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: christian, Andy Lutomirski, Andy Lutomirski, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <CAK8P3a0kqPii5TwFAo_JHLX=o_FDMFVKXxgzzbDjLFZ7OQ5QCQ@mail.gmail.com>
Arnd Bergmann <arnd@arndb.de> writes:
> On Fri, Nov 30, 2018 at 7:56 AM Christian Brauner <christian@brauner.io> wrote:
>> On Thu, Nov 29, 2018 at 11:13:57PM -0600, Eric W. Biederman wrote:
>> > Arnd Bergmann <arnd@arndb.de> writes:
>> > > On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski <luto@amacapital.net> wrote:
>> > >
>> > > It looks like we already have a 'struct signalfd_siginfo' that is defined in a
>> > > sane architecture-independent way, so I'd suggest we use that.
>> >
>> > Unfortunately it isn't maintained very well. What you can
>> > express with signalfd_siginfo is a subset what you can express with
>> > siginfo. Many of the linux extensions to siginfo for exception
>> > information add pointers and have integers right after those pointers.
>> > Not all of those linux specific extentions for exceptions are handled
>> > by signalfd_siginfo (it needs new fields).
>
> Would those fit in the 30 remaining padding bytes?
Probably. I expect at some point the technique signalfd has been using
won't scale. Most of what are missing are synchronous exceptions but
the crazy seccomp extensions might be missing as well. I say crazy
because seccomp places an integer immediately after a pointer which
makes 32bit 64bit compatibility impossible.
>> > As originally defined siginfo had the sigval_t union at the end so it
>> > was perfectly fine on both 32bit and 64bit as it only had a single
>> > pointer in the structure and the other fields were 32bits in size.
>
> The problem with sigval_t of course is that it is incompatible between
> 32-bit and 64-bit. We can add the same information, but at least on
> the syscall level that would have to be a __u64.
But sigval_t with nothing after it is not incompatible between 32bit and
64bit. With nothing after it sigval_t in struct siginfo already has
the extra 32bits you would need. It is sort of like transparently
adding a __u64 member to the union.
That is where we really are with sigval_t. Except for some crazy
corner cases.
>> > Although I do feel the pain as x86_64 has to deal with 3 versions
>> > of siginfo. A 64bit one. A 32bit one for ia32. A 32bit one for x32
>> > with a 64bit si_clock_t.
>
> At least you and Al have managed to get it down to a single source-level
> definition across all architectures, but there is also the (lesser) problem
> that the structure has a slightly different binary layout on each of the
> classic architectures.
>
> If we go with Andy's suggestion of having only a single binary layout
> on x86 for the new call, I'd argue that we should also make it have
> the exact same layout on all other architectures.
Yes.
>> > > We may then also want to make sure that any system call that takes a
>> > > siginfo has a replacement that takes a signalfd_siginfo, and that this
>> > > replacement can be used to implement the old version purely in
>> > > user space.
>> >
>> > If you are not implementing CRIU and reinserting exceptions to yourself.
>> > At most user space wants the ability to implement sigqueue:
>> >
>> > AKA:
>> > sigqueue(pid_t pid, int sig, const union sigval value);
>> >
>> > Well sigqueue with it's own si_codes so the following would cover all
>> > the use cases I know of:
>> > int sendnewsig(pid_t pid, int sig, int si_code, const union sigval value);
>> >
>> > The si_code could even be set to SI_USER to request that the normal
>> > trusted SI_USER values be filled in. si_code values of < 0 if not
>> > recognized could reasonably safely be treated as the _rt member of
>> > the siginfo union. Or perhaps better we error out in such a case.
>> >
>> > If we want to be flexible and not have N kinds of system calls that
>> > is the direction I would go. That is simple, and you don't need any of
>> > the rest.
>
> I'm not sure I understand what you are suggesting here. Would the
> low-level implementation of this be based on procfd, or do you
> mean that would be done for pid_t at the kernel level, plus another
> syscall for procfd?
I was thinking in general and for something that would be enough to back
sigqueue, and normal kill semantics.
For Christians proposed system call I can think of two ways we could go:
long procfd_sigsend(int fd, int sig, int si_code, __u64 sigval);
Where sigval is an appropriately written version of union sigval that
works on both 32bit and 64bit. Or we could go with:
long procfd_sigqueueinfo(int fd, struct siginfo_subset *info)
Both would support the same set of system calls today and both would
have some similar challenges. Both would take an si_code value of
SI_USER as a request that the kernel fill in the struct siginfo as kill
would. As otherwise it is invalid for userspace to send a signal with
SI_USER. I would not worry about the signal injection case for CRIU
where we explicitly allow any siginfo to be sent if we are sending it to
ourselves. We already have a system call for that.
What I would do is carefully define siginfo_subset architecturally as
a proper subset of siginfo on each architecture. That in most if not
all cases will work on 32bit and 64bit. Because of the extra padding
after union sigval.
That siginfo_subset userspace could just consider a siginfo_t. We would
just leave out the trouble some bits. So we could do a simple
untranslated copy_from_user on it, and then validate that si_code is in
a range we support.
That definition of siginfo_subset doesn't preclude extensions in the
future. And so it could very much be a pass through interface on 32bit
on 64bit and into everyone's existing stack frames that embed siginfo.
So if we want to fix the situation that is what I would do, as userspace
is not allowed to send any of the problematic siginfo definitions today.
There might be some finicky detail of union sigval or some architure
might have funny alignment between 32bit and 64bit, but I don't think
there is. If we want to be more flexible than my procfd_sigsend above
I think we should definitely look at that.
>> > Alternatively we abandon the mistake of sigqueueinfo and not allow
>> > a signal number in the arguments that differs from the si_signo in the
>> > siginfo and allow passing the entire thing unchanged from sender to
>> > receiver. That is maximum flexibility.
>
> This would be regardless of which flavor of siginfo (today's arch
> specific one, signalfd_siginfo, or a new one) we pass, right?
No. signalfd_siginfo offers much less future extensibility because it
must be translated. Once you can not just pass the structure through
you fail. As you can't be transparently forward compatible.
In a similar way Linux filesystems written assuming all the world was
ascii wound up forward compatible with utf8 as they are both byte
encodings of strings that can use a NULL terminator.
>> > signalfd_siginfo just sucks in practice. It is larger that siginfo 104
>> > bytes versus 48. We must deliver it to userspace as a siginfo so it
>> > must be translated. Because of the translation signalfd_siginfo adds
>> > no flexiblity in practice, because it can not just be passed through.
>> > Finallay signalfd_siginfo does not have encodings for all of the
>> > siginfo union members, so it fails to be fully general.
>> >
>> > Personally if I was to define signalfd_siginfo today I would make it:
>> > struct siginfo_subset {
>> > __u32 sis_signo;
>> > __s32 sis_errno;
>> > __s32 sis_code;
>> > __u32 sis_pad;
>> > __u32 sis_pid;
>> > __u32 sis_uid;
>> > __u64 sis_data (A pointer or integer data field);
>> > };
>> >
>> > That is just 32bytes, and is all that is needed for everything
>> > except for synchronous exceptions. Oh and that happens to be a proper
>> > subset of a any sane siginfo layout, on both 32bit and 64bit.
>
> And that would work for signalfd and waitid_time64, but not for
> procfd_signal/kill/sendsiginfo?
No. I was bringing this up because it will work for procfd_signal. See
above. It supports every signal userspace can send today. We should be
able to specify it in a way that is compatible with the existing siginfo
definitions on all architectures.
For signalfd or let me say a future signalfd2 the practical problem is
that we have Linux specific signal extensions that are not synchronous
signals like seccomp and sigchld that have fields in a bad location.
If we successfully define a siginfo_subset that is the same on
32bit and 64bit I would include the defintion in the kernels definition
of siginfo_t so new extensions are automatically picked up and require
any extensions to siginfo_t to be added to siginfo_subset_t, in the
appropriate 32bit/64bit way.
>> > This is one of those rare times where POSIX is sane and what linux
>> > has implemented is not.
>>
>> Thanks for the in-depth explanation. So your point is that we are better
>> off if we stick with siginfo_t instead of struct signalfd_siginfo in
>> procfd_signal()?
My point is that we are better sticking with siginfo_t or just cherry
picking the relevant fields and making them system call parameters.
The only advantage of using siginfo_t is forwards compatibility. If we
give up forward compatibility we can just as easily have our system call
prototype include "...(..., int sig, int code, union sigval value)"
for the definition of the signal bits. Simple easy and enough for all
posix signals.
Eric
^ permalink raw reply
* Re: [PATCH v2 1/1] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Jürg Billeter @ 2018-12-01 13:57 UTC (permalink / raw)
To: Florian Weimer
Cc: Andrew Morton, Oleg Nesterov, Thomas Gleixner, Eric Biederman,
Kees Cook, Andy Lutomirski, linux-api, linux-kernel
In-Reply-To: <878t19o2h6.fsf@oldenburg.str.redhat.com>
On Sat, 2018-12-01 at 13:28 +0100, Florian Weimer wrote:
> * Jürg Billeter:
>
> > On Fri, 2018-11-30 at 14:40 +0100, Florian Weimer wrote:
> > > * Jürg Billeter:
> > >
> > > > This introduces a new thread group flag that can be set by calling
> > > >
> > > > prctl(PR_SET_KILL_DESCENDANTS_ON_EXIT, 1, 0, 0, 0)
> > > >
> > > > When a thread group exits with this flag set, it will send SIGKILL to
> > > > all descendant processes. This can be used to prevent stray child
> > > > processes.
> > > >
> > > > This flag is cleared on privilege gaining execve(2) to ensure an
> > > > unprivileged process cannot get a privileged process to send SIGKILL.
> > >
> > > So this is inherited across regular execve? I'm not sure that's a good
> > > idea.
> >
> > Yes, this matches PR_SET_CHILD_SUBREAPER (and other process
> > attributes). Besides consistency and allowing a parent to configure the
> > flag for a spawned process, this is also needed to prevent a process
> > from clearing the flag (in combination with a seccomp filter).
>
> I think the semantics of PR_SET_CHILD_SUBREAPER are different, and the
> behavior makes more sense there.
In my opinion, introducing inconsistency by deviating from the common
behavior of retaining process attributes across execve would be more
confusing/surprising to users. I don't see why it makes sense for
PR_SET_CHILD_SUBREAPER but not for PR_SET_KILL_DESCENDANTS_ON_EXIT.
Also, the main motivation is to provide a subset of PID namespace
features to unprivileged processes with a lightweight mechanism.
Retaining kill_descendants_on_exit across execve allows very similar
usage to PID namespaces: E.g., the parent can set
PR_SET_KILL_DESCENDANTS_ON_EXIT and PR_SET_CHILD_SUBREAPER in the child
before execve and the spawned init-like executable doesn't need to know
about this flag itself, i.e., the same init-like program can function
as a leader of a PID namespace or as a subreaper with this extra flag
set without code changes.
If the flag was cleared by execve, the program would need to know about
this flag and it would be impossible for the parent to lock this down
using seccomp.
>
> > > > Descendants that are orphaned and reparented to an ancestor of the
> > > > current process before the current process exits, will not be killed.
> > > > PR_SET_CHILD_SUBREAPER can be used to contain orphaned processes.
> > >
> > > For double- or triple-forking daemons, the reparenting will be racy, if
> > > I understand things correctly.
> >
> > Can you please elaborate, if you're concerned about a particular race?
> > As the commit message mentions, for containment this flag can be
> > combined with PR_SET_CHILD_SUBREAPER (and PR_SET_NO_NEW_PRIVS).
>
> Without PR_SET_CHILD_SUBREAPER, if a newly execve'ed daemon performs
> double/triple forking to disentangle itself from the parent process
> session, and the parent process which set
> PR_SET_KILL_DESCENDANTS_ON_EXIT terminates, behavior depends on when
> exactly the parent process terminates. The daemon process will leak if
> it has completed its reparenting.
>
> I think this could be sufficiently common that solution is needed here.
I expect the common case to be that PR_SET_KILL_DESCENDANTS_ON_EXIT
will be used together with PR_SET_CHILD_SUBREAPER (and possibly
PR_SET_NO_NEW_PRIVS) to prevent stray children. And I don't see a race
condition in that case.
PR_SET_KILL_DESCENDANTS_ON_EXIT can be used for non-subreapers but I
expect this to be used in more specialized scenarios where the program
is designed/known to avoid such race conditions. We could theoretically
restrict PR_SET_KILL_DESCENDANTS_ON_EXIT to subreapers but I currently
don't see a strong enough reason for this.
Jürg
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Eric W. Biederman @ 2018-12-01 13:41 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Arnd Bergmann, Christian Brauner, Florian Weimer, 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: <CALCETrW2aphWwEY4=hUwo_JBCvkQyMjxzxGd9FCW017kMLaMOQ@mail.gmail.com>
Andy Lutomirski <luto@kernel.org> writes:
> On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> siginfo_t as it is now still has a number of other downsides, and Andy in
>> particular didn't like the idea of having three new variants on x86
>> (depending on how you count). His alternative suggestion of having
>> a single syscall entry point that takes a 'signfo_t __user *' but interprets
>> it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
>> should work correctly, but feels wrong to me, or at least inconsistent
>> with how we do this elsewhere.
>
> BTW, do we consider siginfo_t to be extensible? If so, and if we pass
> in a pointer, presumably we should pass a length as well.
siginfo is extensible in the sense that the structure is 128 bytes
and we use at most 48 bytes. siginfo gets embedded in stack frames when
signals get delivered so a size change upwards is non-trivial, and is
possibly and ABI break so I believe a length field would be pointless.
Eric
^ permalink raw reply
* Re: [PATCH v2 1/1] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Florian Weimer @ 2018-12-01 12:28 UTC (permalink / raw)
To: Jürg Billeter
Cc: Andrew Morton, Oleg Nesterov, Thomas Gleixner, Eric Biederman,
Kees Cook, Andy Lutomirski, linux-api, linux-kernel
In-Reply-To: <e92ab3c06fe42fc9872261999623be19e2b0a294.camel@bitron.ch>
* Jürg Billeter:
> On Fri, 2018-11-30 at 14:40 +0100, Florian Weimer wrote:
>> * Jürg Billeter:
>>
>> > This introduces a new thread group flag that can be set by calling
>> >
>> > prctl(PR_SET_KILL_DESCENDANTS_ON_EXIT, 1, 0, 0, 0)
>> >
>> > When a thread group exits with this flag set, it will send SIGKILL to
>> > all descendant processes. This can be used to prevent stray child
>> > processes.
>> >
>> > This flag is cleared on privilege gaining execve(2) to ensure an
>> > unprivileged process cannot get a privileged process to send SIGKILL.
>>
>> So this is inherited across regular execve? I'm not sure that's a good
>> idea.
>
> Yes, this matches PR_SET_CHILD_SUBREAPER (and other process
> attributes). Besides consistency and allowing a parent to configure the
> flag for a spawned process, this is also needed to prevent a process
> from clearing the flag (in combination with a seccomp filter).
I think the semantics of PR_SET_CHILD_SUBREAPER are different, and the
behavior makes more sense there.
>> > Descendants that are orphaned and reparented to an ancestor of the
>> > current process before the current process exits, will not be killed.
>> > PR_SET_CHILD_SUBREAPER can be used to contain orphaned processes.
>>
>> For double- or triple-forking daemons, the reparenting will be racy, if
>> I understand things correctly.
>
> Can you please elaborate, if you're concerned about a particular race?
> As the commit message mentions, for containment this flag can be
> combined with PR_SET_CHILD_SUBREAPER (and PR_SET_NO_NEW_PRIVS).
Without PR_SET_CHILD_SUBREAPER, if a newly execve'ed daemon performs
double/triple forking to disentangle itself from the parent process
session, and the parent process which set
PR_SET_KILL_DESCENDANTS_ON_EXIT terminates, behavior depends on when
exactly the parent process terminates. The daemon process will leak if
it has completed its reparenting.
I think this could be sufficiently common that solution is needed here.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v2 1/1] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Jürg Billeter @ 2018-12-01 10:39 UTC (permalink / raw)
To: Florian Weimer
Cc: Andrew Morton, Oleg Nesterov, Thomas Gleixner, Eric Biederman,
Kees Cook, Andy Lutomirski, linux-api, linux-kernel
In-Reply-To: <87bm66u1j5.fsf@oldenburg.str.redhat.com>
On Fri, 2018-11-30 at 14:40 +0100, Florian Weimer wrote:
> * Jürg Billeter:
>
> > This introduces a new thread group flag that can be set by calling
> >
> > prctl(PR_SET_KILL_DESCENDANTS_ON_EXIT, 1, 0, 0, 0)
> >
> > When a thread group exits with this flag set, it will send SIGKILL to
> > all descendant processes. This can be used to prevent stray child
> > processes.
> >
> > This flag is cleared on privilege gaining execve(2) to ensure an
> > unprivileged process cannot get a privileged process to send SIGKILL.
>
> So this is inherited across regular execve? I'm not sure that's a good
> idea.
Yes, this matches PR_SET_CHILD_SUBREAPER (and other process
attributes). Besides consistency and allowing a parent to configure the
flag for a spawned process, this is also needed to prevent a process
from clearing the flag (in combination with a seccomp filter).
>
> > Descendants that are orphaned and reparented to an ancestor of the
> > current process before the current process exits, will not be killed.
> > PR_SET_CHILD_SUBREAPER can be used to contain orphaned processes.
>
> For double- or triple-forking daemons, the reparenting will be racy, if
> I understand things correctly.
Can you please elaborate, if you're concerned about a particular race?
As the commit message mentions, for containment this flag can be
combined with PR_SET_CHILD_SUBREAPER (and PR_SET_NO_NEW_PRIVS).
Jürg
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Arnd Bergmann @ 2018-12-01 10:27 UTC (permalink / raw)
To: Andy Lutomirski
Cc: christian, Eric W . Biederman, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <CAK8P3a1OX1Hb17=NbTYqZxgEM-sk5-dh_VeKa0bXJpq=k=KxHA@mail.gmail.com>
On Sat, Dec 1, 2018 at 9:51 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Sat, Dec 1, 2018 at 12:54 AM Andy Lutomirski <luto@kernel.org> wrote:
> > On Fri, Nov 30, 2018 at 2:10 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski <luto@kernel.org> wrote:
> > > > On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > siginfo_t as it is now still has a number of other downsides, and Andy in
> > > > > particular didn't like the idea of having three new variants on x86
> > > > > (depending on how you count). His alternative suggestion of having
> > > > > a single syscall entry point that takes a 'signfo_t __user *' but interprets
> > > > > it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
> > > > > should work correctly, but feels wrong to me, or at least inconsistent
> > > > > with how we do this elsewhere.
>
> > > The '548 | 0x40000000' part seems to be the only sensible
> > > way to handle x32 here. What exactly would you propose to
> > > avoid defining the other entry points?
> >
> > I would propose that it should be 335 | 0x40000000. I can't see any
> > reasonable way to teach the kernel to reject 335 | 0x40000000 that
> > wouldn't work just as well to accept it and make it do the right
> > thing. Currently we accept it and do the *wrong* thing, which is no
> > good.
I guess we could start with something like the change below, which
would unify the entry points for rt_{tg,}sigqueueinfo, so that
e.g. the 129 and 536 syscall numbers do the exact same thing, and
that would be the lp64 or ilp32 behavior, depending on the
0x40000000 bit. For the new syscalls, we can then do the same
thing without assigning another number.
Arnd
diff --git a/arch/x86/entry/syscall_64.c b/arch/x86/entry/syscall_64.c
index d5252bc1e380..3233fb889a51 100644
--- a/arch/x86/entry/syscall_64.c
+++ b/arch/x86/entry/syscall_64.c
@@ -7,6 +7,11 @@
#include <asm/asm-offsets.h>
#include <asm/syscall.h>
+#ifdef CONFIG_X86_X32_ABI
+#define __x64_sys_x86_rt_sigqueueinfo __x64_sys_rt_sigqueueinfo
+#define __x64_sys_x86_rt_tgsigqueueinfo __x64_sys_rt_tgsigqueueinfo
+#endif
+
/* this is a lie, but it does not hurt as sys_ni_syscall just returns
-EINVAL */
extern asmlinkage long sys_ni_syscall(const struct pt_regs *);
#define __SYSCALL_64(nr, sym, qual) extern asmlinkage long sym(const
struct pt_regs *);
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl
b/arch/x86/entry/syscalls/syscall_64.tbl
index 0823eed2b02e..4a7393d34e03 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -137,7 +137,7 @@
126 common capset __x64_sys_capset
127 64 rt_sigpending __x64_sys_rt_sigpending
128 64 rt_sigtimedwait __x64_sys_rt_sigtimedwait
-129 64 rt_sigqueueinfo __x64_sys_rt_sigqueueinfo
+129 64 rt_sigqueueinfo __x64_sys_x86_rt_sigqueueinfo
130 common rt_sigsuspend __x64_sys_rt_sigsuspend
131 64 sigaltstack __x64_sys_sigaltstack
132 common utime __x64_sys_utime
@@ -305,7 +305,7 @@
294 common inotify_init1 __x64_sys_inotify_init1
295 64 preadv __x64_sys_preadv
296 64 pwritev __x64_sys_pwritev
-297 64 rt_tgsigqueueinfo __x64_sys_rt_tgsigqueueinfo
+297 64 rt_tgsigqueueinfo __x64_sys_x86_rt_tgsigqueueinfo
298 common perf_event_open __x64_sys_perf_event_open
299 64 recvmmsg __x64_sys_recvmmsg
300 common fanotify_init __x64_sys_fanotify_init
@@ -369,7 +369,7 @@
521 x32 ptrace __x32_compat_sys_ptrace
522 x32 rt_sigpending __x32_compat_sys_rt_sigpending
523 x32 rt_sigtimedwait __x32_compat_sys_rt_sigtimedwait
-524 x32 rt_sigqueueinfo __x32_compat_sys_rt_sigqueueinfo
+524 x32 rt_sigqueueinfo __x64_sys_x86_rt_sigqueueinfo
525 x32 sigaltstack __x32_compat_sys_sigaltstack
526 x32 timer_create __x32_compat_sys_timer_create
527 x32 mq_notify __x32_compat_sys_mq_notify
@@ -381,7 +381,7 @@
533 x32 move_pages __x32_compat_sys_move_pages
534 x32 preadv __x32_compat_sys_preadv64
535 x32 pwritev __x32_compat_sys_pwritev64
-536 x32 rt_tgsigqueueinfo __x32_compat_sys_rt_tgsigqueueinfo
+536 x32 rt_tgsigqueueinfo __x64_sys_x86_rt_tgsigqueueinfo
537 x32 recvmmsg __x32_compat_sys_recvmmsg
538 x32 sendmmsg __x32_compat_sys_sendmmsg
539 x32 process_vm_readv __x32_compat_sys_process_vm_readv
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 92a3b312a53c..2f16330cac83 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -892,4 +892,38 @@ asmlinkage long sys32_x32_rt_sigreturn(void)
signal_fault(regs, frame, "x32 rt_sigreturn");
return 0;
}
+
+SYSCALL_DEFINE3(x86_rt_sigqueueinfo, pid_t, pid, int, sig,
+ siginfo_t __user *, uinfo)
+{
+ kernel_siginfo_t info;
+ int ret;
+
+ if (!in_x32_syscall()
+ ret = __copy_siginfo_from_user(sig, &info, uinfo);
+ else
+ ret = __copy_siginfo_from_user32(sig, &info, uinfo);
+
+ if (unlikely(ret))
+ return ret;
+ return do_rt_sigqueueinfo(pid, sig, &info);
+}
+
+SYSCALL_DEFINE3(x86_rt_tsigqueueinfo, pid_t, tgid, pid_t, pid, int, sig,
+ siginfo_t __user *, uinfo)
+{
+ kernel_siginfo_t info;
+ int ret;
+
+ if (!in_x32_syscall()
+ ret = __copy_siginfo_from_user(sig, &info, uinfo);
+ else
+ ret = __copy_siginfo_from_user32(sig, &info, uinfo);
+
+ if (unlikely(ret))
+ return ret;
+ return do_rt_tsigqueueinfo(tgid, pid, sig, &info);
+}
+
+
#endif
^ permalink raw reply related
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-12-01 9:17 UTC (permalink / raw)
To: Arnd Bergmann, Andy Lutomirski
Cc: Eric W . Biederman, Florian Weimer, Linux Kernel Mailing List,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov, cyphar,
Al Viro, Linux FS-devel Mailing List, Linux API,
Daniel Colascione, Tim Murray, linux-man, Kees Cook
In-Reply-To: <CAK8P3a1OX1Hb17=NbTYqZxgEM-sk5-dh_VeKa0bXJpq=k=KxHA@mail.gmail.com>
On December 1, 2018 9:51:18 PM GMT+13:00, Arnd Bergmann <arnd@arndb.de> wrote:
>On Sat, Dec 1, 2018 at 12:54 AM Andy Lutomirski <luto@kernel.org>
>wrote:
>> On Fri, Nov 30, 2018 at 2:10 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski <luto@kernel.org>
>wrote:
>> > > On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann <arnd@arndb.de>
>wrote:
>> > > > siginfo_t as it is now still has a number of other downsides,
>and Andy in
>> > > > particular didn't like the idea of having three new variants on
>x86
>> > > > (depending on how you count). His alternative suggestion of
>having
>> > > > a single syscall entry point that takes a 'signfo_t __user *'
>but interprets
>> > > > it as compat_siginfo depending on
>in_compat_syscall()/in_x32_syscall()
>> > > > should work correctly, but feels wrong to me, or at least
>inconsistent
>> > > > with how we do this elsewhere.
>
>> > The '548 | 0x40000000' part seems to be the only sensible
>> > way to handle x32 here. What exactly would you propose to
>> > avoid defining the other entry points?
>>
>> I would propose that it should be 335 | 0x40000000. I can't see any
>> reasonable way to teach the kernel to reject 335 | 0x40000000 that
>> wouldn't work just as well to accept it and make it do the right
>> thing. Currently we accept it and do the *wrong* thing, which is no
>> good.
>>
>> > and we have to
>> > add more complexity to the copy_siginfo_from_user()
>> > implementation to duplicate the hack that exists in
>> > copy_siginfo_from_user32().
>>
>> What hack are you referring to here?
>
>I mean this part:
>
>#ifdef CONFIG_COMPAT
>int copy_siginfo_to_user32(struct compat_siginfo __user *to,
> const struct kernel_siginfo *from)
>#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION)
>{
> return __copy_siginfo_to_user32(to, from, in_x32_syscall());
>}
>int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
> const struct kernel_siginfo *from, bool x32_ABI)
>#endif
>{
>...
> case SIL_CHLD:
> new.si_pid = from->si_pid;
> new.si_uid = from->si_uid;
> new.si_status = from->si_status;
>#ifdef CONFIG_X86_X32_ABI
> if (x32_ABI) {
> new._sifields._sigchld_x32._utime = from->si_utime;
> new._sifields._sigchld_x32._stime = from->si_stime;
> } else
>#endif
> {
> new.si_utime = from->si_utime;
> new.si_stime = from->si_stime;
> }
> break;
>...
>}
>#endif
>
>If we have a '548 | 0x40000000' entry pointing to
>__x32_compat_sys_procfd_kill, then that will do the right
>thing. If you instead try to have x32 call into the native
>sys_procfd_kill, then copy_siginfo_to_user() will also have
>to know about x32, effectively duplicating that mess above,
>unless you want to also change all users of
>copy_siginfo_to_user32() to use copy_siginfo_to_user()
>and handle all cases in one function.
I've been looking into having siginfo64_t
with the new copy_siginfo_to_user64()
function. It looks like a pretty intricate
task. Are we sure that we want to go
down this road? I'm not sure that it'll be
worth it. Especially since we force yet
another signal struct on user space.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Arnd Bergmann @ 2018-12-01 8:51 UTC (permalink / raw)
To: Andy Lutomirski
Cc: christian, Eric W . Biederman, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <CALCETrWcJCjNGEqgq57Bm8O3qWkfEwau2SiXAv+SNYvNmXnBCA@mail.gmail.com>
On Sat, Dec 1, 2018 at 12:54 AM Andy Lutomirski <luto@kernel.org> wrote:
> On Fri, Nov 30, 2018 at 2:10 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski <luto@kernel.org> wrote:
> > > On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > siginfo_t as it is now still has a number of other downsides, and Andy in
> > > > particular didn't like the idea of having three new variants on x86
> > > > (depending on how you count). His alternative suggestion of having
> > > > a single syscall entry point that takes a 'signfo_t __user *' but interprets
> > > > it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
> > > > should work correctly, but feels wrong to me, or at least inconsistent
> > > > with how we do this elsewhere.
> > The '548 | 0x40000000' part seems to be the only sensible
> > way to handle x32 here. What exactly would you propose to
> > avoid defining the other entry points?
>
> I would propose that it should be 335 | 0x40000000. I can't see any
> reasonable way to teach the kernel to reject 335 | 0x40000000 that
> wouldn't work just as well to accept it and make it do the right
> thing. Currently we accept it and do the *wrong* thing, which is no
> good.
>
> > and we have to
> > add more complexity to the copy_siginfo_from_user()
> > implementation to duplicate the hack that exists in
> > copy_siginfo_from_user32().
>
> What hack are you referring to here?
I mean this part:
#ifdef CONFIG_COMPAT
int copy_siginfo_to_user32(struct compat_siginfo __user *to,
const struct kernel_siginfo *from)
#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION)
{
return __copy_siginfo_to_user32(to, from, in_x32_syscall());
}
int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
const struct kernel_siginfo *from, bool x32_ABI)
#endif
{
...
case SIL_CHLD:
new.si_pid = from->si_pid;
new.si_uid = from->si_uid;
new.si_status = from->si_status;
#ifdef CONFIG_X86_X32_ABI
if (x32_ABI) {
new._sifields._sigchld_x32._utime = from->si_utime;
new._sifields._sigchld_x32._stime = from->si_stime;
} else
#endif
{
new.si_utime = from->si_utime;
new.si_stime = from->si_stime;
}
break;
...
}
#endif
If we have a '548 | 0x40000000' entry pointing to
__x32_compat_sys_procfd_kill, then that will do the right
thing. If you instead try to have x32 call into the native
sys_procfd_kill, then copy_siginfo_to_user() will also have
to know about x32, effectively duplicating that mess above,
unless you want to also change all users of
copy_siginfo_to_user32() to use copy_siginfo_to_user()
and handle all cases in one function.
Arnd
^ permalink raw reply
* Re: [PATCH] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Kees Cook @ 2018-12-01 4:28 UTC (permalink / raw)
To: Oleg Nesterov
Cc: j, Eric W. Biederman, Andrew Morton, Thomas Gleixner,
Andy Lutomirski, Linux API, LKML
In-Reply-To: <20181130103329.GB23670@redhat.com>
On Fri, Nov 30, 2018 at 2:33 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 11/29, Jürg Billeter wrote:
> >
> > On Thu, 2018-11-29 at 13:34 +0100, Oleg Nesterov wrote:
> > > So I think the patch is mostly fine, the only problem I can see is that
> > > PR_SET_KILL_DESCENDANTS_ON_EXIT can race with PR_SET_CHILD_SUBREAPER, they both
> > > need to update the bits in the same word.
> >
> > Good point. I'll make it a regular bool instead of a bitfield for v2,
>
> Agreed,
Is it worth doing something for singal_struct like we did for
task_struct with the TASK_PFA_* and atomic_flags?
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-12-01 1:25 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Andy Lutomirski, Andy Lutomirski, Florian Weimer,
Eric W . Biederman, Linux Kernel Mailing List, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <CAK8P3a1_if7+Ak2eefU6JeZT9KW827gkLHaObX-QOsHrB5ZwfA@mail.gmail.com>
On November 30, 2018 10:40:49 AM GMT+13:00, Arnd Bergmann <arnd@arndb.de> wrote:
>On Thu, Nov 29, 2018 at 10:35 PM Christian Brauner
><christian@brauner.io> wrote:
>> On Thu, Nov 29, 2018 at 10:02:13PM +0100, Arnd Bergmann wrote:
>> > On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski
><luto@amacapital.net> wrote:
>> >
>> > Is the current procfd_signal() proposal (under whichever name)
>sufficient
>> > to correctly implement both sys_rt_sigqueueinfo() and
>sys_rt_tgsigqueueinfo()?
>>
>> Yes, I see no reason why not. My idea is to extend it - after we have
>a
>> basic version in - to also work with:
>> /proc/<pid>/task/<tid>
>> If I'm not mistaken this should be sufficient to get
>rt_tgsigqueueinfo.
>> The thread will be uniquely identified by the tid descriptor and no
>> combination of /proc/<pid> and /proc/<pid>/task/<tid> is needed. Does
>> that sound reasonable?
>
>Yes. So it would currently replace rt_gsigqueueinfo() but
>not rt_tgsigqueueinfo(), and could be extended to do both
>afterwards, without making the interface ugly in any form?
Yes. :)
>
>I suppose we can always add more flags if needed, and you
>already ensure that flags is zero for the moment.
Yep.
>
>> > Can we implement sys_rt_sigtimedwait() based on signalfd()?
>> >
>> > If yes, that would leave waitid(), which already needs a
>replacement
>> > for y2038, and that should then also return a signalfd_siginfo.
>> > My current preference for waitid() would be to do a version that
>> > closely resembles the current interface, but takes a
>signalfd_siginfo
>> > and a __kernel_timespec based rusage replacement (possibly
>> > two of them to let us map wait6), but does not operate on procfd or
>> > take a signal mask. That would require yet another syscall, but I
>> > don't think I can do that before we want to have the set of y2038
>> > safe syscalls.
>>
>> All sounds reasonable to me but that's not a blocker for the current
>> syscall though, is it?
>
>I'd like to at least understand about sys_rt_sigtimedwait() before
>we go on, so we all know what's coming, and document the
>plans in the changelog.
>
>waitid() probably remains on my plate anyway, and I hope understand
>where we're at with it.
>
> Arnd
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-12-01 1:20 UTC (permalink / raw)
Cc: Arnd Bergmann, Daniel Colascione, Andrew Lutomirski,
Eric W. Biederman, Florian Weimer, 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: <CALCETrWcUQC_aeHRwVF0yr8GATndRU9277rQQ+caOcR1uJ1W9g@mail.gmail.com>
On December 1, 2018 12:46:22 PM GMT+13:00, Andy Lutomirski <luto@kernel.org> wrote:
>On Fri, Nov 30, 2018 at 3:40 PM Christian Brauner
><christian@brauner.io> wrote:
>>
>> On December 1, 2018 12:12:53 PM GMT+13:00, Arnd Bergmann
><arnd@arndb.de> wrote:
>> >On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione
><dancol@google.com>
>> >wrote:
>> >> On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner
>> ><christian@brauner.io> wrote:
>> >> > On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann
>> ><arnd@arndb.de> wrote:
>> >> >
>> >> > One humble point I would like to make is that what I care about
>> >most is a sensible way forward without having to redo essential
>parts
>> >of how syscalls work.
>> >> > I don't want to introduce a sane, small syscall that ends up
>> >breaking all over the place because we decided to fix past mistakes
>> >that technically have nothing to do with the patch itself.
>> >> > However, I do sympathize and understand these concerns.
>> >>
>> >> IMHO, it's fine to just replicate all the splits we have for the
>> >> existing signal system calls. It's ugly, but once it's done, it'll
>be
>> >> done for a long time. I can't see a need to add even more signal
>> >> system calls after this one.
>> >
>> >We definitely need waitid_time64() and rt_sigtimedwait_time64()
>> >in the very near future.
>>
>> Right, I remember you pointing this out in a prior mail.
>> Thanks for working on this for such a long time now, Arnd!
>> Can we agree to move on with the procfd syscall given the current
>constraints?
>> I just don't want to see the syscall being
>> blocked by a generic problem whose
>> ultimate solution is to get rid of weird
>> architectural constraints.
>
>Creating and using a copy_siginfo_from_user64() function would work
>for everyone, no?
Meaning, no compat syscalls, introduce
new struct siginfo64_t and the copy
function you named above?
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Andy Lutomirski @ 2018-11-30 23:53 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Andrew Lutomirski, Christian Brauner, Eric W. Biederman,
Florian Weimer, 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: <CAK8P3a1isSRCd-OLJK15C+gSJdpeR=zEXghc8+AMM5QYYw3Tyg@mail.gmail.com>
On Fri, Nov 30, 2018 at 2:10 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski <luto@kernel.org> wrote:
> >
> > On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > siginfo_t as it is now still has a number of other downsides, and Andy in
> > > particular didn't like the idea of having three new variants on x86
> > > (depending on how you count). His alternative suggestion of having
> > > a single syscall entry point that takes a 'signfo_t __user *' but interprets
> > > it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
> > > should work correctly, but feels wrong to me, or at least inconsistent
> > > with how we do this elsewhere.
> >
> > If everyone else is okay with it, I can get on board with three
> > variants on x86. What I can't get on board with is *five* variants on
> > x86, which would be:
> >
> > procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
> >
> > syscall64 with nr == 335 (presumably): 64-bit
>
> These seem unavoidable
Indeed, although, in hindsight, they should have had the same numbers.
>
> > syscall64 with nr == 548 | 0x40000000: x32
> >
> > syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
> > true, behavior is arbitrary
> >
> > syscall64 with nr == 335 | 0x40000000: x32 entry, but
> > in_compat_syscall() == false, behavior is arbitrary
>
> Am I misreading the code? The way I understand it, setting the
> 0x40000000 bit means that both in_compat_syscall() and
> in_x32_syscall become() true, based on
>
> static inline bool in_x32_syscall(void)
> {
> #ifdef CONFIG_X86_X32_ABI
> if (task_pt_regs(current)->orig_ax & __X32_SYSCALL_BIT)
> return true;
> #endif
> return false;
> }
Yes.
>
> The '548 | 0x40000000' part seems to be the only sensible
> way to handle x32 here. What exactly would you propose to
> avoid defining the other entry points?
I would propose that it should be 335 | 0x40000000. I can't see any
reasonable way to teach the kernel to reject 335 | 0x40000000 that
wouldn't work just as well to accept it and make it do the right
thing. Currently we accept it and do the *wrong* thing, which is no
good.
>
> > This mess isn't really Christian's fault -- it's been there for a
> > while, but it's awful and I don't think we want to perpetuate it.
>
> I'm not convinced that not assigning an x32 syscall number
> improves the situation, it just means that we now have one
> syscall that behaves completely differently from all others,
> in that the x32 version requires being called through a
> SYSCALL_DEFINE() entry point rather than a
> COMPAT_SYSCALL_DEFINE() one,
There's nothing particularly novel about this. seccomp(), for
example, already works like this.
> and we have to
> add more complexity to the copy_siginfo_from_user()
> implementation to duplicate the hack that exists in
> copy_siginfo_from_user32().
What hack are you referring to here?
>
> Of course, the nicest option would be to completely remove
> x32 so we can stop worrying about it.
True :)
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-30 23:52 UTC (permalink / raw)
To: Florian Weimer
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man,
Kees Cook
In-Reply-To: <87in0g5aqo.fsf@oldenburg.str.redhat.com>
On November 30, 2018 1:28:15 AM GMT+13:00, Florian Weimer <fweimer@redhat.com> wrote:
>Disclaimer: I'm looking at this patch because Christian requested it.
>I'm not a kernel developer.
Given all your expertise this really doesn't matter. :)
You're the one having to deal with this
in glibc after all.
Thanks for doing this and sorry for the late reply.
I missed that mail.
>
>* Christian Brauner:
>
>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl
>b/arch/x86/entry/syscalls/syscall_32.tbl
>> index 3cf7b533b3d1..3f27ffd8ae87 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_compat_sys_procfd_signal
>> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl
>b/arch/x86/entry/syscalls/syscall_64.tbl
>> index f0b1709a5ffb..8a30cde82450 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 64 procfd_signal __x64_sys_procfd_signal
>>
>> #
>> # x32-specific system call numbers start at 512 to avoid cache
>impact
>> @@ -386,3 +387,4 @@
>> 545 x32 execveat __x32_compat_sys_execveat/ptregs
>> 546 x32 preadv2 __x32_compat_sys_preadv64v2
>> 547 x32 pwritev2 __x32_compat_sys_pwritev64v2
>> +548 x32 procfd_signal __x32_compat_sys_procfd_signal
>
>Is there a reason why these numbers have to be different?
>
>(See the recent discussion with Andy Lutomirski.)
>
>> +static int do_procfd_signal(int fd, int sig, kernel_siginfo_t
>*kinfo, int flags,
>> + bool had_siginfo)
>> +{
>> + int ret;
>> + struct fd f;
>> + struct pid *pid;
>> +
>> + /* Enforce flags be set to 0 until we add an extension. */
>> + if (flags)
>> + return -EINVAL;
>> +
>> + f = fdget_raw(fd);
>> + if (!f.file)
>> + return -EBADF;
>> +
>> + /* Is this a process file descriptor? */
>> + ret = -EINVAL;
>> + if (!proc_is_tgid_procfd(f.file))
>> + goto err;
>[…]
>> + ret = kill_pid_info(sig, kinfo, pid);
>
>I would like to see some comment here what happens to zombie processes.
You'd get ESRCH.
I'm not sure if that has always been the case.
Eric recently did some excellent refactoring of the signal code.
Iirc, part of that involved not delivering signals to zombies.
That's at least how I remember it.
I don't have access to source code though atm.
>
>> +/**
>> + * 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)
>
>Sorry, I'm quite unhappy with the name. “signal” is for signal handler
>management. procfd_sendsignal, procfd_sigqueueinfo or something like
>that would be fine. Even procfd_kill would be better IMHO.
Ok. I only have strong opinions to procfd_kill().
Mainly because the new syscall takes
the job of multiple other syscalls
so kill gives the wrong impression.
I'll come up with a better name in the next iteration.
>
>Looking at the rt_tgsigqueueinfo interface, is there a way to implement
>the “tg” part with the current procfd_signal interface? Would you use
>openat to retrieve the Tgid: line from "status"?
Yes, the tg part can be implemented.
As I pointed out in another mail my
I is to make this work by using file
descriptors for /proc/<pid>/task/<tid>.
I don't want this in the initial patchset though.
I prefer to slowly add those features
once we have gotten the basic functionality
in.
>
>Thanks,
>Florian
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Andy Lutomirski @ 2018-11-30 23:46 UTC (permalink / raw)
To: Christian Brauner
Cc: Arnd Bergmann, Daniel Colascione, Andrew Lutomirski,
Eric W. Biederman, Florian Weimer, 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: <7C5B4CBD-6364-4DCE-9EFD-3657C67DACEB@brauner.io>
On Fri, Nov 30, 2018 at 3:40 PM Christian Brauner <christian@brauner.io> wrote:
>
> On December 1, 2018 12:12:53 PM GMT+13:00, Arnd Bergmann <arnd@arndb.de> wrote:
> >On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione <dancol@google.com>
> >wrote:
> >> On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner
> ><christian@brauner.io> wrote:
> >> > On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann
> ><arnd@arndb.de> wrote:
> >> >
> >> > One humble point I would like to make is that what I care about
> >most is a sensible way forward without having to redo essential parts
> >of how syscalls work.
> >> > I don't want to introduce a sane, small syscall that ends up
> >breaking all over the place because we decided to fix past mistakes
> >that technically have nothing to do with the patch itself.
> >> > However, I do sympathize and understand these concerns.
> >>
> >> IMHO, it's fine to just replicate all the splits we have for the
> >> existing signal system calls. It's ugly, but once it's done, it'll be
> >> done for a long time. I can't see a need to add even more signal
> >> system calls after this one.
> >
> >We definitely need waitid_time64() and rt_sigtimedwait_time64()
> >in the very near future.
>
> Right, I remember you pointing this out in a prior mail.
> Thanks for working on this for such a long time now, Arnd!
> Can we agree to move on with the procfd syscall given the current constraints?
> I just don't want to see the syscall being
> blocked by a generic problem whose
> ultimate solution is to get rid of weird
> architectural constraints.
Creating and using a copy_siginfo_from_user64() function would work
for everyone, no?
^ permalink raw reply
* Re: Security modules and sending signals within the same process
From: John Johansen @ 2018-11-30 23:38 UTC (permalink / raw)
To: Casey Schaufler, Florian Weimer, apparmor-nLRlyDuq1AZFpShjVBNYrg,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
selinux-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Arnd Bergmann, H. Peter Anvin
In-Reply-To: <2c3e813c-f56a-3354-1299-30b0646f40e1-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org>
On 11/30/18 9:54 AM, Casey Schaufler wrote:
> On 11/30/2018 7:14 AM, Florian Weimer wrote:
>> Is it guaranteed that tasks in the same thread group can always send
>> signals to each other, irrespective of their respective credentials
>> structs?
>
> No. An LSM may chose to disallow this based on just about any
> criteria it desires.
>
And apparmor is in fact doing this a few limited situations, userspace
has to request the profile change via an api, and regular policy
enforcement based on credentials mediates the signals. Its not
something we recommend but it has been used.
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-30 23:37 UTC (permalink / raw)
To: Arnd Bergmann, Daniel Colascione
Cc: Andy Lutomirski, Eric W . Biederman, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Tim Murray, linux-man,
Kees Cook
In-Reply-To: <CAK8P3a2x3nQKKvLBSH8G9R6w+sv=09y3h7gWdFYUMNAsB8aj6g@mail.gmail.com>
On December 1, 2018 12:12:53 PM GMT+13:00, Arnd Bergmann <arnd@arndb.de> wrote:
>On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione <dancol@google.com>
>wrote:
>> On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner
><christian@brauner.io> wrote:
>> > On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann
><arnd@arndb.de> wrote:
>> >
>> > One humble point I would like to make is that what I care about
>most is a sensible way forward without having to redo essential parts
>of how syscalls work.
>> > I don't want to introduce a sane, small syscall that ends up
>breaking all over the place because we decided to fix past mistakes
>that technically have nothing to do with the patch itself.
>> > However, I do sympathize and understand these concerns.
>>
>> IMHO, it's fine to just replicate all the splits we have for the
>> existing signal system calls. It's ugly, but once it's done, it'll be
>> done for a long time. I can't see a need to add even more signal
>> system calls after this one.
>
>We definitely need waitid_time64() and rt_sigtimedwait_time64()
>in the very near future.
Right, I remember you pointing this out in a prior mail.
Thanks for working on this for such a long time now, Arnd!
Can we agree to move on with the procfd syscall given the current constraints?
I just don't want to see the syscall being
blocked by a generic problem whose
ultimate solution is to get rid of weird
architectural constraints. :)
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Arnd Bergmann @ 2018-11-30 23:15 UTC (permalink / raw)
To: Daniel Colascione
Cc: christian, Andy Lutomirski, Eric W . Biederman, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Tim Murray, linux-man,
Kees Cook
In-Reply-To: <CAK8P3a2x3nQKKvLBSH8G9R6w+sv=09y3h7gWdFYUMNAsB8aj6g@mail.gmail.com>
On Sat, Dec 1, 2018 at 12:12 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione <dancol@google.com> wrote:
> > On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner <christian@brauner.io> wrote:
> > > On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > One humble point I would like to make is that what I care about most is a sensible way forward without having to redo essential parts of how syscalls work.
> > > I don't want to introduce a sane, small syscall that ends up breaking all over the place because we decided to fix past mistakes that technically have nothing to do with the patch itself.
> > > However, I do sympathize and understand these concerns.
> >
> > IMHO, it's fine to just replicate all the splits we have for the
> > existing signal system calls. It's ugly, but once it's done, it'll be
> > done for a long time. I can't see a need to add even more signal
> > system calls after this one.
>
> We definitely need waitid_time64() and rt_sigtimedwait_time64()
> in the very near future.
To clarify: we probably don't need rt_sigtimedwait_time64() for
x32, as it already has a 64-bit time_t. We might need waitid_time64()
or something similar though, since the plan now is to change the
time resolution for rusage to nanoseconds (__kernel_timespec)
now. The exact behavior and name of waitid_time64() is still
a matter of discussion.
Arnd
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Arnd Bergmann @ 2018-11-30 23:12 UTC (permalink / raw)
To: Daniel Colascione
Cc: christian, Andy Lutomirski, Eric W . Biederman, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Tim Murray, linux-man,
Kees Cook
In-Reply-To: <CAKOZuesEPFnkd7YGbu82TRQwhR=KPwNW8GpYTw=719_xmJx9FA@mail.gmail.com>
On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione <dancol@google.com> wrote:
> On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner <christian@brauner.io> wrote:
> > On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > One humble point I would like to make is that what I care about most is a sensible way forward without having to redo essential parts of how syscalls work.
> > I don't want to introduce a sane, small syscall that ends up breaking all over the place because we decided to fix past mistakes that technically have nothing to do with the patch itself.
> > However, I do sympathize and understand these concerns.
>
> IMHO, it's fine to just replicate all the splits we have for the
> existing signal system calls. It's ugly, but once it's done, it'll be
> done for a long time. I can't see a need to add even more signal
> system calls after this one.
We definitely need waitid_time64() and rt_sigtimedwait_time64()
in the very near future.
Arnd
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Daniel Colascione @ 2018-11-30 23:05 UTC (permalink / raw)
To: Christian Brauner
Cc: Arnd Bergmann, Andy Lutomirski, Eric W. Biederman, Florian Weimer,
linux-kernel, 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: <EC87B1BE-BDD0-414F-85F8-806C3CC4DD07@brauner.io>
On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner <christian@brauner.io> wrote:
>
> On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann <arnd@arndb.de> wrote:
> >On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski <luto@kernel.org>
> >wrote:
> >>
> >> On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >> > siginfo_t as it is now still has a number of other downsides, and
> >Andy in
> >> > particular didn't like the idea of having three new variants on x86
> >> > (depending on how you count). His alternative suggestion of having
> >> > a single syscall entry point that takes a 'signfo_t __user *' but
> >interprets
> >> > it as compat_siginfo depending on
> >in_compat_syscall()/in_x32_syscall()
> >> > should work correctly, but feels wrong to me, or at least
> >inconsistent
> >> > with how we do this elsewhere.
> >>
> >> If everyone else is okay with it, I can get on board with three
> >> variants on x86. What I can't get on board with is *five* variants
> >on
> >> x86, which would be:
> >>
> >> procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
> >>
> >> syscall64 with nr == 335 (presumably): 64-bit
> >
> >These seem unavoidable
> >
> >> syscall64 with nr == 548 | 0x40000000: x32
> >>
> >> syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
> >> true, behavior is arbitrary
> >>
> >> syscall64 with nr == 335 | 0x40000000: x32 entry, but
> >> in_compat_syscall() == false, behavior is arbitrary
> >
> >Am I misreading the code? The way I understand it, setting the
> >0x40000000 bit means that both in_compat_syscall() and
> >in_x32_syscall become() true, based on
> >
> >static inline bool in_x32_syscall(void)
> >{
> >#ifdef CONFIG_X86_X32_ABI
> > if (task_pt_regs(current)->orig_ax & __X32_SYSCALL_BIT)
> > return true;
> >#endif
> > return false;
> >}
> >
> >The '548 | 0x40000000' part seems to be the only sensible
> >way to handle x32 here. What exactly would you propose to
> >avoid defining the other entry points?
> >
> >> This mess isn't really Christian's fault -- it's been there for a
> >> while, but it's awful and I don't think we want to perpetuate it.
> >
> >I'm not convinced that not assigning an x32 syscall number
> >improves the situation, it just means that we now have one
> >syscall that behaves completely differently from all others,
> >in that the x32 version requires being called through a
> >SYSCALL_DEFINE() entry point rather than a
> >COMPAT_SYSCALL_DEFINE() one, and we have to
> >add more complexity to the copy_siginfo_from_user()
> >implementation to duplicate the hack that exists in
> >copy_siginfo_from_user32().
> >
> >Of course, the nicest option would be to completely remove
> >x32 so we can stop worrying about it.
>
> One humble point I would like to make is that what I care about most is a sensible way forward without having to redo essential parts of how syscalls work.
> I don't want to introduce a sane, small syscall that ends up breaking all over the place because we decided to fix past mistakes that technically have nothing to do with the patch itself.
> However, I do sympathize and understand these concerns.
IMHO, it's fine to just replicate all the splits we have for the
existing signal system calls. It's ugly, but once it's done, it'll be
done for a long time. I can't see a need to add even more signal
system calls after this one.
^ permalink raw reply
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Dmitry V. Levin @ 2018-11-30 22:53 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Jann Horn, Michael Ellerman, Eugene Syromyatnikov,
Steven Rostedt, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Andy Lutomirski, linux-api-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <20181130112920.GD23670-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 974 bytes --]
On Fri, Nov 30, 2018 at 12:29:21PM +0100, Oleg Nesterov wrote:
> On 11/30, Dmitry V. Levin wrote:
> > On Thu, Nov 29, 2018 at 03:47:43PM +0100, Oleg Nesterov wrote:
> >
> > > > so that PTRACE_GETEVENTMSG users can easily tell
> > > > whether this new semantics is supported by the kernel or not.
> > >
> > > Yes. And how much this can help? Again, an application can trivially detect
> > > if this feature implemented or not, and it should do this anyway if it wants
> > > to (try to) use PTRACE_EVENTMSG_SYSCALL_ENTRY/EXIT ?
> >
> > How an application can easily detect whether this feature is implemented?
>
> As I already said, it can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL) ?
> If it returns -EIO then this feature is not implemented. Any other error
> code (actually EINVAL or EFAULT) means it is implemented.
Fair enough.
We can change PTRACE_EVENTMSG_SYSCALL_ENTRY/EXIT to 1/2 if you like,
and document this trick somewhere.
--
ldv
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 137 bytes --]
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-30 22:26 UTC (permalink / raw)
To: Arnd Bergmann, Andy Lutomirski
Cc: Eric W . Biederman, Florian Weimer, Linux Kernel Mailing List,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov, cyphar,
Al Viro, Linux FS-devel Mailing List, Linux API,
Daniel Colascione, Tim Murray, linux-man, Kees Cook
In-Reply-To: <CAK8P3a1isSRCd-OLJK15C+gSJdpeR=zEXghc8+AMM5QYYw3Tyg@mail.gmail.com>
On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann <arnd@arndb.de> wrote:
>On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski <luto@kernel.org>
>wrote:
>>
>> On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> > siginfo_t as it is now still has a number of other downsides, and
>Andy in
>> > particular didn't like the idea of having three new variants on x86
>> > (depending on how you count). His alternative suggestion of having
>> > a single syscall entry point that takes a 'signfo_t __user *' but
>interprets
>> > it as compat_siginfo depending on
>in_compat_syscall()/in_x32_syscall()
>> > should work correctly, but feels wrong to me, or at least
>inconsistent
>> > with how we do this elsewhere.
>>
>> If everyone else is okay with it, I can get on board with three
>> variants on x86. What I can't get on board with is *five* variants
>on
>> x86, which would be:
>>
>> procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
>>
>> syscall64 with nr == 335 (presumably): 64-bit
>
>These seem unavoidable
>
>> syscall64 with nr == 548 | 0x40000000: x32
>>
>> syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
>> true, behavior is arbitrary
>>
>> syscall64 with nr == 335 | 0x40000000: x32 entry, but
>> in_compat_syscall() == false, behavior is arbitrary
>
>Am I misreading the code? The way I understand it, setting the
>0x40000000 bit means that both in_compat_syscall() and
>in_x32_syscall become() true, based on
>
>static inline bool in_x32_syscall(void)
>{
>#ifdef CONFIG_X86_X32_ABI
> if (task_pt_regs(current)->orig_ax & __X32_SYSCALL_BIT)
> return true;
>#endif
> return false;
>}
>
>The '548 | 0x40000000' part seems to be the only sensible
>way to handle x32 here. What exactly would you propose to
>avoid defining the other entry points?
>
>> This mess isn't really Christian's fault -- it's been there for a
>> while, but it's awful and I don't think we want to perpetuate it.
>
>I'm not convinced that not assigning an x32 syscall number
>improves the situation, it just means that we now have one
>syscall that behaves completely differently from all others,
>in that the x32 version requires being called through a
>SYSCALL_DEFINE() entry point rather than a
>COMPAT_SYSCALL_DEFINE() one, and we have to
>add more complexity to the copy_siginfo_from_user()
>implementation to duplicate the hack that exists in
>copy_siginfo_from_user32().
>
>Of course, the nicest option would be to completely remove
>x32 so we can stop worrying about it.
One humble point I would like to make is that what I care about most is a sensible way forward without having to redo essential parts of how syscalls work.
I don't want to introduce a sane, small syscall that ends up breaking all over the place because we decided to fix past mistakes that technically have nothing to do with the patch itself.
However, I do sympathize and understand these concerns.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Arnd Bergmann @ 2018-11-30 22:09 UTC (permalink / raw)
To: Andy Lutomirski
Cc: christian, Eric W . Biederman, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <CALCETrW2aphWwEY4=hUwo_JBCvkQyMjxzxGd9FCW017kMLaMOQ@mail.gmail.com>
On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > siginfo_t as it is now still has a number of other downsides, and Andy in
> > particular didn't like the idea of having three new variants on x86
> > (depending on how you count). His alternative suggestion of having
> > a single syscall entry point that takes a 'signfo_t __user *' but interprets
> > it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
> > should work correctly, but feels wrong to me, or at least inconsistent
> > with how we do this elsewhere.
>
> If everyone else is okay with it, I can get on board with three
> variants on x86. What I can't get on board with is *five* variants on
> x86, which would be:
>
> procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
>
> syscall64 with nr == 335 (presumably): 64-bit
These seem unavoidable
> syscall64 with nr == 548 | 0x40000000: x32
>
> syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
> true, behavior is arbitrary
>
> syscall64 with nr == 335 | 0x40000000: x32 entry, but
> in_compat_syscall() == false, behavior is arbitrary
Am I misreading the code? The way I understand it, setting the
0x40000000 bit means that both in_compat_syscall() and
in_x32_syscall become() true, based on
static inline bool in_x32_syscall(void)
{
#ifdef CONFIG_X86_X32_ABI
if (task_pt_regs(current)->orig_ax & __X32_SYSCALL_BIT)
return true;
#endif
return false;
}
The '548 | 0x40000000' part seems to be the only sensible
way to handle x32 here. What exactly would you propose to
avoid defining the other entry points?
> This mess isn't really Christian's fault -- it's been there for a
> while, but it's awful and I don't think we want to perpetuate it.
I'm not convinced that not assigning an x32 syscall number
improves the situation, it just means that we now have one
syscall that behaves completely differently from all others,
in that the x32 version requires being called through a
SYSCALL_DEFINE() entry point rather than a
COMPAT_SYSCALL_DEFINE() one, and we have to
add more complexity to the copy_siginfo_from_user()
implementation to duplicate the hack that exists in
copy_siginfo_from_user32().
Of course, the nicest option would be to completely remove
x32 so we can stop worrying about it.
Arnd
^ 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