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: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Wednesday, April 23, 2014 6:35 PM
> To: Wu, Feng
> Cc: xen-devel@xxxxxxxxxxxxx; JBeulich@xxxxxxxx; Tian, Kevin; Dong, Eddie;
> Nakajima, Jun; ian.campbell@xxxxxxxxxx
> Subject: Re: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user
> pages in kernel mode
> 
> On 23/04/14 15:35, Feng Wu wrote:
> > Use STAC/CLAC to temporarily disable SMAP to allow legal accesses to
> > user pages in kernel mode
> >
> > Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> > ---
> >  xen/arch/x86/domain_build.c         | 3 +++
> >  xen/arch/x86/usercopy.c             | 6 ++++++
> >  xen/arch/x86/x86_64/compat/entry.S  | 2 ++
> >  xen/arch/x86/x86_64/entry.S         | 4 ++++
> >  xen/include/asm-x86/uaccess.h       | 4 ++++
> >  xen/include/asm-x86/x86_64/system.h | 2 ++
> >  6 files changed, 21 insertions(+)
> >
> > diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> > index 84ce392..15af110 100644
> > --- 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();
> 
> As constructing dom0 is trusted, this should be near the top of top of
> the function

We cannot call stac() near the top of the function, because construct_dom0() 
calls
elf_load_binary() which calls copy_from_user(), we can only add stac() after 
calling
elf_load_binary(), otherwise the AC bit will remain cleared after 
elf_load_binary().

I just sugguest another method in another mail, can you please have a look?

> 
> >      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();
> 
> and this should be after the out label.
> 
> >              goto out;
> >          }
> >          hypercall_page_initialise(
> > @@ -1150,6 +1152,7 @@ int __init construct_dom0(
> >                 elf_check_broken(&elf));
> >
> >      iommu_dom0_init(dom0);
> > +    clac();
> 
> This will need rebasing given recent changes in staging.
> 
> ~Andrew

Thanks,
Feng

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