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

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

[Liran Alon]

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

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

> +		best->edx |= KF(SPEC_CTRL) | KF(STIBP);
> +
>  	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
>  	if (!best) {
>  		vcpu->arch.guest_supported_xcr0 = 0;
> -- 
> 2.7.4

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

[Paolo Bonzini]

On 09/01/2018 09:43, Liran Alon wrote:
> 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.
> 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.

In turn, this will cause guests to fail horribly when migrated from
patched to unpatched hosts (#GP on the first access to
MSR_IA32_SPEC_CTRL).  Shortcuts simply don't work when you take
migration into account.

Paolo

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

[Wei Wang]

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

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

[Paolo Bonzini]

On 09/01/2018 10:16, Wei Wang wrote:
> 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 user is free to do whatever it wants.  For example, you could use
KVM with an old CPU model to test the performance effect of IBRS on a
guest, or to reverse engineer whether the guest is using IBRS or
retpolines or whatever, and so on.

Policy isn't implemented in KVM, QEMU or even libvirt.

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

But we support it by using "-cpu Haswell" when we start the machine on a
Skylake.

Patch 7 is ugly right now, but it will be just a usual "add CPUID bits"
patch as soon as Linux gets the cpufeature.  A security-related feature
is a good reason to fast track adding the CPUID bits, it's not a good
reason to bypass all the lessons that we've learnt through the years
about guest ABI and live migration.

Paolo

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

[Jim Mattson]

On Tue, Jan 9, 2018 at 1:16 AM, Wei Wang <wei.w.wang@intel.com> wrote:
> 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?

Due to the performance impact on the neighboring hyperthread, one may
not want to expose SPEC_CTRL to all VMs.

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

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

[Wei Wang]

On 01/09/2018 04:39 PM, Paolo Bonzini wrote:
> On 09/01/2018 07:25, Wei Wang wrote:
>> This patch shows an alternative approach to the one posted here:
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1580364.html
>>
>> 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, 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);
> Is this meant to replace the whole series or just patch 1/7?  The
> functions in patch 1/7 are used later by vmx.c and svm.c.
>

The pointer should have pointed to 7/7 (not 1/7). Just patch 7/7 actually.

Best,
Wei

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

[Paolo Bonzini]

On 09/01/2018 09:52, Liran Alon wrote:
> 
> ----- pbonzini@redhat.com wrote:
> 
>> On 09/01/2018 09:43, Liran Alon wrote:
>>> 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.
>>> 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.
>>
>> In turn, this will cause guests to fail horribly when migrated from
>> patched to unpatched hosts (#GP on the first access to
>> MSR_IA32_SPEC_CTRL).  Shortcuts simply don't work when you take
>> migration into account.
> 
> That's true but this will also happen when explicitly exposing new CPUIDs via QEMU cmdline.
> The live-migration controller should not attempt to migrate a VM that runs with these CPUIDs to a host
> which cannot support them.

Indeed libvirt does such a check, and QEMU does it too if you use "-cpu
...,enforce".  But if KVM or even QEMU[1] adds flags behind the user's
back, the controller cannot even _know_ that the VM cannot be migrated
to an unpatched host.

Paolo

[1] as in this patch:
https://lists.nongnu.org/archive/html/qemu-devel/2018-01/txtfadLGhMEF6.txt

> But I agree with you, as at least when it is explicit, this scenario can also be checked in QEMU.
> (QEMU can check if explicit CPUID flags to expose doesn't match host capabilities).

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

[Liran Alon]

----- pbonzini@redhat.com wrote:

> On 09/01/2018 09:43, Liran Alon wrote:
> > 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.
> > 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.
> 
> In turn, this will cause guests to fail horribly when migrated from
> patched to unpatched hosts (#GP on the first access to
> MSR_IA32_SPEC_CTRL).  Shortcuts simply don't work when you take
> migration into account.

That's true but this will also happen when explicitly exposing new CPUIDs via QEMU cmdline.
The live-migration controller should not attempt to migrate a VM that runs with these CPUIDs to a host
which cannot support them.

But I agree with you, as at least when it is explicit, this scenario can also be checked in QEMU.
(QEMU can check if explicit CPUID flags to expose doesn't match host capabilities).

-Liran

> 
> Paolo

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

[Paolo Bonzini]

On 09/01/2018 07:25, Wei Wang wrote:
> This patch shows an alternative approach to the one posted here:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1580364.html
> 
> 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, 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);

Is this meant to replace the whole series or just patch 1/7?  The
functions in patch 1/7 are used later by vmx.c and svm.c.

Paolo

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

[Wei Wang]

This patch shows an alternative approach to the one posted here:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1580364.html

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, 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)))
+		best->edx |= KF(SPEC_CTRL) | KF(STIBP);
+
 	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
 	if (!best) {
 		vcpu->arch.guest_supported_xcr0 = 0;
-- 
2.7.4