mbox series

[v4,0/2] Syscall User Redirection

Message ID 20200716193141.4068476-1-krisman@collabora.com (mailing list archive)
Headers show
Series Syscall User Redirection | expand

Message

Gabriel Krisman Bertazi July 16, 2020, 7:31 p.m. UTC
Hi,

This is v4 of Syscall User Redirection.  The implementation itself is
not modified from v3, it only applies the latest round of reviews to the
selftests.

__NR_syscalls is not really exported in header files other than
asm-generic for every architecture, so it felt safer to optionally
expose it with a fallback to a high value.

Also, I didn't expose tests for PR_GET as that is not currently
implemented.  If possible, I'd have it supported by a future patchset,
since it is not immediately necessary to support this feature.

Finally, one question: Which tree would this go through?

Gabriel Krisman Bertazi (2):
  kernel: Implement selective syscall userspace redirection
  selftests: Add kselftest for syscall user dispatch

 arch/Kconfig                                  |  20 ++
 arch/x86/Kconfig                              |   1 +
 arch/x86/entry/common.c                       |   5 +
 arch/x86/include/asm/thread_info.h            |   4 +-
 arch/x86/kernel/signal_compat.c               |   2 +-
 fs/exec.c                                     |   2 +
 include/linux/sched.h                         |   3 +
 include/linux/syscall_user_dispatch.h         |  50 ++++
 include/uapi/asm-generic/siginfo.h            |   3 +-
 include/uapi/linux/prctl.h                    |   5 +
 kernel/Makefile                               |   1 +
 kernel/fork.c                                 |   1 +
 kernel/sys.c                                  |   5 +
 kernel/syscall_user_dispatch.c                |  92 +++++++
 tools/testing/selftests/Makefile              |   1 +
 .../syscall_user_dispatch/.gitignore          |   2 +
 .../selftests/syscall_user_dispatch/Makefile  |   9 +
 .../selftests/syscall_user_dispatch/config    |   1 +
 .../syscall_user_dispatch.c                   | 256 ++++++++++++++++++
 19 files changed, 460 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/syscall_user_dispatch.h
 create mode 100644 kernel/syscall_user_dispatch.c
 create mode 100644 tools/testing/selftests/syscall_user_dispatch/.gitignore
 create mode 100644 tools/testing/selftests/syscall_user_dispatch/Makefile
 create mode 100644 tools/testing/selftests/syscall_user_dispatch/config
 create mode 100644 tools/testing/selftests/syscall_user_dispatch/syscall_user_dispatch.c

Comments

Kees Cook July 16, 2020, 8:04 p.m. UTC | #1
On Thu, Jul 16, 2020 at 03:31:39PM -0400, Gabriel Krisman Bertazi wrote:
> This is v4 of Syscall User Redirection.  The implementation itself is
> not modified from v3, it only applies the latest round of reviews to the
> selftests.
> 
> __NR_syscalls is not really exported in header files other than
> asm-generic for every architecture, so it felt safer to optionally
> expose it with a fallback to a high value.
> 
> Also, I didn't expose tests for PR_GET as that is not currently
> implemented.  If possible, I'd have it supported by a future patchset,
> since it is not immediately necessary to support this feature.

Thanks! That all looks good to me.

> Finally, one question: Which tree would this go through?

I haven't heard from several other x86 maintainers yet (which is where
I would normally expect this series to land), but I would be comfortable
taking this through my seccomp tree if I got Acks/Reviews at least from
Andy and Matthew.

Andy, Matthew, what do you think of this?
Christian Brauner July 16, 2020, 8:22 p.m. UTC | #2
On Thu, Jul 16, 2020 at 01:04:38PM -0700, Kees Cook wrote:
> On Thu, Jul 16, 2020 at 03:31:39PM -0400, Gabriel Krisman Bertazi wrote:
> > This is v4 of Syscall User Redirection.  The implementation itself is
> > not modified from v3, it only applies the latest round of reviews to the
> > selftests.
> > 
> > __NR_syscalls is not really exported in header files other than
> > asm-generic for every architecture, so it felt safer to optionally
> > expose it with a fallback to a high value.
> > 
> > Also, I didn't expose tests for PR_GET as that is not currently
> > implemented.  If possible, I'd have it supported by a future patchset,
> > since it is not immediately necessary to support this feature.
> 
> Thanks! That all looks good to me.

Don't have any problem with this but did this ever get exposure on
linux-api? This is the first time I see this pop up.

