Re: [Xen-devel] [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.04.14 at 16:35, <feng.wu@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -778,6 +778,7 @@ int __init construct_dom0(
>      }
>      bootstrap_map(NULL);
>  
> +    stac();
>      if ( UNSET_ADDR != parms.virt_hypercall )
>      {
>          if ( (parms.virt_hypercall < v_start) ||
> @@ -787,6 +788,7 @@ int __init construct_dom0(
>              write_ptbase(current);
>              printk("Invalid HYPERCALL_PAGE field in ELF notes.\n");
>              rc = -1;
> +            clac();
>              goto out;

If done this way, this really should be moved to the out: label. But
I think the risk of missing a return path is quite high this way - I'd
much rather want the caller to wrap its call in a stac()/clac() pair.

> @@ -52,6 +54,7 @@ __copy_from_user_ll(void *to, const void __user *from, 
> unsigned n)
>      unsigned long __d0, __d1, __d2, __n = n;
>  
>      asm volatile (
> +             ASM_STAC(%%)"\n"
>          "    cmp  $"STR(2*BYTES_PER_LONG-1)",%0\n"
>          "    jbe  1f\n"
>          "    mov  %1,%0\n"

Mismatched indentation (also further down).

> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -266,6 +266,7 @@ ENTRY(compat_int80_direct_trap)
>  /* On return only %rbx and %rdx are guaranteed non-clobbered.            */
>  compat_create_bounce_frame:
>          ASSERT_INTERRUPTS_ENABLED
> +        ASM_STAC
>          mov   %fs,%edi
>          testb $2,UREGS_cs+8(%rsp)
>          jz    1f
> @@ -337,6 +338,7 @@ __UNLIKELY_END(compat_bounce_null_selector)
>          movl  %eax,UREGS_cs+8(%rsp)
>          movl  TRAPBOUNCE_eip(%rdx),%eax
>          movl  %eax,UREGS_rip+8(%rsp)
> +        ASM_CLAC
>          ret
>  .section .fixup,"ax"
>  .Lfx13:

This ignores the path(s) leading to asm_domain_crash_synchronous.

> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index d294064..e49f9c4 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -380,6 +380,7 @@ __UNLIKELY_END(create_bounce_frame_bad_sp)
>          movb  TRAPBOUNCE_flags(%rdx),%cl
>          subq  $40,%rsi
>          movq  UREGS_ss+8(%rsp),%rax
> +        ASM_STAC
>  .Lft2:  movq  %rax,32(%rsi)             # SS
>          movq  UREGS_rsp+8(%rsp),%rax
>  .Lft3:  movq  %rax,24(%rsi)             # RSP
> @@ -437,9 +438,11 @@ UNLIKELY_END(bounce_failsafe)
>          testq %rax,%rax
>  UNLIKELY_START(z, create_bounce_frame_bad_bounce_ip)
>          lea   
> UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_bounce_ip)(%rip), %rdi
> +        ASM_CLAC
>          jmp   asm_domain_crash_synchronous  /* Does not return */

Interestingly here you spotted the need - you may want to consider
doing this at the asm_domain_crash_synchronous label instead.

Jan


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