LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andy Lutomirski <luto@amacapital.net>
To: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ingo Molnar <mingo@kernel.org>, Borislav Petkov <bp@alien8.de>,
	"H. Peter Anvin" <hpa@zytor.com>, Oleg Nesterov <oleg@redhat.com>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	Alexei Starovoitov <ast@plumgrid.com>,
	Will Drewry <wad@chromium.org>, Kees Cook <keescook@chromium.org>,
	X86 ML <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] x86: entry_32.S: change ESPFIX test to not touch PT_OLDSS(%esp)
Date: Mon, 9 Mar 2015 07:18:45 -0700	[thread overview]
Message-ID: <CALCETrV0zvMjZcd=Ctvxxr_1Zr37HUnbAyddpTuzBuKOWeP8Tw@mail.gmail.com> (raw)
In-Reply-To: <1425909943-14687-1-git-send-email-dvlasenk@redhat.com>

On Mon, Mar 9, 2015 at 7:05 AM, Denys Vlasenko <dvlasenk@redhat.com> 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 <ldt_ss>
>     New:
>      1e6:   0f ba 64 24 38 11       btl    $0x11,0x38(%esp)
>      1ec:   72 0e                   jb     1fc <restore_nocheck>
>      1ee:   f6 44 24 34 03          testb  $0x3,0x34(%esp)
>      1f3:   74 07                   je     1fc <restore_nocheck>
>      1f5:   f6 44 24 40 04          testb  $0x4,0x40(%esp)
>      1fa:   75 0f                   jne    20b <ldt_ss>
>
> Patch is run-tested.
>
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Alexei Starovoitov <ast@plumgrid.com>
> CC: Will Drewry <wad@chromium.org>
> CC: Kees Cook <keescook@chromium.org>
> 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

  reply	other threads:[~2015-03-09 14:19 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-09 14:05 Denys Vlasenko
2015-03-09 14:18 ` Andy Lutomirski [this message]
2015-03-09 15:00   ` Denys Vlasenko
2015-03-09 15:09     ` Andy Lutomirski
2015-03-09 19:31       ` Denys Vlasenko
2015-03-09 15:13     ` Ingo Molnar
2015-03-09 15:18       ` Andy Lutomirski
2015-03-09 15:47       ` Steven Rostedt
2015-03-09 15:54         ` Ingo Molnar
2015-03-09 16:08 ` Linus Torvalds
2015-03-09 16:28   ` Denys Vlasenko
2015-03-09 16:44     ` Linus Torvalds
2015-03-09 17:44       ` H. Peter Anvin
2015-03-09 19:13         ` Andy Lutomirski
2015-03-09 19:26           ` H. Peter Anvin
2015-03-09 19:51             ` Andy Lutomirski
2015-03-09 17:42   ` H. Peter Anvin
2015-03-09 17:45     ` Andy Lutomirski
2015-03-09 17:59       ` Linus Torvalds
2015-03-09 18:04         ` Andy Lutomirski
2015-03-09 18:16           ` Linus Torvalds
2015-03-09 18:32             ` Denys Vlasenko
2015-03-09 18:36             ` Andy Lutomirski
2015-03-10  6:25               ` 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='CALCETrV0zvMjZcd=Ctvxxr_1Zr37HUnbAyddpTuzBuKOWeP8Tw@mail.gmail.com' \
    --to=luto@amacapital.net \
    --cc=ast@plumgrid.com \
    --cc=bp@alien8.de \
    --cc=dvlasenk@redhat.com \
    --cc=fweisbec@gmail.com \
    --cc=hpa@zytor.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=rostedt@goodmis.org \
    --cc=torvalds@linux-foundation.org \
    --cc=wad@chromium.org \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH] x86: entry_32.S: change ESPFIX test to not touch PT_OLDSS(%esp)' \
    /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).