[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 1/2] x86/hvm: Always do SMAP check when updating runstate_guest(v)
> -----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. > > > + 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? Seems I cannot find a padding place in struct arch_vcpu, neither in struct hvm_vcpu. Thanks, Feng > > Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |