|
Message-ID: <CAJcbSZENjCgn3TNDGzCBBSOXMvbTX8mBO7Trugdb5VRzhxwmHg@mail.gmail.com> Date: Thu, 27 Apr 2017 07:16:41 -0700 From: Thomas Garnier <thgarnie@...gle.com> To: Ingo Molnar <mingo@...nel.org> Cc: Martin Schwidefsky <schwidefsky@...ibm.com>, Heiko Carstens <heiko.carstens@...ibm.com>, Dave Hansen <dave.hansen@...el.com>, Arnd Bergmann <arnd@...db.de>, David Howells <dhowells@...hat.com>, René Nyffenegger <mail@...enyffenegger.ch>, Andrew Morton <akpm@...ux-foundation.org>, "Paul E . McKenney" <paulmck@...ux.vnet.ibm.com>, "Eric W . Biederman" <ebiederm@...ssion.com>, Thomas Gleixner <tglx@...utronix.de>, Oleg Nesterov <oleg@...hat.com>, Pavel Tikhomirov <ptikhomirov@...tuozzo.com>, Ingo Molnar <mingo@...hat.com>, "H . Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>, Paolo Bonzini <pbonzini@...hat.com>, Kees Cook <keescook@...omium.org>, Rik van Riel <riel@...hat.com>, Josh Poimboeuf <jpoimboe@...hat.com>, Borislav Petkov <bp@...en8.de>, Brian Gerst <brgerst@...il.com>, "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>, Christian Borntraeger <borntraeger@...ibm.com>, Russell King <linux@...linux.org.uk>, Will Deacon <will.deacon@....com>, Catalin Marinas <catalin.marinas@....com>, Mark Rutland <mark.rutland@....com>, James Morse <james.morse@....com>, linux-s390 <linux-s390@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, Linux API <linux-api@...r.kernel.org>, "the arch/x86 maintainers" <x86@...nel.org>, linux-arm-kernel@...ts.infradead.org, Kernel Hardening <kernel-hardening@...ts.openwall.com> Subject: Re: [PATCH v8 1/4] syscalls: Verify address limit before returning to user-mode On Wed, Apr 26, 2017 at 11:49 PM, Ingo Molnar <mingo@...nel.org> wrote: > > * Thomas Garnier <thgarnie@...gle.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
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.