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 10:10 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 15:26, <feng.wu@xxxxxxxxx> wrote:
> >> 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();
> 
> Yes.
> 
> >> > --- 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)
> 
> Right, but the question you need to ask: Does it hurt anything? I
> don't think so, and it covers all relevant cases in one go. It's a
> code path that's being taken when something is wrong with the
> guest anyway, so doing an clac in that case doesn't hurt a all.

Yes, it doesn't hurt anything.

> 
> Jan

Thanks,
Feng

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