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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.01.01 at 23:52, <feng.wu@xxxxxxxxx> wrote:
> -bool_t update_runstate_area(const struct vcpu *v)
> +bool_t update_runstate_area(struct vcpu *v)
>  {
> +    bool_t rc;
> +    smap_check_policy_t saved_policy;
> +
>      if ( guest_handle_is_null(runstate_guest(v)) )
>          return 1;
>  
> +    saved_policy = smap_policy_change(v, 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);
> +        smap_policy_change(v, saved_policy);
>          return 1;
>      }
> -
> -    return __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
> -           sizeof(v->runstate);
> +    else

Pointless else. Remember, in the previous version you used "goto",
which I asked you to avoid by using "else". Since you opted for
retaining the "return" above in this version, the need for the "else"
went away. The alternative would now be to convert the "return"
above to "rc = 1" and move the return below out of the "else" scope.

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

Indentation.

> +typedef enum {
> +    SMAP_CHECK_HONOR_CPL_AC = 0,  /* honor the guest's CPL and AC */

Pointless " = 0".

> +    SMAP_CHECK_ENABLED,           /* enable the check */
> +    SMAP_CHECK_DISABLED,          /* disable the check */
> +} smap_check_policy_t;

Perhaps worth adding the __packed__ attribute here to get this
back down to a byte ...

> @@ -446,13 +452,22 @@ 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_policy_t smap_check_policy;
>  } __cacheline_aligned;

... and try to find a better place (current padding) in this structure?

Jan


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