Re: [PATCH] x86: entry_32.S: change ESPFIX test to not touch PT_OLDSS(%esp)

[Andy Lutomirski]

On Mon, Mar 9, 2015 at 7:05 AM, Denys Vlasenko <dvlasenk@xxxxxxxxxx> 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@xxxxxxxxxx>
> CC: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> CC: Steven Rostedt <rostedt@xxxxxxxxxxx>
> CC: Ingo Molnar <mingo@xxxxxxxxxx>
> CC: Borislav Petkov <bp@xxxxxxxxx>
> CC: "H. Peter Anvin" <hpa@xxxxxxxxx>
> CC: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> CC: Oleg Nesterov <oleg@xxxxxxxxxx>
> CC: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> CC: Alexei Starovoitov <ast@xxxxxxxxxxxx>
> CC: Will Drewry <wad@xxxxxxxxxxxx>
> CC: Kees Cook <keescook@xxxxxxxxxxxx>
> CC: x86@xxxxxxxxxx
> CC: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/