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

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Tuesday, July 29, 2014 3:19 PM
> To: Wu, Feng
> Cc: linux@xxxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx;
> tim@xxxxxxx
> Subject: Re: [PATCH v3 1/2] x86/hvm: Always do SMAP check when updating
> runstate_guest(v)
> 
> >>> 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.

Sorry, It was my negligence. This 'else' is pretty pointless:)

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

Sorry, I don't find any indentation issue here. Do you mean 
"((seg.attr.fields.dpl == 3) ||" and "!(regs->eflags & X86_EFLAGS_AC));"
are not indented with "hvm_smap_enabled(v) &&"? In fact they are in good 
indentation. Maybe it is the display that make it look
like in wrong 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 ...

Good idea!

Thanks,
Feng

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