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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, April 23, 2014 6:39 PM
> To: Wu, Feng
> Cc: andrew.cooper3@xxxxxxxxxx; ian.campbell@xxxxxxxxxx; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user
> pages in kernel mode
> 
> >>> 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.

So, do you think code like the following looks better?

stac();
construct_dom0();
clac();

> 
> > @@ -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), %rd
> i
> > +        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.

Yes, that should be a clean solution. But seems we don't need to call 
"ASM_CLAC" in
all the places where asm_domain_crash_synchronous() is called. For example:

In file xen/arch/x86/x86_64/entry.S:

create_bounce_frame:
        ......
        ......

UNLIKELY_START(g, create_bounce_frame_bad_sp)
        lea   UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_sp)(%rip), %rdi
        jmp   asm_domain_crash_synchronous  /* Does not return */
__UNLIKELY_END(create_bounce_frame_bad_sp)

        ......


> 
> Jan


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