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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@eikelenboom.it>
> Signed-off-by: Feng Wu <feng.wu@intel.com>

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.

> ---
>  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

> +            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 *);
>