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 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.

Jan


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