Christian
Kees Cook July 16, 2020, 8:25 p.m. UTC | #3
On Thu, Jul 16, 2020 at 10:22:34PM +0200, Christian Brauner wrote:
> On Thu, Jul 16, 2020 at 01:04:38PM -0700, Kees Cook wrote:
> > On Thu, Jul 16, 2020 at 03:31:39PM -0400, Gabriel Krisman Bertazi wrote:
> > > This is v4 of Syscall User Redirection.  The implementation itself is
> > > not modified from v3, it only applies the latest round of reviews to the
> > > selftests.
> > > 
> > > __NR_syscalls is not really exported in header files other than
> > > asm-generic for every architecture, so it felt safer to optionally
> > > expose it with a fallback to a high value.
> > > 
> > > Also, I didn't expose tests for PR_GET as that is not currently
> > > implemented.  If possible, I'd have it supported by a future patchset,
> > > since it is not immediately necessary to support this feature.
> > 
> > Thanks! That all looks good to me.
> 
> Don't have any problem with this but did this ever get exposure on
> linux-api? This is the first time I see this pop up.

I thought I'd added it to CC in the past, but that might have been other
recent unrelated threads. Does this need a full repost there too, you
think?
Christian Brauner July 16, 2020, 8:29 p.m. UTC | #4
On Thu, Jul 16, 2020 at 01:25:43PM -0700, Kees Cook wrote:
> On Thu, Jul 16, 2020 at 10:22:34PM +0200, Christian Brauner wrote:
> > On Thu, Jul 16, 2020 at 01:04:38PM -0700, Kees Cook wrote:
> > > On Thu, Jul 16, 2020 at 03:31:39PM -0400, Gabriel Krisman Bertazi wrote:
> > > > This is v4 of Syscall User Redirection.  The implementation itself is
> > > > not modified from v3, it only applies the latest round of reviews to the
> > > > selftests.
> > > > 
> > > > __NR_syscalls is not really exported in header files other than
> > > > asm-generic for every architecture, so it felt safer to optionally
> > > > expose it with a fallback to a high value.
> > > > 
> > > > Also, I didn't expose tests for PR_GET as that is not currently
> > > > implemented.  If possible, I'd have it supported by a future patchset,
> > > > since it is not immediately necessary to support this feature.
> > > 
> > > Thanks! That all looks good to me.
> > 
> > Don't have any problem with this but did this ever get exposure on
> > linux-api? This is the first time I see this pop up.
> 
> I thought I'd added it to CC in the past, but that might have been other
> recent unrelated threads. Does this need a full repost there too, you
> think?

Nah, wasn't my intention to force a repost. Seems that several people
have looked this over. :) Just curious why it didn't get to linux-api
and we know quite some people who only do look at linux-api (for sanity). :)

Christian
Gabriel Krisman Bertazi July 16, 2020, 8:30 p.m. UTC | #5
Christian Brauner <christian.brauner@ubuntu.com> writes:

> On Thu, Jul 16, 2020 at 01:25:43PM -0700, Kees Cook wrote:
>> On Thu, Jul 16, 2020 at 10:22:34PM +0200, Christian Brauner wrote:
>> > On Thu, Jul 16, 2020 at 01:04:38PM -0700, Kees Cook wrote:
>> > > On Thu, Jul 16, 2020 at 03:31:39PM -0400, Gabriel Krisman Bertazi wrote:
>> > > > This is v4 of Syscall User Redirection.  The implementation itself is
>> > > > not modified from v3, it only applies the latest round of reviews to the
>> > > > selftests.
>> > > > 
>> > > > __NR_syscalls is not really exported in header files other than
>> > > > asm-generic for every architecture, so it felt safer to optionally
>> > > > expose it with a fallback to a high value.
>> > > > 
>> > > > Also, I didn't expose tests for PR_GET as that is not currently
>> > > > implemented.  If possible, I'd have it supported by a future patchset,
>> > > > since it is not immediately necessary to support this feature.
>> > > 
>> > > Thanks! That all looks good to me.
>> > 
>> > Don't have any problem with this but did this ever get exposure on
>> > linux-api? This is the first time I see this pop up.
>> 
>> I thought I'd added it to CC in the past, but that might have been other
>> recent unrelated threads. Does this need a full repost there too, you
>> think?
>
> Nah, wasn't my intention to force a repost. Seems that several people
> have looked this over. :) Just curious why it didn't get to linux-api
> and we know quite some people who only do look at linux-api (for sanity). :)

