LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Lutomirski <luto@kernel.org>
To: Christian Brauner <christian@brauner.io>
Cc: 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 Lutomirski <luto@kernel.org>,
	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 11:22:58 -0800
Message-ID: <CALCETrWnQNMQcCmFZrftVVYgAMW6DT3gyxvVb_v9_enUCUkHiw@mail.gmail.com> (raw)
In-Reply-To: <993B98AC-51DF-4131-AF7F-7DA2A7F485F1@brauner.io>

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.

  reply index

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-20 10:51 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 [this message]
2018-11-29 19:55         ` Christian Brauner
2018-11-29 20:14           ` Andy Lutomirski
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=CALCETrWnQNMQcCmFZrftVVYgAMW6DT3gyxvVb_v9_enUCUkHiw@mail.gmail.com \
    --to=luto@kernel.org \
    --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=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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git