[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode
> -----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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |