From: Andy Lutomirski <luto@amacapital.net>
To: Christian Brauner <christian@brauner.io>
Cc: Andy Lutomirski <luto@kernel.org>,
Florian Weimer <fweimer@redhat.com>,
"Eric W. Biederman" <ebiederm@xmission.com>,
LKML <linux-kernel@vger.kernel.org>,
"Serge E. Hallyn" <serge@hallyn.com>,
Jann Horn <jannh@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
Oleg Nesterov <oleg@redhat.com>, Aleksa Sarai <cyphar@cyphar.com>,
Al Viro <viro@zeniv.linux.org.uk>,
Linux FS Devel <linux-fsdevel@vger.kernel.org>,
Linux API <linux-api@vger.kernel.org>,
Daniel Colascione <dancol@google.com>,
Tim Murray <timmurray@google.com>,
linux-man <linux-man@vger.kernel.org>,
Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH v2] signal: add procfd_signal() syscall
Date: Thu, 29 Nov 2018 12:14:05 -0800 [thread overview]
Message-ID: <6E21165F-2C76-4877-ABD9-0C86D55FD6AA@amacapital.net> (raw)
In-Reply-To: <20181129195551.woe2bl3z3yaysqb6@brauner.io>
> On Nov 29, 2018, at 11:55 AM, Christian Brauner <christian@brauner.io> wrote:
>
>> On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote:
>>> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner <christian@brauner.io> wrote:
>>>
>>>> On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski <luto@amacapital.net> wrote:
>>>>
>>>>
>>>>> On Nov 29, 2018, at 4:28 AM, Florian Weimer <fweimer@redhat.com>
>>>> wrote:
>>>>>
>>>>> Disclaimer: I'm looking at this patch because Christian requested it.
>>>>> I'm not a kernel developer.
>>>>>
>>>>> * 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.)
>>>>
>>>> Hah, I missed this part of the patch. Let’s not add new x32 syscall
>>>> numbers.
>>>>
>>>> Also, can we perhaps rework this a bit to get rid of the compat entry
>>>> point? The easier way would be to check in_compat_syscall(). The nicer
>>>> way IMO would be to use the 64-bit structure for 32-bit as well.
>>>
>>> Do you have a syscall which set precedence/did this before I could look at?
>>> Just if you happen to remember one.
>>> Fwiw, I followed the other signal syscalls.
>>> They all introduce compat syscalls.
>>>
>>
>> Not really.
>>
>> Let me try to explain. I have three issues with the approach in your patchset:
>>
>> 1. You're introducing a new syscall, and it behaves differently on
>> 32-bit and 64-bit because the structure you pass in is different.
>> This is necessary for old syscalls where compatibility matters, but
>> maybe we can get rid of it for new syscalls. Could we define a
>> siginfo64_t that is identical to the 64-bit siginfo_t and just use
>> that in all cases?
>>
>> 2. Assuming that #1 doesn't work, then we need compat support. But
>> you're doing it by having two different entry points. Instead, you
>> could have a single entry point that calls in_compat_syscall() to
>> decide which structure to read. This would simplify things because
>> x86 doesn't really support the separate compat entry points, which
>> leads me to #3.
>>
>> 3. The separate x32 numbers are a huge turd that may have security
>> holes and certainly have comprehensibility holes. I will object to
>> any patch that adds a new one (like yours). Fixing #1 or #2 makes
>> this problem go away.
>>
>> Does that make any sense? The #2 fix would be something like:
>>
>> if (in_compat_syscall)
>> copy...user32();
>> else
>> copy_from_user();
>>
>> The #1 fix would add a copy_siginfo_from_user64() or similar.
>
> Thanks very much! That all helped a bunch already! I'll try to go the
> copy_siginfo_from_user64() way first and see if I can make this work. If
> we do this I would however only want to use it for the new syscall first
> and not change all other signal syscalls over to it too. I'd rather keep
> this patchset focussed and small and do such conversions caused by the
> new approach later. Does that sound reasonable?
Absolutely. I don’t think we can change old syscalls — the ABI is set in stone. But for new syscalls, I think the always-64-bit behavior makes sense.
next prev parent reply other threads:[~2018-11-29 20:14 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-20 10:51 [PATCH v2] signal: add procfd_signal() syscall Christian Brauner
2018-11-20 10:51 ` [PATCH v2] procfd_signal.2: document procfd_signal syscall Christian Brauner
2018-11-22 8:00 ` [PATCH v2] signal: add procfd_signal() syscall Serge E. Hallyn
2018-11-22 8:23 ` Aleksa Sarai
2018-11-28 14:05 ` Arnd Bergmann
2018-11-29 12:28 ` Florian Weimer
2018-11-29 16:54 ` Andy Lutomirski
2018-11-29 19:16 ` Christian Brauner
2018-11-29 19:22 ` Andy Lutomirski
2018-11-29 19:55 ` Christian Brauner
2018-11-29 20:14 ` Andy Lutomirski [this message]
2018-11-29 21:02 ` Arnd Bergmann
2018-11-29 21:35 ` Christian Brauner
2018-11-29 21:40 ` Arnd Bergmann
2018-11-30 2:40 ` Aleksa Sarai
2018-12-01 1:25 ` Christian Brauner
2018-11-30 5:13 ` Eric W. Biederman
2018-11-30 6:56 ` Christian Brauner
2018-11-30 11:41 ` Arnd Bergmann
2018-11-30 16:35 ` Andy Lutomirski
2018-11-30 21:57 ` Christian Brauner
2018-11-30 22:09 ` Arnd Bergmann
2018-11-30 22:26 ` Christian Brauner
2018-11-30 23:05 ` Daniel Colascione
2018-11-30 23:12 ` Arnd Bergmann
2018-11-30 23:15 ` Arnd Bergmann
2018-11-30 23:37 ` Christian Brauner
2018-11-30 23:46 ` Andy Lutomirski
2018-12-01 1:20 ` Christian Brauner
2018-11-30 23:53 ` Andy Lutomirski
2018-12-01 8:51 ` Arnd Bergmann
2018-12-01 9:17 ` Christian Brauner
2018-12-01 10:27 ` Arnd Bergmann
2018-12-01 13:41 ` Eric W. Biederman
2018-12-01 14:46 ` Eric W. Biederman
2018-12-01 15:28 ` Eric W. Biederman
2018-12-01 15:52 ` Andy Lutomirski
2018-12-01 16:27 ` Christian Brauner
2018-12-02 0:06 ` Eric W. Biederman
2018-12-02 1:14 ` Andy Lutomirski
2018-12-02 8:52 ` Christian Brauner
2018-11-30 23:52 ` Christian Brauner
2018-12-02 10:03 ` Christian Brauner
2018-12-03 16:57 ` Florian Weimer
2018-12-03 18:02 ` Christian Brauner
2018-12-04 6:03 ` Aleksa Sarai
2018-12-04 12:55 ` Florian Weimer
2018-12-04 13:26 ` Christian Brauner
2018-12-06 18:54 ` Andy Lutomirski
2018-12-06 18:56 ` Florian Weimer
2018-12-06 19:03 ` Christian Brauner
2018-12-25 5:32 ` Lai Jiangshan
2018-12-25 7:11 ` Lai Jiangshan
2018-12-25 12:07 ` Aleksa Sarai
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6E21165F-2C76-4877-ABD9-0C86D55FD6AA@amacapital.net \
--to=luto@amacapital.net \
--cc=akpm@linux-foundation.org \
--cc=christian@brauner.io \
--cc=cyphar@cyphar.com \
--cc=dancol@google.com \
--cc=ebiederm@xmission.com \
--cc=fweimer@redhat.com \
--cc=jannh@google.com \
--cc=keescook@chromium.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-man@vger.kernel.org \
--cc=luto@kernel.org \
--cc=oleg@redhat.com \
--cc=serge@hallyn.com \
--cc=timmurray@google.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).