linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Gabriel Krisman Bertazi <krisman@collabora.com>
To: Andy Lutomirski <luto@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>,
	kernel@collabora.com, Matthew Wilcox <willy@infradead.org>,
	Paul Gofman <gofmanp@gmail.com>,
	Kees Cook <keescook@chromium.org>,
	"open list\:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>
Subject: Re: [PATCH v4 1/2] kernel: Implement selective syscall userspace redirection
Date: Thu, 16 Jul 2020 22:15:35 -0400	[thread overview]
Message-ID: <87wo32j394.fsf@collabora.com> (raw)
In-Reply-To: <CALCETrWdCN5KsRUkrb8VoYGRBhy71P-MAHGWhuJ5y4Z3vByyvg@mail.gmail.com> (Andy Lutomirski's message of "Thu, 16 Jul 2020 17:20:02 -0700")

Andy Lutomirski <luto@kernel.org> writes:

> On Thu, Jul 16, 2020 at 12:31 PM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
>>
>
> This is quite nice.  I have a few comments, though:
>
> You mentioned rt_sigreturn().  Should this automatically exempt the
> kernel-provided signal restorer on architectures (e.g. x86_32) that
> provide one?

That seems reasonable.  Not sure how easy it is to do it, though.

> The amount of syscall entry wiring that arches need to do is IMO
> already a bit out of hand.  Should we instead rename TIF_SECCOMP to
> TIF_SYSCALL_INTERCEPTION and have one generic callback that handles
> seccomp and this new thing?

Considering the previous suggestion from Kees to hide it inside the
tracehook and Thomas rework of this path, I'm not sure what is the best
solution here, but some rework of these flags is due.  Thomas suggested
expanding these flags to 64 bits and having some arch specific and
arch-agnostic flags.  With the storage expansion and arch-agnostic flags,
would this still be desirable?

>> +int do_syscall_user_dispatch(struct pt_regs *regs)
>> +{
>> +       struct syscall_user_dispatch *sd = &current->syscall_dispatch;
>> +       unsigned long ip = instruction_pointer(regs);
>> +       char state;
>> +
>> +       if (likely(ip >= sd->dispatcher_start && ip <= sd->dispatcher_end))
>> +               return 0;
>> +
>> +       if (likely(sd->selector)) {
>> +               if (unlikely(__get_user(state, sd->selector)))
>> +                       do_exit(SIGSEGV);
>> +
>> +               if (likely(state == 0))
>> +                       return 0;
>> +
>> +               if (state != 1)
>> +                       do_exit(SIGSEGV);
>
> This seems a bit extreme and hard to debug if it ever happens.

Makes sense, but I don't see a better way to return the error here.
Maybe a SIGSYS with a different si_errno?  Alternatively, we could
revert to the previous behavior of allowing syscalls on state != 0, that
existed in v1.  What do you think?

-- 
Gabriel Krisman Bertazi

  reply	other threads:[~2020-07-17  2:15 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-16 19:31 [PATCH v4 0/2] Syscall User Redirection Gabriel Krisman Bertazi
2020-07-16 19:31 ` [PATCH v4 1/2] kernel: Implement selective syscall userspace redirection Gabriel Krisman Bertazi
2020-07-16 21:06   ` Matthew Wilcox
2020-07-16 21:26     ` Kees Cook
2020-07-17  0:20   ` Andy Lutomirski
2020-07-17  2:15     ` Gabriel Krisman Bertazi [this message]
2020-07-17  4:48       ` Andy Lutomirski
2020-07-21 12:06         ` Mark Rutland
2020-07-20  9:23     ` Thomas Gleixner
2020-07-20  9:44       ` Will Deacon
2020-07-20 10:08   ` Thomas Gleixner
2020-07-20 13:46     ` Gabriel Krisman Bertazi
2020-07-16 19:31 ` [PATCH v4 2/2] selftests: Add kselftest for syscall user dispatch Gabriel Krisman Bertazi
2020-07-16 20:04 ` [PATCH v4 0/2] Syscall User Redirection Kees Cook
2020-07-16 20:22   ` Christian Brauner
2020-07-16 20:25     ` Kees Cook
2020-07-16 20:29       ` Christian Brauner
2020-07-16 20:30         ` Gabriel Krisman Bertazi
2020-07-16 21:06           ` Carlos O'Donell
2020-08-02 12:01 ` Pavel Machek
2020-08-04 14:26   ` Gabriel Krisman Bertazi

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=87wo32j394.fsf@collabora.com \
    --to=krisman@collabora.com \
    --cc=gofmanp@gmail.com \
    --cc=keescook@chromium.org \
    --cc=kernel@collabora.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=shuah@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH v4 1/2] kernel: Implement selective syscall userspace redirection' \
    /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

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).