Re: [PATCH] x86/HVM: support emulated UMIP

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.02.2021 15:10, Andrew Cooper wrote:
> On 29/01/2021 11:45, Jan Beulich wrote:
>> There are three noteworthy drawbacks:
>> 1) The intercepts we need to enable here are CPL-independent, i.e. we
>>    now have to emulate certain instructions for ring 0.
>> 2) On VMX there's no intercept for SMSW, so the emulation isn't really
>>    complete there.
>> 3) The CR4 write intercept on SVM is lower priority than all exception
>>    checks, so we need to intercept #GP.
>> Therefore this emulation doesn't get offered to guests by default.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> I wonder if it would be helpful for this to be 3 patches, simply because
> of the differing complexity of the VT-x and SVM pieces.

If so, then three or even four. One each for SVM/VMX, and
a final one for the enabling in vendor independent code.
For the possible 4th one, see below in the
hvm_descriptor_access_intercept() related part.

>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -453,6 +453,13 @@ static void __init calculate_hvm_max_pol
>>      __set_bit(X86_FEATURE_X2APIC, hvm_featureset);
>>  
>>      /*
>> +     * Xen can often provide UMIP emulation to HVM guests even if the host
>> +     * doesn't have such functionality.
>> +     */
>> +    if ( hvm_funcs.set_descriptor_access_exiting )
> 
> No need for this check.  Exiting is available on all generations and
> vendors.

VMX code treats this as optional, and I think it validly
does so at least in case we're running virtualized ourselves
and the lower hypervisor doesn't emulate this.

> Also, the header file probably wants a ! annotation for UMIP to signify
> that we doing something special with it.

I can do so, sure. I'm never really sure how wide the scope
of "special" is here.

>> @@ -3731,6 +3732,13 @@ int hvm_descriptor_access_intercept(uint
>>      struct vcpu *curr = current;
>>      struct domain *currd = curr->domain;
>>  
>> +    if ( (is_write || curr->arch.hvm.guest_cr[4] & X86_CR4_UMIP) &&
> 
> Brackets for & expression?

Oops.

>> +         hvm_get_cpl(curr) )
>> +    {
>> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> +        return X86EMUL_OKAY;
>> +    }
> 
> I believe this is a logical change for monitor - previously, non-ring0
> events would go all the way to userspace.
> 
> I don't expect this to be an issue - monitoring agents really shouldn't
> be interested in userspace actions the guest kernel is trying to turn
> into #GP.

Isn't the present behavior flawed, in that UMIP (if supported
by hardware and enabled by the guest) doesn't get honored,
and the access _instead_ gets forwarded to the monitor?
Looking at public/vm_event.h I can't seem to be able to spot
any means by which the monitor could say "I want an exception
here" in response. IOW - shouldn't this hunk be split out as
a prereq bug fix (i.e. aforementioned 4th patch)?

>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -547,6 +547,28 @@ void svm_update_guest_cr(struct vcpu *v,
>>              value &= ~(X86_CR4_SMEP | X86_CR4_SMAP);
>>          }
>>  
>> +        if ( v->domain->arch.cpuid->feat.umip && !cpu_has_umip )
> 
> Throughout the series, examples like this should have the !cpu_has_umip
> clause first.  It is static per host, rather than variable per VM, and
> will improve the branch prediction.
> 
> Where the logic is equivalent, it is best to have the clauses in
> stability order, as this will prevent a modern CPU from even evaluating
> the CPUID policy.
> 
>> +        {
>> +            u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
>> +
>> +            if ( v->arch.hvm.guest_cr[4] & X86_CR4_UMIP )
>> +            {
>> +                value &= ~X86_CR4_UMIP;
>> +                ASSERT(vmcb_get_cr_intercepts(vmcb) & 
>> CR_INTERCEPT_CR0_READ);
> 
> It occurs to me that adding CR0 read exiting adds a lot of complexity
> for very little gain.
> 
> From a practical standpoint, UMIP exists to block SIDT/SGDT which are
> the two instructions which confer an attacker with useful information
> (linear addresses of the IDT/GDT respectively).  SLDT/STR only confer a
> 16 bit index within the GDT (fixed per OS), and SMSW is as good as a
> constant these days.
> 
> Given that Intel cannot intercept SMSW at all and we've already accepted
> that as a limitation vs architectural UMIP, I don't think extra
> complexity on AMD is worth the gain.

Hmm, I didn't want to make this emulation any less complete
than is necessary because of external restrictions. As an
intermediate solution, how about hiding this behind a
default-off command line option, e.g. "svm=full-umip"?

>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -1537,6 +1552,7 @@ static void vmx_update_guest_cr(struct v
>>                                               (X86_CR4_PSE | X86_CR4_SMEP |
>>                                                X86_CR4_SMAP)
>>                                               : 0;
>> +            v->arch.hvm.vmx.cr4_host_mask |= cpu_has_umip ? 0 : 
>> X86_CR4_UMIP;
> 
> if ( !cpu_has_umip )
>      v->arch.hvm.vmx.cr4_host_mask |= X86_CR4_UMIP;

This wouldn't be in line with immediately preceding code
(visible in context), and subsequent code using if() is, aiui,
because the conditions are (textually) more complex there. So
if I was to make the change, I'd at least like to understand
why adjacent code is fine doing differently, even more so that
iirc it was often you to introduce such constructs in favor of
if()-s.

Jan