From: Thomas Garnier <thgarnie@google.com>
To: Ingo Molnar <mingo@kernel.org>
Cc: "Martin Schwidefsky" <schwidefsky@de.ibm.com>,
"Heiko Carstens" <heiko.carstens@de.ibm.com>,
"Dave Hansen" <dave.hansen@intel.com>,
"Arnd Bergmann" <arnd@arndb.de>,
"David Howells" <dhowells@redhat.com>,
"René Nyffenegger" <mail@renenyffenegger.ch>,
"Andrew Morton" <akpm@linux-foundation.org>,
"Paul E . McKenney" <paulmck@linux.vnet.ibm.com>,
"Eric W . Biederman" <ebiederm@xmission.com>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Oleg Nesterov" <oleg@redhat.com>,
"Pavel Tikhomirov" <ptikhomirov@virtuozzo.com>,
"Ingo Molnar" <mingo@redhat.com>,
"H . Peter Anvin" <hpa@zytor.com>,
"Andy Lutomirski" <luto@kernel.org>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Kees Cook" <keescook@chromium.org>,
"Rik van Riel" <riel@redhat.com>,
"Josh Poimboeuf" <jpoimboe@redhat.com>,
"Borislav Petkov" <bp@alien8.de>,
"Brian Gerst" <brgerst@gmail.com>,
"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
"Christian Borntraeger" <borntraeger@de.ibm.com>,
"Russell King" <linux@armlinux.org.uk>,
"Will Deacon" <will.deacon@arm.com>,
"Catalin Marinas" <catalin.marinas@arm.com>,
"Mark Rutland" <mark.rutland@arm.com>,
"James Morse" <james.morse@arm.com>,
linux-s390 <linux-s390@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
"Linux API" <linux-api@vger.kernel.org>,
"the arch/x86 maintainers" <x86@kernel.org>,
linux-arm-kernel@lists.infradead.org,
"Kernel Hardening" <kernel-hardening@lists.openwall.com>
Subject: Re: [PATCH v8 1/4] syscalls: Verify address limit before returning to user-mode
Date: Thu, 27 Apr 2017 07:16:41 -0700 [thread overview]
Message-ID: <CAJcbSZENjCgn3TNDGzCBBSOXMvbTX8mBO7Trugdb5VRzhxwmHg@mail.gmail.com> (raw)
In-Reply-To: <20170427064917.phwo6yl4v4q43fql@gmail.com>
On Wed, Apr 26, 2017 at 11:49 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Thomas Garnier <thgarnie@google.com> wrote:
>
>> +
>> +/*
>> + * Called before coming back to user-mode. Returning to user-mode with an
>> + * address limit different than USER_DS can allow to overwrite kernel memory.
>> + */
>> +static inline void addr_limit_check_syscall(void)
>> +{
>> + BUG_ON(!segment_eq(get_fs(), USER_DS));
>> +}
>> +
>> +#ifndef CONFIG_ADDR_LIMIT_CHECK
>> +#define __CHECK_USERMODE_SYSCALL() \
>> + bool user_caller = segment_eq(get_fs(), USER_DS)
>> +#define __VERIFY_ADDR_LIMIT() \
>> + if (user_caller) addr_limit_check_syscall()
>> +#else
>> +#define __CHECK_USERMODE_SYSCALL()
>> +#define __VERIFY_ADDR_LIMIT()
>> +asmlinkage void addr_limit_check_failed(void) __noreturn;
>> +#endif
>
> _Please_ harmonize all the externally exposed names and symbols.
>
> There's no reason for this mismash of names:
>
> CONFIG_ADDR_LIMIT_CHECK
>
> __CHECK_USERMODE_SYSCALL
> __VERIFY_ADDR_LIMIT
>
> When we could just as easily name them consistently, along the existing pattern:
>
> CONFIG_ADDR_LIMIT_CHECK
>
> __SYSCALL_ADDR_LIMIT_CHECK
> __ADDR_LIMIT_CHECK
>
> which should fit into existing nomenclature:
>
>> #define __SYSCALL_DEFINEx(x, name, ...) \
>
> But even with that fixed, the whole construct still looks pretty weird:
>
>> { \
>> - long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
>> + long ret; \
>> + __CHECK_USERMODE_SYSCALL(); \
>> + ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
>> + __ADDR_LIMIT_CHECK(); \
>> __MAP(x,__SC_TEST,__VA_ARGS__); \
>> __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
>> return ret; \
>
> I think something like this would be more natural to read:
>
>> + ADDR_LIMIT_CHECK_PRE(); \
>> + ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
>> + ADDR_LIMIT_CHECK_POST(); \
>
> it's a clear pre/post construct. Also note the lack of double underscores.
I think this construct makes more sense because the first macro check
if the syscall was called by user-mode. I will send an update for this
on this thread.
>
> BTW., a further simplification would be:
>
> #ifndef ADDR_LIMIT_CHECK_PRE
> # define ADDR_LIMIT_CHECK_PRE ...
> #endif
>
> This way architectures could override this generic functionality simply by
> defining the helpers. Architectures that don't do that get the generic version.
I don't think architectures need to do that. The optimizations are
embedding the checks on their architecture-specific code to make it
faster and remove the size impact. The pre/post is fine for the rest.
>
> Thanks,
>
> Ingo
--
Thomas
next prev parent reply other threads:[~2017-04-27 14:16 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-26 18:34 [PATCH v8 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier
2017-04-26 18:34 ` [PATCH v8 2/4] x86/syscalls: Optimize address limit check Thomas Garnier
2017-04-26 18:34 ` [PATCH v8 3/4] arm/syscalls: " Thomas Garnier
2017-04-26 18:34 ` [PATCH v8 4/4] arm64/syscalls: " Thomas Garnier
2017-04-27 6:49 ` [PATCH v8 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar
2017-04-27 14:16 ` Thomas Garnier [this message]
2017-04-27 14:42 ` Thomas Garnier
2017-04-28 6:35 ` Ingo Molnar
2017-04-28 6:33 ` Ingo Molnar
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=CAJcbSZENjCgn3TNDGzCBBSOXMvbTX8mBO7Trugdb5VRzhxwmHg@mail.gmail.com \
--to=thgarnie@google.com \
--cc=akpm@linux-foundation.org \
--cc=arnd@arndb.de \
--cc=borntraeger@de.ibm.com \
--cc=bp@alien8.de \
--cc=brgerst@gmail.com \
--cc=catalin.marinas@arm.com \
--cc=dave.hansen@intel.com \
--cc=dhowells@redhat.com \
--cc=ebiederm@xmission.com \
--cc=heiko.carstens@de.ibm.com \
--cc=hpa@zytor.com \
--cc=james.morse@arm.com \
--cc=jpoimboe@redhat.com \
--cc=keescook@chromium.org \
--cc=kernel-hardening@lists.openwall.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=luto@kernel.org \
--cc=mail@renenyffenegger.ch \
--cc=mark.rutland@arm.com \
--cc=mingo@kernel.org \
--cc=mingo@redhat.com \
--cc=oleg@redhat.com \
--cc=paulmck@linux.vnet.ibm.com \
--cc=pbonzini@redhat.com \
--cc=ptikhomirov@virtuozzo.com \
--cc=riel@redhat.com \
--cc=schwidefsky@de.ibm.com \
--cc=tglx@linutronix.de \
--cc=will.deacon@arm.com \
--cc=x86@kernel.org \
/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).