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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 04/03/16 11:27, Jan Beulich wrote:
> Since such guests' kernel code runs in ring 1, their memory accesses,
> at the paging layer, are supervisor mode ones, and hence subject to
> SMAP/SMEP checks. Such guests cannot be expected to be aware of those
> two features though (and so far we also don't expose the respective
> feature flags), and hence may suffer page faults they cannot deal with.
>
> While the placement of the re-enabling slightly weakens the intended
> protection, it was selected such that 64-bit paths would remain
> unaffected where possible. At the expense of a further performance hit
> the re-enabling could be put right next to the CLACs.
>
> Note that this introduces a number of extra TLB flushes - CR4.SMEP
> transitioning from 0 to 1 always causes a flush, and it transitioning
> from 1 to 0 may also do.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -67,6 +67,8 @@ boolean_param("smep", opt_smep);
>  static bool_t __initdata opt_smap = 1;
>  boolean_param("smap", opt_smap);
>  
> +unsigned long __read_mostly cr4_smep_smap_mask;

Are we liable to gain any other cr4 features which would want to be
included in this?  Might it be wise to chose a slightly more generic
name such as cr4_pv32_mask ?

>  #define SHADOW_BYTES 16 /* Shadow EIP + shadow hypercall # */
>  #else
>          /* Relocate argument registers and zero-extend to 64 bits. */
> -        movl  %eax,%eax              /* Hypercall #  */
>          xchgl %ecx,%esi              /* Arg 2, Arg 4 */
>          movl  %edx,%edx              /* Arg 3        */
>          movl  %edi,%r8d              /* Arg 5        */
> @@ -174,10 +174,43 @@ compat_bad_hypercall:
>  /* %rbx: struct vcpu, interrupts disabled */
>  ENTRY(compat_restore_all_guest)
>          ASSERT_INTERRUPTS_DISABLED
> +.Lcr4_orig:
> +        ASM_NOP3 /* mov   %cr4, %rax */
> +        ASM_NOP6 /* and   $..., %rax */
> +        ASM_NOP3 /* mov   %rax, %cr4 */
> +        .pushsection .altinstr_replacement, "ax"
> +.Lcr4_alt:
> +        mov   %cr4, %rax
> +        and   $~(X86_CR4_SMEP|X86_CR4_SMAP), %rax
> +        mov   %rax, %cr4
> +.Lcr4_alt_end:
> +        .section .altinstructions, "a"
> +        altinstruction_entry .Lcr4_orig, .Lcr4_alt, X86_FEATURE_SMEP, 12, \
> +                             (.Lcr4_alt_end - .Lcr4_alt)
> +        altinstruction_entry .Lcr4_orig, .Lcr4_alt, X86_FEATURE_SMAP, 12, \
> +                             (.Lcr4_alt_end - .Lcr4_alt)

These 12's look as if they should be (.Lcr4_alt - .Lcr4_orig).

> +        .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_smep_smap_restore)
> +        mov   %cr4, %rax
> +        test  $X86_CR4_SMEP|X86_CR4_SMAP,%eax
> +        jnz   0f
> +        or    cr4_smep_smap_mask(%rip), %rax
> +        mov   %rax, %cr4
> +        ret
> +0:
> +        and   cr4_smep_smap_mask(%rip), %eax
> +        cmp   cr4_smep_smap_mask(%rip), %eax
> +        je    1f
> +        BUG

What is the purpose of this bugcheck? It looks like it is catching a
mismatch of masked options, but I am not completely sure.

For all other ASM level BUG's, I put a short comment on the same line,
to aid people who hit the bug.

