Re: [PATCH v3 2/4] x86: suppress SMEP and SMAP while running 32-bit PV guest code

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Keir Fraser <keir@xen.org>, Feng Wu <feng.wu@intel.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v3 2/4] x86: suppress SMEP and SMAP while running 32-bit PV guest code
Date: Fri, 25 Mar 2016 14:01:18 -0400	[thread overview]
Message-ID: <20160325180117.GC20741@char.us.oracle.com> (raw)
In-Reply-To: <56EA72C402000078000DD92F@prv-mh.provo.novell.com>

> @@ -174,10 +174,61 @@ compat_bad_hypercall:
>  /* %rbx: struct vcpu, interrupts disabled */
>  ENTRY(compat_restore_all_guest)
>          ASSERT_INTERRUPTS_DISABLED
> +.Lcr4_orig:
> +        ASM_NOP8 /* testb $3,UREGS_cs(%rsp) */
> +        ASM_NOP2 /* jpe   .Lcr4_alt_end */
> +        ASM_NOP8 /* mov   CPUINFO_cr4...(%rsp), %rax */
> +        ASM_NOP6 /* and   $..., %rax */
> +        ASM_NOP8 /* mov   %rax, CPUINFO_cr4...(%rsp) */
> +        ASM_NOP3 /* mov   %rax, %cr4 */
> +.Lcr4_orig_end:
> +        .pushsection .altinstr_replacement, "ax"
> +.Lcr4_alt:
> +        testb $3,UREGS_cs(%rsp)
> +        jpe   .Lcr4_alt_end

This would jump if the last operation had even bits set. And the
'testb' is 'and' operation which would give us the '011' (for $3).

Why not just depend on the ZF ? Other places that test UREGS_cs()
look to be using that?

> +        mov   CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp), %rax
> +        and   $~(X86_CR4_SMEP|X86_CR4_SMAP), %rax
> +        mov   %rax, CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp)
> +        mov   %rax, %cr4
> +.Lcr4_alt_end:
> +        .section .altinstructions, "a"
> +        altinstruction_entry .Lcr4_orig, .Lcr4_alt, X86_FEATURE_SMEP, \
> +                             (.Lcr4_orig_end - .Lcr4_orig), \
> +                             (.Lcr4_alt_end - .Lcr4_alt)
> +        altinstruction_entry .Lcr4_orig, .Lcr4_alt, X86_FEATURE_SMAP, \
> +                             (.Lcr4_orig_end - .Lcr4_orig), \
> +                             (.Lcr4_alt_end - .Lcr4_alt)
> +        .popsection
>          RESTORE_ALL adj=8 compat=1
>  .Lft0:  iretq
>          _ASM_PRE_EXTABLE(.Lft0, handle_exception)
>  
> +/* This mustn't modify registers other than %rax. */
> +ENTRY(cr4_pv32_restore)
> +        push  %rdx
> +        GET_CPUINFO_FIELD(cr4, %rdx)
> +        mov   (%rdx), %rax
> +        test  $X86_CR4_SMEP|X86_CR4_SMAP,%eax
> +        jnz   0f
> +        or    cr4_pv32_mask(%rip), %rax
> +        mov   %rax, %cr4
> +        mov   %rax, (%rdx)

Here you leave %rax with the cr4_pv32_mask value, but:

> +        pop   %rdx
> +        ret
> +0:
> +#ifndef NDEBUG
> +        /* Check that _all_ of the bits intended to be set actually are. */
> +        mov   %cr4, %rax
> +        and   cr4_pv32_mask(%rip), %eax
> +        cmp   cr4_pv32_mask(%rip), %eax
> +        je    1f
> +        BUG
> +1:
> +#endif
> +        pop   %rdx
> +        xor   %eax, %eax

.. Here you clear it. Any particular reason?

> +        ret
> +
>  /* %rdx: trap_bounce, %rbx: struct vcpu */
>  ENTRY(compat_post_handle_exception)
>          testb $TBF_EXCEPTION,TRAPBOUNCE_flags(%rdx)
.. snip..
> -.macro LOAD_C_CLOBBERED compat=0
> +.macro LOAD_C_CLOBBERED compat=0 ax=1
>  .if !\compat
>          movq  UREGS_r11(%rsp),%r11
>          movq  UREGS_r10(%rsp),%r10
>          movq  UREGS_r9(%rsp),%r9
>          movq  UREGS_r8(%rsp),%r8
> -.endif
> +.if \ax
>          movq  UREGS_rax(%rsp),%rax
> +.endif

Why the .endif here considering you are doing an:

> +.elseif \ax

an else if here?
> +        movl  UREGS_rax(%rsp),%eax
> +.endif

Actually, Why two 'if ax' ? checks?

Or am I reading this incorrect?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel