Re: [Xen-devel] [PATCH 1/2] x86/hvm: Always do SMAP check when updating runstate_guest(v)

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

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Tuesday, July 08, 2014 6:05 PM
> To: Wu, Feng; xen-devel@xxxxxxxxxxxxx
> Cc: tim@xxxxxxx; linux@xxxxxxxxxxxxxx; jbeulich@xxxxxxxx; keir@xxxxxxx
> Subject: Re: [Xen-devel] [PATCH 1/2] x86/hvm: Always do SMAP check when
> updating runstate_guest(v)
> 
> On 08/07/14 00:18, Feng Wu wrote:
> > In the current implementation, we honor the guest's CPL and AC
> > to determain whether do the SMAP check or not for runstate_guest(v).
> > However, this doesn't work. The VMCS feild is invalid when we try
> > to get geust's SS by hvm_get_segment_register(), since the
> > right VMCS has not beed loaded for the current VCPU.
> >
> > In this patch, we always do the SMAP check when updating
> > runstate_guest(v) for the guest when SMAP is enabled by it.
> >
> > Reported-by: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>
> > Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> 
> Why can't the VMCS be reloaded in vmx_ctxt_switch_to() rather than
> vmx_do_resume() ?  The problem is not with updating the runstate area
> persay, but due to using guest_walk_tables() during a context switch
> before the VMCS has been loaded.

This is another option in the discussion with Jan. However, seems
the current option is preferred by Jan.

> 
> > ---
> >  xen/arch/x86/domain.c        | 15 ++++++++++++---
> >  xen/arch/x86/mm/guest_walk.c | 41
> ++++++++++++++++++++++++++++-------------
> >  xen/include/asm-x86/domain.h | 15 ++++++++++++++-
> >  3 files changed, 54 insertions(+), 17 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index e896210..b0c8810 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1349,22 +1349,31 @@ static void paravirt_ctxt_switch_to(struct vcpu
> *v)
> >  }
> >
> >  /* Update per-VCPU guest runstate shared memory area (if registered). */
> > -bool_t update_runstate_area(const struct vcpu *v)
> > +bool_t update_runstate_area(struct vcpu *v)
> >  {
> > +    bool_t rc;
> > +
> >      if ( guest_handle_is_null(runstate_guest(v)) )
> >          return 1;
> >
> > +    v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
> > +
> >      if ( has_32bit_shinfo(v->domain) )
> >      {
> >          struct compat_vcpu_runstate_info info;
> >
> >          XLAT_vcpu_runstate_info(&info, &v->runstate);
> >          __copy_to_guest(v->runstate_guest.compat, &info, 1);
> > -        return 1;
> > +        rc = 1;
> > +        goto out;
> >      }
> >
> > -    return __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
> > +    rc = __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
> >             sizeof(v->runstate);
> > +
> > +out:
> > +    v->arch.smap_check_policy = SMAP_CHECK_HONOR_CPL_AC;
> > +    return rc;
> >  }
> >
> >  static void _update_runstate_area(struct vcpu *v)
> > diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
> > index bb38fda..1afa7fd 100644
> > --- a/xen/arch/x86/mm/guest_walk.c
> > +++ b/xen/arch/x86/mm/guest_walk.c
> > @@ -164,25 +164,40 @@ guest_walk_tables(struct vcpu *v, struct
> p2m_domain *p2m,
> >          struct segment_register seg;
> >          const struct cpu_user_regs *regs = guest_cpu_user_regs();
> >
> > -        hvm_get_segment_register(v, x86_seg_ss, &seg);
> > -
> >          /* 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) &&
> > -               ((seg.attr.fields.dpl == 3) || !(regs->eflags &
> X86_EFLAGS_AC));
> > +        switch ( v->arch.smap_check_policy )
> > +        {
> > +        case SMAP_CHECK_HONOR_CPL_AC:
> > +            hvm_get_segment_register(v, x86_seg_ss, &seg);
> > +
> > +            /*
> > +             * 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) &&
> > +                   ((seg.attr.fields.dpl == 3) ||
> > +                   !(regs->eflags & X86_EFLAGS_AC));
> > +            break;
> > +        case SMAP_CHECK_ENABLED:
> > +            smap = hvm_smap_enabled(v);
> 
> Can you describe what behavioural change this will cause.  From my
> understanding, for a guest which has enabled CR4.SMAP, it will
> unconditionally cause an -EFAULT for runstate areas in user pagetables?
> 
> ~Andrew

Here is the call path according to the Xen call trace:

copy_to_user_hvm() --> hvm_copy_to_guest_virt_nofault() --> __hvm_copy()
--> paging_gva_to_gfn() --> hap_gva_to_gfn_3_levels() --> 
hap_p2m_ga_to_gfn_3_levels()
--> guest_walk_tables_3_levels()

In the case you mentioned, guest_walk_tables_3_levels() will return an non-zero 
value, so
hap_p2m_ga_to_gfn_3_levels will return INVALID_GFN, and this return value will 
be eventually
returned by paging_gva_to_gfn(). Hence HVMCOPY_bad_gva_to_gfn will be returned 
by
hvm_copy_to_guest_virt_nofault(). At last in copy_to_user_hvm() the following 
code is executed:

    rc = hvm_copy_to_guest_virt_nofault((unsigned long)to, (void *)from,
                                        len, 0);
    return rc ? len : 0; /* fake a copy_to_user() return code */

So, 'len' will be returned. Maybe this is not the expected behavior,
However, do you think there are some problems with this return logic in 
copy_to_user_hvm()
irrespective of SMAP cases? If hvm_copy_to_guest_virt_nofault() fails, it 
always return a
non-zero value, and in this case, copy_to_user_hvm() will continue to return 
'len' instead of an error.

Thanks,
Feng

> 
> > +            break;
> > +        case SMAP_CHECK_DISABLED:
> > +            break;
> > +        default:
> > +            printk(XENLOG_INFO "Invalid SMAP check type!\n");
> > +            break;
> > +        }
> >      }
> >
> >      if ( smep || smap )
> > diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> > index abf55fb..d7cac4f 100644
> > --- a/xen/include/asm-x86/domain.h
> > +++ b/xen/include/asm-x86/domain.h
> > @@ -446,13 +446,26 @@ struct arch_vcpu
> >
> >      /* A secondary copy of the vcpu time info. */
> >      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> > +
> > +    /*
> > +     * The SMAP check policy when updating runstate_guest(v) and the
> > +     * secondary system time.
> > +     *     SMAP_CHECK_HONOR_CPL_AC - honor the guest's CPL and
> AC
> > +     *     SMAP_CHECK_ENABLED      - enable the check
> > +     *     SMAP_CHECK_DISABLED     - disable the check
> > +     */
> > +    uint8_t smap_check_policy;
> >  } __cacheline_aligned;
> >
> > +#define SMAP_CHECK_HONOR_CPL_AC  0
> > +#define SMAP_CHECK_ENABLED       1
> > +#define SMAP_CHECK_DISABLED      2
> > +
> >  /* Shorthands to improve code legibility. */
> >  #define hvm_vmx         hvm_vcpu.u.vmx
> >  #define hvm_svm         hvm_vcpu.u.svm
> >
> > -bool_t update_runstate_area(const struct vcpu *);
> > +bool_t update_runstate_area(struct vcpu *);
> >  bool_t update_secondary_system_time(const struct vcpu *,
> >                                      struct vcpu_time_info *);
> >


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