Re: [PATCH RFC] x86/kvm: expose the CPUID of SPEC_CTRL and STIBP to guests

[Wei Wang <wei.w.wang@intel.com>]

On 01/09/2018 04:43 PM, Liran Alon wrote:
> ----- wei.w.wang@intel.com wrote:
>
>> This patch shows an alternative approach to the one posted here:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mail-2Darchive.com_linux-2Dkernel-40vger.kernel.org_msg1580364.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=o_uh0-IfLdLMwf2MaOOQVKtZbPlJ-fDVmglRj277cwk&s=j1oGOoYysZhUuGuuvuwdTU_OO7bv1ysIyvDtAlc6C-c&e=
>>
>> The advantages are
>> 1) Simpler;
>> 2) More reasonable because this is used to fill the hardware security
>> hole, for all the x86 cpus that physically support the two CPUIDs,
>> which means the hole already exists physically. All the VMs should
>> use this feature no matter what CPU model they are using. So,
> I'm not sure I 100% agree with this.
> There should be a way for the userspace agent to disable these CPUIDs if wanted.
> You don't want to lose the ability to expose a mimic of a real physical CPU-model of core2duo that
> doesn't have these CPUIDs. A good solution can be that these features will be exposed by default to guests
> if available on hardware but can still be explicitly not-exposed if userspace agent wishes so.

I think the case we are handling here is different:
It shouldn't be treated as a regular feature (e.g. xsaves) that a user 
can choose to use or not. It is a security hole (or say a bug). When we 
fixed a bug, we don't give users an option to select to trigger the bug, 
right?


> The only weird side-effect of this is that live-migration between different physical hosts running with
> exact same QEMU cmdline will result in different CPUID values exposed to guest.

I think live migration itself doesn't do the CPUID check, so adding the 
QEMU side hardcode part doesn't help. That is, even using that patch 
7/7, I think it wouldn't make a difference.
This is the same case when we migrate a VM from a skylake physical 
machine to an older machine, the difference of supported CPUID won't 
even generate a warning when the destination side QEMU gets booted, right?




>> exposing
>> the two CPUIDs as long as they are physically supported by the
>> hardware,
>> and this doesn't require the QEMU side hardcode as usual.
>>
>> When the related feature bits are added to the kernel, and we can
>> simply
>> change it to:
>> 	best->edx |= F(SPEC_CTRL) | F(PRED_CMD);
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 0099e10..c33d3d4 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -70,6 +70,8 @@ u64 kvm_supported_xcr0(void)
>>   /* These are scattered features in cpufeatures.h. */
>>   #define KVM_CPUID_BIT_AVX512_4VNNIW     2
>>   #define KVM_CPUID_BIT_AVX512_4FMAPS     3
>> +#define KVM_CPUID_BIT_SPEC_CTRL         26
>> +#define KVM_CPUID_BIT_STIBP             27
>>   #define KF(x) bit(KVM_CPUID_BIT_##x)
>>   
>>   int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>> @@ -109,6 +111,9 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>   		}
>>   	}
>>   
>> +	if (cpuid_edx(0x7) & (KF(SPEC_CTRL) | KF(STIBP)))
> You should put this inside the "if (best) {...}" block above.

Right, thanks.

Best,
Wei