From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932911AbbCIOTK (ORCPT ); Mon, 9 Mar 2015 10:19:10 -0400 Received: from mail-lb0-f169.google.com ([209.85.217.169]:45624 "EHLO mail-lb0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932070AbbCIOTI (ORCPT ); Mon, 9 Mar 2015 10:19:08 -0400 MIME-Version: 1.0 In-Reply-To: <1425909943-14687-1-git-send-email-dvlasenk@redhat.com> References: <1425909943-14687-1-git-send-email-dvlasenk@redhat.com> From: Andy Lutomirski Date: Mon, 9 Mar 2015 07:18:45 -0700 Message-ID: Subject: Re: [PATCH] x86: entry_32.S: change ESPFIX test to not touch PT_OLDSS(%esp) To: Denys Vlasenko Cc: Linus Torvalds , Steven Rostedt , Ingo Molnar , Borislav Petkov , "H. Peter Anvin" , Oleg Nesterov , Frederic Weisbecker , Alexei Starovoitov , Will Drewry , Kees Cook , X86 ML , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 9, 2015 at 7:05 AM, Denys Vlasenko wrote: > Old code was trying to avoid having three branch insns, > but instead it has a chain of six insns where each insn > depends on previos one. > > And it was touching PT_OLDSS(%esp) unconditionally, even when it may > contain bogus data. Elsewhere we have to jump thru hoops > just to make sure here PT_OLDSS(%esp) is at least in a valid page. > > All this just to have one branch instead of three? > > The new code simply checks each condition. > All three checks can run in parallel on an out-of-order CPU. > Most of the time, none of branches will be taken. > > Comparison of object code: > Old: > 1e6: 8b 44 24 38 mov 0x38(%esp),%eax > 1ea: 8a 64 24 40 mov 0x40(%esp),%ah > 1ee: 8a 44 24 34 mov 0x34(%esp),%al > 1f2: 25 03 04 02 00 and $0x20403,%eax > 1f7: 3d 03 04 00 00 cmp $0x403,%eax > 1fc: 74 0f je 20d > New: > 1e6: 0f ba 64 24 38 11 btl $0x11,0x38(%esp) > 1ec: 72 0e jb 1fc > 1ee: f6 44 24 34 03 testb $0x3,0x34(%esp) > 1f3: 74 07 je 1fc > 1f5: f6 44 24 40 04 testb $0x4,0x40(%esp) > 1fa: 75 0f jne 20b > > Patch is run-tested. > > Signed-off-by: Denys Vlasenko > CC: Linus Torvalds > CC: Steven Rostedt > CC: Ingo Molnar > CC: Borislav Petkov > CC: "H. Peter Anvin" > CC: Andy Lutomirski > CC: Oleg Nesterov > CC: Frederic Weisbecker > CC: Alexei Starovoitov > CC: Will Drewry > CC: Kees Cook > CC: x86@kernel.org > CC: linux-kernel@vger.kernel.org > --- > arch/x86/kernel/entry_32.S | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > index e33ba51..0a4996b 100644 > --- a/arch/x86/kernel/entry_32.S > +++ b/arch/x86/kernel/entry_32.S > @@ -516,16 +516,15 @@ restore_all: > TRACE_IRQS_IRET > restore_all_notrace: > #ifdef CONFIG_X86_ESPFIX32 > - movl PT_EFLAGS(%esp), %eax # mix EFLAGS, SS and CS > - # Warning: PT_OLDSS(%esp) contains the wrong/random values if we > - # are returning to the kernel. > - # See comments in process.c:copy_thread() for details. > - movb PT_OLDSS(%esp), %ah > - movb PT_CS(%esp), %al > - andl $(X86_EFLAGS_VM | (SEGMENT_TI_MASK << 8) | SEGMENT_RPL_MASK), %eax > - cmpl $((SEGMENT_LDT << 8) | USER_RPL), %eax > CFI_REMEMBER_STATE > - je ldt_ss # returning to user-space with LDT SS > + btl $X86_EFLAGS_VM_BIT,PT_EFLAGS(%esp) > + jc restore_nocheck # VM set, not it This seems useless. In vm86 mode, espfix should work fine (even if pointlessly), CS won't have the two low bits set, and SS won't reference the LDT because it's not a selector at all. That being said, what ends up in the high bits of esp when we iret to vm86 mode? Do we actually need espfix on all returns to vm86 mode? Your patch passes my sigreturn test, so it at least results in functional espvix32 in the non-vm86 case. --Andy > + testb $3,PT_CS(%esp) > + jz restore_nocheck # CPL0, not it > + # Note: we access PT_OLDSS only when we know it exists. > + # If PT_CS is from CPL0, it does not. > + testb $SEGMENT_TI_MASK,PT_OLDSS(%esp) > + jnz ldt_ss # returning to user-space with LDT SS > #endif > restore_nocheck: > RESTORE_REGS 4 # skip orig_eax/error_code > -- > 1.8.1.4 > -- Andy Lutomirski AMA Capital Management, LLC