> +1:
> +        xor   %eax, %eax
> +        ret
> +
>  /* %rdx: trap_bounce, %rbx: struct vcpu */
>  ENTRY(compat_post_handle_exception)
>          testb $TBF_EXCEPTION,TRAPBOUNCE_flags(%rdx)
> @@ -190,6 +223,7 @@ ENTRY(compat_post_handle_exception)
>  /* See lstar_enter for entry register state. */
>  ENTRY(cstar_enter)
>          sti
> +        SMEP_SMAP_RESTORE
>          movq  8(%rsp),%rax /* Restore %rax. */
>          movq  $FLAT_KERNEL_SS,8(%rsp)
>          pushq %r11
> @@ -225,6 +259,7 @@ UNLIKELY_END(compat_syscall_gpf)
>          jmp   .Lcompat_bounce_exception
>  
>  ENTRY(compat_sysenter)
> +        SMEP_SMAP_RESTORE
>          movq  VCPU_trap_ctxt(%rbx),%rcx
>          cmpb  $TRAP_gp_fault,UREGS_entry_vector(%rsp)
>          movzwl VCPU_sysenter_sel(%rbx),%eax
> @@ -238,6 +273,7 @@ ENTRY(compat_sysenter)
>          jmp   compat_test_all_events
>  
>  ENTRY(compat_int80_direct_trap)
> +        SMEP_SMAP_RESTORE
>          call  compat_create_bounce_frame
>          jmp   compat_test_all_events
>  
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -434,6 +434,7 @@ ENTRY(dom_crash_sync_extable)
>  
>  ENTRY(common_interrupt)
>          SAVE_ALL CLAC
> +        SMEP_SMAP_RESTORE
>          movq %rsp,%rdi
>          callq do_IRQ
>          jmp ret_from_intr
> @@ -454,13 +455,64 @@ ENTRY(page_fault)
>  GLOBAL(handle_exception)
>          SAVE_ALL CLAC
>  handle_exception_saved:
> +        GET_CURRENT(%rbx)
>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>          jz    exception_with_ints_disabled
> -        sti
> +
> +.Lsmep_smap_orig:
> +        jmp   0f
> +        .if 0 // GAS bug (affecting at least 2.22 ... 2.26)
> +        .org .Lsmep_smap_orig + (.Lsmep_smap_alt_end - .Lsmep_smap_alt), 0xcc
> +        .else
> +        // worst case: rex + opcode + modrm + 4-byte displacement
> +        .skip (1 + 1 + 1 + 4) - 2, 0xcc
> +        .endif

Which bug is this?  How does it manifest.  More generally, what is this
alternative trying to achieve?

> +        .pushsection .altinstr_replacement, "ax"
> +.Lsmep_smap_alt:
> +        mov   VCPU_domain(%rbx),%rax
> +.Lsmep_smap_alt_end:
> +        .section .altinstructions, "a"
> +        altinstruction_entry .Lsmep_smap_orig, .Lsmep_smap_alt, \
> +                             X86_FEATURE_SMEP, \
> +                             (.Lsmep_smap_alt_end - .Lsmep_smap_alt), \
> +                             (.Lsmep_smap_alt_end - .Lsmep_smap_alt)
> +        altinstruction_entry .Lsmep_smap_orig, .Lsmep_smap_alt, \
> +                             X86_FEATURE_SMAP, \
> +                             (.Lsmep_smap_alt_end - .Lsmep_smap_alt), \
> +                             (.Lsmep_smap_alt_end - .Lsmep_smap_alt)
> +        .popsection
> +
> +        testb $3,UREGS_cs(%rsp)
> +        jz    0f
> +        cmpb  $0,DOMAIN_is_32bit_pv(%rax)

This comparison is wrong on hardware lacking SMEP and SMAP, as the "mov
VCPU_domain(%rbx),%rax" won't have happened.

> +        je    0f
> +        call  cr4_smep_smap_restore
> +        /*
> +         * An NMI or #MC may occur between clearing CR4.SMEP and CR4.SMAP in
> +         * compat_restore_all_guest and it actually returning to guest
> +         * context, in which case the guest would run with the two features
> +         * enabled. The only bad that can happen from this is a kernel mode
> +         * #PF which the guest doesn't expect. Rather than trying to make the
> +         * NMI/#MC exit path honor the intended CR4 setting, simply check
> +         * whether the wrong CR4 was in use when the #PF occurred, and exit
> +         * back to the guest (which will in turn clear the two CR4 bits) to
> +         * re-execute the instruction. If we get back here, the CR4 bits
> +         * should then be found clear (unless another NMI/#MC occurred at
> +         * exactly the right time), and we'll continue processing the
> +         * exception as normal.
> +         */
> +        test  %rax,%rax
> +        jnz   0f
> +        mov   $PFEC_page_present,%al
> +        cmpb  $TRAP_page_fault,UREGS_entry_vector(%rsp)
> +        jne   0f
> +        xor   UREGS_error_code(%rsp),%eax
> +        test  $~(PFEC_write_access|PFEC_insn_fetch),%eax
> +        jz    compat_test_all_events
> +0:      sti

Its code like this which makes me even more certain that we have far too
much code written in assembly which doesn't need to be.  Maybe not this
specific sample, but it has taken me 15 minutes and a pad of paper to
try and work out how this conditional works, and I am still not certain
its correct.  In particular, PFEC_prot_key looks like it fool the test
into believing a non-smap/smep fault was a smap/smep fault.

Can you at least provide some C in a comment with the intended
conditional, to aid clarity?

~Andrew

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