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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.01.01 at 01:10, <feng.wu@xxxxxxxxx> wrote:
> --- 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:

Labels should be indented by at least one space. But even better
would be to handle this with "else" instead of "goto".

> +    v->arch.smap_check_policy = SMAP_CHECK_HONOR_CPL_AC;

Please save the old value and restore it here rather than blindly
enforcing "honor" mode.

> +        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);
> +            break;
> +        case SMAP_CHECK_DISABLED:
> +            break;
> +        default:
> +            printk(XENLOG_INFO "Invalid SMAP check type!\n");

Isn't this more a BUG() or ASSERT(0), or perhaps - with the
SMAP_CHECK_DISABLED case dropped, BUG_ON(... !=
SMAP_CHECK_DISABLED) or ASSERT(... ==
SMAP_CHECK_DISABLED)?

> +    /*
> +     * 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

I'd prefer this to be an enum.

Jan


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