Re: [Xen-devel] [PATCH v2 6/7] x86/hvm: Add SMAP support to HVM guest

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, April 23, 2014 6:49 PM
> To: Wu, Feng
> Cc: andrew.cooper3@xxxxxxxxxx; ian.campbell@xxxxxxxxxx; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v2 6/7] x86/hvm: Add SMAP support to HVM guest
> 
> >>> On 23.04.14 at 16:37, <feng.wu@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -2536,12 +2536,15 @@ void vmx_vmexit_handler(struct cpu_user_regs
> *regs)
> >      unsigned long exit_qualification, exit_reason, idtv_info, intr_info =
> > 0;
> >      unsigned int vector = 0;
> >      struct vcpu *v = current;
> > +    unsigned long sel;
> >
> >      __vmread(GUEST_RIP,    &regs->rip);
> >      __vmread(GUEST_RSP,    &regs->rsp);
> >      __vmread(GUEST_RFLAGS, &regs->rflags);
> > +    __vmread(GUEST_CS_SELECTOR, &sel);
> >
> >      hvm_invalidate_regs_fields(regs);
> > +    regs->cs = sel;
> 
> No, not for the purpose that it is being used for below:
> 
> > @@ -159,13 +159,30 @@ guest_walk_tables(struct vcpu *v, struct
> p2m_domain *p2m,
> >      mflags = mandatory_flags(v, pfec);
> >      iflags = (_PAGE_NX_BIT | _PAGE_INVALID_BITS);
> >
> > -    /* SMEP: kernel-mode instruction fetches from user-mode mappings
> > -     * should fault.  Unlike NX or invalid bits, we're looking for _all_
> > -     * entries in the walk to have _PAGE_USER set, so we need to do the
> > -     * whole walk as if it were a user-mode one and then invert the answer.
> */
> > -    smep = (is_hvm_vcpu(v) && hvm_smep_enabled(v)
> > -            && (pfec & PFEC_insn_fetch) && !(pfec &
> PFEC_user_mode) );
> > -    if ( smep )
> > +    if ( is_hvm_vcpu(v) && !(pfec & PFEC_user_mode) )
> > +    {
> > +        struct cpu_user_regs *regs = guest_cpu_user_regs();
> > +
> > +        /* SMEP: kernel-mode instruction fetches from user-mode
> mappings
> > +         * should fault.  Unlike NX or invalid bits, we're looking for 
> > _all_
> > +         * entries in the walk to have _PAGE_USER set, so we need to do
> the
> > +         * whole walk as if it were a user-mode one and then invert the
> answer. */
> > +        smep =  hvm_smep_enabled(v) && (pfec & PFEC_insn_fetch);
> > +
> > +        /*
> > +         * SMAP: kernel-mode data accesses from user-mode mappings
> should fault
> > +         * A fault is considered as a SMAP violation if the following
> > +         * conditions come true:
> > +         *   - X86_CR4_SMAP is set in CR4
> > +         *   - A user page is accessed
> > +         *   - CPL = 3 or X86_EFLAGS_AC is clear
> > +         *   - Page fault in kernel mode
> > +         */
> > +        smap = hvm_smap_enabled(v) &&
> > +               !(!ring_3(regs) && (regs->eflags & X86_EFLAGS_AC));
> 
> ring_3() isn't supposed to be used for HVM guests. Please use proper
> HVM methods here.

To get the guest CS, I should use the following code path, right?
hvm_get_segment_register() --> vmx_get_segment_register()

I don't find any HVM methods, so can I just use ((regs->cs & 3) < 3) here?

> 
> > @@ -360,6 +362,15 @@ static inline bool_t hvm_vcpu_has_smep(void)
> >      return !!(ebx & cpufeat_mask(X86_FEATURE_SMEP));
> >  }
> >
> > +static inline bool_t hvm_vcpu_has_smap(void)
> > +{
> > +    unsigned int ebx = 0, leaf = 0x7;
> > +
> > +    hvm_cpuid(leaf, NULL, &ebx, NULL, NULL);
> > +
> > +    return !!(ebx & cpufeat_mask(X86_FEATURE_SMAP));
> > +}
> 
> Same comments as for the similar SMEP code in an earlier patch.
> 
> Jan

Thanks,
Feng

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