[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJcbSZGjos1K9qvQWPTpr4COEcnW6Sn_jo7hCezGbh-BhPAH7w@mail.gmail.com>
Date: Tue, 14 Mar 2017 09:51:55 -0700
From: Thomas Garnier <thgarnie@...gle.com>
To: "H. Peter Anvin" <hpa@...or.com>
Cc: Andy Lutomirski <luto@...capital.net>, Ingo Molnar <mingo@...nel.org>,
Martin Schwidefsky <schwidefsky@...ibm.com>, Heiko Carstens <heiko.carstens@...ibm.com>,
David Howells <dhowells@...hat.com>, Arnd Bergmann <arnd@...db.de>, Al Viro <viro@...iv.linux.org.uk>,
Dave Hansen <dave.hansen@...el.com>, René Nyffenegger <mail@...enyffenegger.ch>,
Andrew Morton <akpm@...ux-foundation.org>, Kees Cook <keescook@...omium.org>,
"Paul E . McKenney" <paulmck@...ux.vnet.ibm.com>, Andy Lutomirski <luto@...nel.org>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>, Nicolas Pitre <nicolas.pitre@...aro.org>,
Petr Mladek <pmladek@...e.com>, Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Sergey Senozhatsky <sergey.senozhatsky@...il.com>, Helge Deller <deller@....de>,
Rik van Riel <riel@...hat.com>, John Stultz <john.stultz@...aro.org>,
Thomas Gleixner <tglx@...utronix.de>, Oleg Nesterov <oleg@...hat.com>,
Stephen Smalley <sds@...ho.nsa.gov>, Pavel Tikhomirov <ptikhomirov@...tuozzo.com>,
Frederic Weisbecker <fweisbec@...il.com>, Stanislav Kinsburskiy <skinsbursky@...tuozzo.com>,
Ingo Molnar <mingo@...hat.com>, Paolo Bonzini <pbonzini@...hat.com>,
Dmitry Safonov <dsafonov@...tuozzo.com>, Borislav Petkov <bp@...en8.de>,
Josh Poimboeuf <jpoimboe@...hat.com>, Brian Gerst <brgerst@...il.com>, Jan Beulich <JBeulich@...e.com>,
Christian Borntraeger <borntraeger@...ibm.com>, Fenghua Yu <fenghua.yu@...el.com>,
He Chen <he.chen@...ux.intel.com>, Russell King <linux@...linux.org.uk>,
Vladimir Murzin <vladimir.murzin@....com>, Will Deacon <will.deacon@....com>,
Catalin Marinas <catalin.marinas@....com>, Mark Rutland <mark.rutland@....com>,
James Morse <james.morse@....com>, "David A . Long" <dave.long@...aro.org>,
Pratyush Anand <panand@...hat.com>, Laura Abbott <labbott@...hat.com>,
Andre Przywara <andre.przywara@....com>, Chris Metcalf <cmetcalf@...lanox.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" <linux-arm-kernel@...ts.infradead.org>,
Kernel Hardening <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH v3 2/4] x86/syscalls: Specific usage of verify_pre_usermode_state
On Tue, Mar 14, 2017 at 9:44 AM, H. Peter Anvin <hpa@...or.com> wrote:
> On 03/14/17 09:29, Thomas Garnier wrote:
>>
>> We want to enforce the address limit by default, not only when
>> CONFIG_BUG_ON_DATA_CORRUPTION is enabled. I tested this one:
>>
>> /* Check user-mode state on fast path return. */
>> movq PER_CPU_VAR(current_task), %rax
>> btq $63, TASK_addr_limit(%rax)
>> jnc 1f
>> #ifdef CONFIG_BUG_ON_DATA_CORRUPTION
>> call syscall_return_slowpath
>> jmp return_from_SYSCALL_64
>> #else
>> movq $TASK_SIZE_MAX, %rcx
>> movq %rcx, TASK_addr_limit(%rax)
>> #endif
>> 1:
>>
>> I saw that syscall_return_slowpath is supposed to be called not jumped
>> to. I could just call verify_pre_usermode_state that would be about
>> the same.
>
> I wanted to comment on that thing: why on earth isn't
> verify_pre_usermode_state() an inline? Making it an out-of-line
> function adds pointless extra overhead to the C code when we are talking
> about a few instructions.
Because outside of arch specific implementation it is called by each
syscall handler. it will increase the code size a lot.
>
> Second, you never do a branch-around to handle an exceptional condition
> on the fast path: you jump *out of line* to handle the special
> condition; a forward branch is preferred since it is slightly more
> likely to be predicted not taken.
>
> Now, I finally had a chance to actually look at the full file (I was
> preoccupied yesterday), and am a bit disappointed, to say the least.
>
> First of all, the jump target you need is only a handful of instructions
> further down the code path; you need to do *exactly* that is done when
> the test of _TIF_ALLWORK_MASK right above is tested! Not only that, but
> you already have PER_CPU_VAR(current_task) in %r11 just ready to be
> used! This was all in the three instructions immediately prior to the
> code you modified...
Correct.
>
> So, all you'd need would be:
>
> movq $TASK_SIZE_MAX, %rcx
> cmpq %rcx, TASK_addr_limit(%r11)
> jne 1f
>
> We even get a short jump instruction!
>
> (Using bt saves one more instruction, but see previous caveats about it.)
Okay that seems fair to me. Andy what do you think? (Given you
suggested the bt). Ingo?
Thanks for the feedback.
>
> -hpa
>
>
>
>
--
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.
