[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > Sent: Tuesday, April 15, 2014 4:47 PM > To: Wu, Feng > Cc: Ian.Campbell@xxxxxxxxxx; Dong, Eddie; Nakajima, Jun; > xen-devel@xxxxxxxxxxxxx > Subject: Re: [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user > pages in kernel mode > > >>> On 15.04.14 at 15:01, <feng.wu@xxxxxxxxx> wrote: > > --- a/xen/arch/x86/clear_page.S > > +++ b/xen/arch/x86/clear_page.S > > @@ -1,9 +1,11 @@ > > #include <xen/config.h> > > #include <asm/page.h> > > +#include <asm/asm_defns.h> > > > > #define ptr_reg %rdi > > > > ENTRY(clear_page_sse2) > > + ASM_STAC > > mov $PAGE_SIZE/16, %ecx > > xor %eax,%eax > > > > @@ -15,5 +17,6 @@ ENTRY(clear_page_sse2) > > lea 16(ptr_reg), ptr_reg > > jnz 0b > > > > + ASM_CLAC > > sfence > > ret > > Wrong code being modified - this isn't used to clear guest memory (or > else we have a security problem). If there are pages having the U bit > set, then I guess you need to first go and clean those up. I got an SMAP violation in function clear_page_sse2() while debugging, which makes me think that this function accesses user pages and the AC bit needs to be set. However, today I find that the real fault happens in construct_dom0() like the following code path. After adopt Kevin's suggestion, we will make construct_dom0() wrapped in a stac()/clac() part as a whole. So the changes here should be removed. Thanks for your comments! __start_xen() --> construct_dom0() --> clear_page() --> clear_page_sse2() > > > --- a/xen/arch/x86/x86_64/compat/entry.S > > +++ b/xen/arch/x86/x86_64/compat/entry.S > > @@ -265,6 +265,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 > > @@ -336,6 +337,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 > > These and the similar changes to xen/arch/x86/x86_64/entry.S seem > pretty pointless without also adding ASM_CLAC to the exception and > interrupt entry points. > Yes, you are right, we should add ASM_CLAC in those places! Thanks a lot! > Jan Thanks, Feng _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |