Re: [Xen-devel] [PATCH v1 2/6] 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: 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