That's my mistake.  I didn't think about it when submitting :(

If this get re-spinned again I will make sure to CC linux-api.
Carlos O'Donell July 16, 2020, 9:06 p.m. UTC | #6
On 7/16/20 4:30 PM, Gabriel Krisman Bertazi wrote:
> Christian Brauner <christian.brauner@ubuntu.com> writes:
> 
>> On Thu, Jul 16, 2020 at 01:25:43PM -0700, Kees Cook wrote:
>>> On Thu, Jul 16, 2020 at 10:22:34PM +0200, Christian Brauner wrote:
>>>> On Thu, Jul 16, 2020 at 01:04:38PM -0700, Kees Cook wrote:
>>>>> On Thu, Jul 16, 2020 at 03:31:39PM -0400, Gabriel Krisman Bertazi wrote:
>>>>>> This is v4 of Syscall User Redirection.  The implementation itself is
>>>>>> not modified from v3, it only applies the latest round of reviews to the
>>>>>> selftests.
>>>>>>
>>>>>> __NR_syscalls is not really exported in header files other than
>>>>>> asm-generic for every architecture, so it felt safer to optionally
>>>>>> expose it with a fallback to a high value.
>>>>>>
>>>>>> Also, I didn't expose tests for PR_GET as that is not currently
>>>>>> implemented.  If possible, I'd have it supported by a future patchset,
>>>>>> since it is not immediately necessary to support this feature.
>>>>>
>>>>> Thanks! That all looks good to me.
>>>>
>>>> Don't have any problem with this but did this ever get exposure on
>>>> linux-api? This is the first time I see this pop up.
>>>
>>> I thought I'd added it to CC in the past, but that might have been other
>>> recent unrelated threads. Does this need a full repost there too, you
>>> think?
>>
>> Nah, wasn't my intention to force a repost. Seems that several people
>> have looked this over. :) Just curious why it didn't get to linux-api
>> and we know quite some people who only do look at linux-api (for sanity). :)
> 
> That's my mistake.  I didn't think about it when submitting :(
> 
> If this get re-spinned again I will make sure to CC linux-api.

Thank you! It helps C library implementors stay up to date and comment
on changes that impact userspace ABIs and APIs. This patch set was new
to me. Interesting new feature.
Pavel Machek Aug. 2, 2020, 12:01 p.m. UTC | #7
Hi!

> This is v4 of Syscall User Redirection.  The implementation itself is
> not modified from v3, it only applies the latest round of reviews to the
> selftests.
> 
> __NR_syscalls is not really exported in header files other than
> asm-generic for every architecture, so it felt safer to optionally
> expose it with a fallback to a high value.
> 
> Also, I didn't expose tests for PR_GET as that is not currently
> implemented.  If possible, I'd have it supported by a future patchset,
> since it is not immediately necessary to support this feature.
> 
> Finally, one question: Which tree would this go through?

Should it come with Documentation?

How does it interact with ptrace, seccomp and similar?
									Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Gabriel Krisman Bertazi Aug. 4, 2020, 2:26 p.m. UTC | #8
Pavel Machek <pavel@ucw.cz> writes:

> Hi!
>
>> This is v4 of Syscall User Redirection.  The implementation itself is
>> not modified from v3, it only applies the latest round of reviews to the
>> selftests.
>> 
>> __NR_syscalls is not really exported in header files other than
>> asm-generic for every architecture, so it felt safer to optionally
>> expose it with a fallback to a high value.
>> 
>> Also, I didn't expose tests for PR_GET as that is not currently
>> implemented.  If possible, I'd have it supported by a future patchset,
>> since it is not immediately necessary to support this feature.
>> 
>> Finally, one question: Which tree would this go through?
>
> Should it come with Documentation?

Hi,

Thanks for the review.

I will prepare it for the next iteration.

> How does it interact with ptrace, seccomp and similar?

That is a very good question.

Regarding seccomp, this must take precedence, since the use case is
emulation (it can be invoked with a different ABI) such that seccomp
filtering by syscall number doesn't make sense in the first place.  In
addition, either the syscall is dispatched back to userspace, in which
case there is no resource for seccomp to protect, or the syscall will be
executed, and seccomp will execute next.

Regarding ptrace, I experimented with before and after, and while the
same ABI argument applies, I felt it was easier to debug if I let ptrace
happen for syscalls that are dispatched back to userspace.  In addition,
doing it after ptrace makes the code in syscall_exit_work slightly
simpler, since it doesn't require special handling for this feature.

For PTRACE_SYSEMU in particular, either placing this before or after is
a bit odd.  I don't think there is a right answer for this one, but I
don't see anyone wanting to use these features at the same time.  I
think as long as it is documented behavior, it should be fine either
way.