Re: [Xen-devel] [PATCH V2] xen: support enabling SMEP/SMAP for HVM only

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.08.16 at 12:03, <he.chen@xxxxxxxxxxxxxxx> wrote:
> On Thu, Aug 11, 2016 at 07:14:06AM -0600, Jan Beulich wrote:
>> >>> On 11.08.16 at 11:17, <he.chen@xxxxxxxxxxxxxxx> wrote:
>> > @@ -1404,12 +1438,20 @@ void __init noreturn __start_xen(unsigned long 
> mbi_p)
>> >      if ( !opt_smep )
>> >          setup_clear_cpu_cap(X86_FEATURE_SMEP);
>> >      if ( cpu_has_smep )
>> > +    {
>> >          set_in_cr4(X86_CR4_SMEP);
>> > +        if ( smep_hvm_only )
>> > +            write_cr4(read_cr4() & ~X86_CR4_SMEP);
>> > +    }
>> 
>> So that'll clear CR4.SMEP right here, but won't help with CR4 loads
>> from mmu_cr4_features (as e.g. happens indirectly during VM exits,
>> since the HOST_CR4 field gets set from this variable).
>> 
>> Did you in fact test your change, including validation of the features
>> correctly remaining off over the lifetime of the system?
>> 
>> Further, considering that you don't clear the two flags from Xen's
>> internal feature bitmap, and taken together with the internal feature
>> bitmap driving alternative instruction patching, I'd assume pointless
>> (and performance wise perhaps harmful) patching to now take place.
>> 
> Let me see whether I understand this correctly...
> 
> Regarding alternative instruction patching, if enabling SMAP for HVM but
> disabling it for Xen, SMAP bit must be set in x86_capability feature
> bitmap and cleared in mmu_cr4_features, which means instruction patching
> would take place and a #UD may occur (since SMAP is disable in Xen, but
> STAC/CLAC are patched and called).

No #UD would occur (hence I only said "performance wise perhaps
harmful"), as per the SDM.

> A little dirty solution I can think of now is to temperarily clear SMAP
> bit in x86_capability before patching instruction and then set it back
> when instruction patching finish, like:
> 
> ```
> if ( opt_smap == SMAP_HVM_ONLY )
>     setup_clear_cpu_cap(X86_FEATURE_SMAP);
> 
> alternative_instructions();
> 
> if ( opt_smap == SMAP_HVM_ONLY )
>     __set_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);
> ```

Except that this would need further adjustment (e.g. using just
__clear_bit() instead of setup_clear_cpu_cap(), or else you'd
break CPU hotplug).

I'm not against "a little dirty" solutions, as long as it doesn't end
in plain hackery, and as long as the end result indeed meets all
requirements.

> Appreciate it if you have a better solution.

Well, if I had a reasonably good solution, I could have gone and
simply implemented it. It is the need to consider all resulting cases
properly which makes this involved, and hence we've turned to
you (Intel) for assistance.

Jan


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