Re: [Xen-devel] [PATCH v3 5/9] 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: Monday, April 28, 2014 5:35 PM
> To: Wu, Feng
> Cc: andrew.cooper3@xxxxxxxxxx; ian.campbell@xxxxxxxxxx; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v3 5/9] x86: Temporary disable SMAP to legally access user
> pages in kernel mode
> 
> >>> On 28.04.14 at 05:16, <feng.wu@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -3754,6 +3754,8 @@ unsigned long do_get_debugreg(int reg)
> >
> >  void asm_domain_crash_synchronous(unsigned long addr)
> >  {
> > +    clac();
> 
> I know it was us asking you to put this here, but that doesn't mean
> you'll get away without adding a brief comment saying why it is here
> (thus avoiding some janitor to come and remove this again).
> 
> > --- 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
> 
> I think this should be deferred as much as possible; I even think it is
> warranted to put this at two places here (in the two conditional
> execution branches) just to avoid doing this too early.

I think about this again. Seems ASM_STAC/ASM_CLAC is not needed for
compat_create_bounce_frame, since in this chunk of code, it only accesses
the pv guest's kernel stack, which should be in ring 1 for 32-bit pv. Is my
understanding correct? Thanks a lot!

> 
> > @@ -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
> 
> And I think this one should be moved up as much as possible.
> 
> > @@ -439,6 +440,7 @@ UNLIKELY_START(z,
> create_bounce_frame_bad_bounce_ip)
> >          jmp   asm_domain_crash_synchronous  /* Does not return */
> >  __UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
> >          movq  %rax,UREGS_rip+8(%rsp)
> > +        ASM_CLAC
> >          ret
> 
> Same here.
> 
> Jan

Thanks,
Feng

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