[PATCH v4 0/2] x86/kvm/fpu: Fix guest migration bugs that can crash guest

[PATCH v4 0/2] x86/kvm/fpu: Fix guest migration bugs that can crash guest

[Leonardo Bras]

This patchset comes from a bug I found during qemu guest migration from a
host with newer CPU to a host with an older version of this CPU, and thus 
having less FPU features.

When the guests were created, the one with less features is used as 
config, so migration is possible.

Patch 1 fix a bug that always happens during this migration, and is
related to the fact that xsave saves all feature flags, but xrstor does
not touch the PKRU flag. It also changes how fpstate->user_xfeatures
is set, going from kvm_check_cpuid() to the later called
kvm_vcpu_after_set_cpuid().

Patch 2 removes kvm_vcpu_arch.guest_supported_xcr0 since it now 
duplicates guest_fpu.fpstate->user_xfeatures. Some wrappers were
introduced in order to make it easier to read the replaced version.

Patches were compile-tested, and could fix the bug found.

Please let me know of anything to improve!

Best regards,
Leo

--
Changes since v3:
- Add new patch to remove the use of kvm_vcpu_arch.guest_supported_xcr0,
  since it is now duplicating guest_fpu.fpstate->user_xfeatures.
- On patch 1, also avoid setting user_xfeatures on kvm_check_cpuid(),
  since it is already set in kvm_vcpu_after_set_cpuid() now.
Changes since v2:
- Fix building error because I forgot to EXPORT_SYMBOL(fpu_user_cfg)
Changes since v1:
- Instead of masking xfeatures, mask user_xfeatures instead. This will
  only change the value sent to user, instead of the one saved in buf.
- Above change removed the need of the patch 2/2
- Instead of masking the current value of user_xfeatures, save on it
  fpu_user_cfg.default_features & vcpu->arch.guest_supported_xcr0 

Leonardo Bras (2):
  x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0
  x86/kvm/fpu: Remove kvm_vcpu_arch.guest_supported_xcr0

 arch/x86/include/asm/kvm_host.h |  1 -
 arch/x86/kernel/fpu/xstate.c    |  5 ++++-
 arch/x86/kvm/cpuid.c            |  5 ++++-
 arch/x86/kvm/x86.c              | 20 +++++++++++++++-----
 4 files changed, 23 insertions(+), 8 deletions(-)

-- 
2.35.1

[PATCH v4 1/2] x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0

[Leonardo Bras]

During host/guest switch (like in kvm_arch_vcpu_ioctl_run()), the kernel
swaps the fpu between host/guest contexts, by using fpu_swap_kvm_fpstate().

When xsave feature is available, the fpu swap is done by:
- xsave(s) instruction, with guest's fpstate->xfeatures as mask, is used
  to store the current state of the fpu registers to a buffer.
- xrstor(s) instruction, with (fpu_kernel_cfg.max_features &
  XFEATURE_MASK_FPSTATE) as mask, is used to put the buffer into fpu regs.

For xsave(s) the mask is used to limit what parts of the fpu regs will
be copied to the buffer. Likewise on xrstor(s), the mask is used to
limit what parts of the fpu regs will be changed.

The mask for xsave(s), the guest's fpstate->xfeatures, is defined on
kvm_arch_vcpu_create(), which (in summary) sets it to all features
supported by the cpu which are enabled on kernel config.

This means that xsave(s) will save to guest buffer all the fpu regs
contents the cpu has enabled when the guest is paused, even if they
are not used.

This would not be an issue, if xrstor(s) would also do that.

xrstor(s)'s mask for host/guest swap is basically every valid feature
contained in kernel config, except XFEATURE_MASK_PKRU.
Accordingto kernel src, it is instead switched in switch_to() and
flush_thread().

Then, the following happens with a host supporting PKRU starts a
guest that does not support it:
1 - Host has XFEATURE_MASK_PKRU set. 1st switch to guest,
2 - xsave(s) fpu regs to host fpustate (buffer has XFEATURE_MASK_PKRU)
3 - xrstor(s) guest fpustate to fpu regs (fpu regs have XFEATURE_MASK_PKRU)
4 - guest runs, then switch back to host,
5 - xsave(s) fpu regs to guest fpstate (buffer now have XFEATURE_MASK_PKRU)
6 - xrstor(s) host fpstate to fpu regs.
7 - kvm_vcpu_ioctl_x86_get_xsave() copy guest fpstate to userspace (with
    XFEATURE_MASK_PKRU, which should not be supported by guest vcpu)

On 5, even though the guest does not support PKRU, it does have the flag
set on guest fpstate, which is transferred to userspace via vcpu ioctl
KVM_GET_XSAVE.

This becomes a problem when the user decides on migrating the above guest
to another machine that does not support PKRU:
The new host restores guest's fpu regs to as they were before (xrstor(s)),
but since the new host don't support PKRU, a general-protection exception
ocurs in xrstor(s) and that crashes the guest.

This can be solved by making the guest's fpstate->user_xfeatures hold
a copy of guest_supported_xcr0. This way, on 7 the only flags copied to
userspace will be the ones compatible to guest requirements, and thus
there will be no issue during migration.

As a bonus, it will also fail if userspace tries to set fpu features
that are not compatible to the guest configuration. (KVM_SET_XSAVE ioctl)

Also, since kvm_vcpu_after_set_cpuid() now sets fpstate->user_xfeatures,
there is not need to set it in kvm_check_cpuid(). So, change
fpstate_realloc() so it does not touch fpstate->user_xfeatures if a
non-NULL guest_fpu is passed, which is the case when kvm_check_cpuid()
calls it.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 arch/x86/kernel/fpu/xstate.c | 5 ++++-
 arch/x86/kvm/cpuid.c         | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 02b3ddaf4f75..7c7824ae7862 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1558,7 +1558,10 @@ static int fpstate_realloc(u64 xfeatures, unsigned int ksize,
 		fpregs_restore_userregs();
 
 	newfps->xfeatures = curfps->xfeatures | xfeatures;
-	newfps->user_xfeatures = curfps->user_xfeatures | xfeatures;
+
+	if (!guest_fpu)
+		newfps->user_xfeatures = curfps->user_xfeatures | xfeatures;
+
 	newfps->xfd = curfps->xfd & ~xfeatures;
 
 	/* Do the final updates within the locked region */
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 494d4d351859..71125291c578 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -296,6 +296,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	vcpu->arch.guest_supported_xcr0 =
 		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
 
+	vcpu->arch.guest_fpu.fpstate->user_xfeatures = vcpu->arch.guest_supported_xcr0;
+
 	kvm_update_pv_runtime(vcpu);
 
 	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
-- 
2.35.1

Re: [PATCH v4 1/2] x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0

[David Edmondson]

The single line summary is now out of date - there's no new masking.

On Thursday, 2022-02-17 at 02:30:29 -03, Leonardo Bras wrote:

> During host/guest switch (like in kvm_arch_vcpu_ioctl_run()), the kernel
> swaps the fpu between host/guest contexts, by using fpu_swap_kvm_fpstate().
>
> When xsave feature is available, the fpu swap is done by:
> - xsave(s) instruction, with guest's fpstate->xfeatures as mask, is used
>   to store the current state of the fpu registers to a buffer.
> - xrstor(s) instruction, with (fpu_kernel_cfg.max_features &
>   XFEATURE_MASK_FPSTATE) as mask, is used to put the buffer into fpu regs.
>
> For xsave(s) the mask is used to limit what parts of the fpu regs will
> be copied to the buffer. Likewise on xrstor(s), the mask is used to
> limit what parts of the fpu regs will be changed.
>
> The mask for xsave(s), the guest's fpstate->xfeatures, is defined on
> kvm_arch_vcpu_create(), which (in summary) sets it to all features
> supported by the cpu which are enabled on kernel config.
>
> This means that xsave(s) will save to guest buffer all the fpu regs
> contents the cpu has enabled when the guest is paused, even if they
> are not used.
>
> This would not be an issue, if xrstor(s) would also do that.
>
> xrstor(s)'s mask for host/guest swap is basically every valid feature
> contained in kernel config, except XFEATURE_MASK_PKRU.
> Accordingto kernel src, it is instead switched in switch_to() and
> flush_thread().
>
> Then, the following happens with a host supporting PKRU starts a
> guest that does not support it:
> 1 - Host has XFEATURE_MASK_PKRU set. 1st switch to guest,
> 2 - xsave(s) fpu regs to host fpustate (buffer has XFEATURE_MASK_PKRU)
> 3 - xrstor(s) guest fpustate to fpu regs (fpu regs have XFEATURE_MASK_PKRU)
> 4 - guest runs, then switch back to host,
> 5 - xsave(s) fpu regs to guest fpstate (buffer now have XFEATURE_MASK_PKRU)
> 6 - xrstor(s) host fpstate to fpu regs.
> 7 - kvm_vcpu_ioctl_x86_get_xsave() copy guest fpstate to userspace (with
>     XFEATURE_MASK_PKRU, which should not be supported by guest vcpu)
>
> On 5, even though the guest does not support PKRU, it does have the flag
> set on guest fpstate, which is transferred to userspace via vcpu ioctl
> KVM_GET_XSAVE.
>
> This becomes a problem when the user decides on migrating the above guest
> to another machine that does not support PKRU:
> The new host restores guest's fpu regs to as they were before (xrstor(s)),
> but since the new host don't support PKRU, a general-protection exception
> ocurs in xrstor(s) and that crashes the guest.
>
> This can be solved by making the guest's fpstate->user_xfeatures hold
> a copy of guest_supported_xcr0. This way, on 7 the only flags copied to
> userspace will be the ones compatible to guest requirements, and thus
> there will be no issue during migration.
>
> As a bonus, it will also fail if userspace tries to set fpu features
> that are not compatible to the guest configuration. (KVM_SET_XSAVE ioctl)
>
> Also, since kvm_vcpu_after_set_cpuid() now sets fpstate->user_xfeatures,
> there is not need to set it in kvm_check_cpuid(). So, change
> fpstate_realloc() so it does not touch fpstate->user_xfeatures if a
> non-NULL guest_fpu is passed, which is the case when kvm_check_cpuid()
> calls it.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  arch/x86/kernel/fpu/xstate.c | 5 ++++-
>  arch/x86/kvm/cpuid.c         | 2 ++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 02b3ddaf4f75..7c7824ae7862 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1558,7 +1558,10 @@ static int fpstate_realloc(u64 xfeatures, unsigned int ksize,
>  		fpregs_restore_userregs();
>
>  	newfps->xfeatures = curfps->xfeatures | xfeatures;
> -	newfps->user_xfeatures = curfps->user_xfeatures | xfeatures;
> +
> +	if (!guest_fpu)
> +		newfps->user_xfeatures = curfps->user_xfeatures | xfeatures;
> +
>  	newfps->xfd = curfps->xfd & ~xfeatures;
>
>  	/* Do the final updates within the locked region */
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 494d4d351859..71125291c578 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -296,6 +296,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	vcpu->arch.guest_supported_xcr0 =
>  		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
>
> +	vcpu->arch.guest_fpu.fpstate->user_xfeatures = vcpu->arch.guest_supported_xcr0;
> +
>  	kvm_update_pv_runtime(vcpu);
>
>  	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);

dme.
-- 
All those lines and circles, to me, a mystery.

Re: [PATCH v4 1/2] x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0

[Paolo Bonzini]

On 2/17/22 13:07, David Edmondson wrote:
> The single line summary is now out of date - there's no new masking.

Thanks for the review, I made the adjustments and pushed to master.

Paolo

> On Thursday, 2022-02-17 at 02:30:29 -03, Leonardo Bras wrote:
> 
>> During host/guest switch (like in kvm_arch_vcpu_ioctl_run()), the kernel
>> swaps the fpu between host/guest contexts, by using fpu_swap_kvm_fpstate().
>>
>> When xsave feature is available, the fpu swap is done by:
>> - xsave(s) instruction, with guest's fpstate->xfeatures as mask, is used
>>    to store the current state of the fpu registers to a buffer.
>> - xrstor(s) instruction, with (fpu_kernel_cfg.max_features &
>>    XFEATURE_MASK_FPSTATE) as mask, is used to put the buffer into fpu regs.
>>
>> For xsave(s) the mask is used to limit what parts of the fpu regs will
>> be copied to the buffer. Likewise on xrstor(s), the mask is used to
>> limit what parts of the fpu regs will be changed.
>>
>> The mask for xsave(s), the guest's fpstate->xfeatures, is defined on
>> kvm_arch_vcpu_create(), which (in summary) sets it to all features
>> supported by the cpu which are enabled on kernel config.
>>
>> This means that xsave(s) will save to guest buffer all the fpu regs
>> contents the cpu has enabled when the guest is paused, even if they
>> are not used.
>>
>> This would not be an issue, if xrstor(s) would also do that.
>>
>> xrstor(s)'s mask for host/guest swap is basically every valid feature
>> contained in kernel config, except XFEATURE_MASK_PKRU.
>> Accordingto kernel src, it is instead switched in switch_to() and
>> flush_thread().
>>
>> Then, the following happens with a host supporting PKRU starts a
>> guest that does not support it:
>> 1 - Host has XFEATURE_MASK_PKRU set. 1st switch to guest,
>> 2 - xsave(s) fpu regs to host fpustate (buffer has XFEATURE_MASK_PKRU)
>> 3 - xrstor(s) guest fpustate to fpu regs (fpu regs have XFEATURE_MASK_PKRU)
>> 4 - guest runs, then switch back to host,
>> 5 - xsave(s) fpu regs to guest fpstate (buffer now have XFEATURE_MASK_PKRU)
>> 6 - xrstor(s) host fpstate to fpu regs.
>> 7 - kvm_vcpu_ioctl_x86_get_xsave() copy guest fpstate to userspace (with
>>      XFEATURE_MASK_PKRU, which should not be supported by guest vcpu)
>>
>> On 5, even though the guest does not support PKRU, it does have the flag
>> set on guest fpstate, which is transferred to userspace via vcpu ioctl
>> KVM_GET_XSAVE.
>>
>> This becomes a problem when the user decides on migrating the above guest
>> to another machine that does not support PKRU:
>> The new host restores guest's fpu regs to as they were before (xrstor(s)),
>> but since the new host don't support PKRU, a general-protection exception
>> ocurs in xrstor(s) and that crashes the guest.
>>
>> This can be solved by making the guest's fpstate->user_xfeatures hold
>> a copy of guest_supported_xcr0. This way, on 7 the only flags copied to
>> userspace will be the ones compatible to guest requirements, and thus
>> there will be no issue during migration.
>>
>> As a bonus, it will also fail if userspace tries to set fpu features
>> that are not compatible to the guest configuration. (KVM_SET_XSAVE ioctl)
>>
>> Also, since kvm_vcpu_after_set_cpuid() now sets fpstate->user_xfeatures,
>> there is not need to set it in kvm_check_cpuid(). So, change
>> fpstate_realloc() so it does not touch fpstate->user_xfeatures if a
>> non-NULL guest_fpu is passed, which is the case when kvm_check_cpuid()
>> calls it.
>>
>> Signed-off-by: Leonardo Bras <leobras@redhat.com>
>> ---
>>   arch/x86/kernel/fpu/xstate.c | 5 ++++-
>>   arch/x86/kvm/cpuid.c         | 2 ++
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>> index 02b3ddaf4f75..7c7824ae7862 100644
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -1558,7 +1558,10 @@ static int fpstate_realloc(u64 xfeatures, unsigned int ksize,
>>   		fpregs_restore_userregs();
>>
>>   	newfps->xfeatures = curfps->xfeatures | xfeatures;
>> -	newfps->user_xfeatures = curfps->user_xfeatures | xfeatures;
>> +
>> +	if (!guest_fpu)
>> +		newfps->user_xfeatures = curfps->user_xfeatures | xfeatures;
>> +
>>   	newfps->xfd = curfps->xfd & ~xfeatures;
>>
>>   	/* Do the final updates within the locked region */
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 494d4d351859..71125291c578 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -296,6 +296,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>>   	vcpu->arch.guest_supported_xcr0 =
>>   		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
>>
>> +	vcpu->arch.guest_fpu.fpstate->user_xfeatures = vcpu->arch.guest_supported_xcr0;
>> +
>>   	kvm_update_pv_runtime(vcpu);
>>
>>   	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
> 
> dme.

[PATCH v4 2/2] x86/kvm/fpu: Remove kvm_vcpu_arch.guest_supported_xcr0

[Leonardo Bras]

kvm_vcpu_arch currently contains the guest supported features in both
guest_supported_xcr0 and guest_fpu.fpstate->user_xfeatures field.

Currently both fields are set to the same value in
kvm_vcpu_after_set_cpuid() and are not changed anywhere else after that.

Since it's not good to keep duplicated data, remove guest_supported_xcr0.

To keep the code more readable, introduce kvm_guest_supported_xcr()
and kvm_guest_supported_xfd() to replace the previous usages of
guest_supported_xcr0.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 -
 arch/x86/kvm/cpuid.c            |  5 +++--
 arch/x86/kvm/x86.c              | 20 +++++++++++++++-----
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6dcccb304775..ec9830d2aabf 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -703,7 +703,6 @@ struct kvm_vcpu_arch {
 	struct fpu_guest guest_fpu;
 
 	u64 xcr0;
-	u64 guest_supported_xcr0;
 
 	struct kvm_pio_request pio;
 	void *pio_data;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 71125291c578..b8f8d268d058 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -282,6 +282,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	struct kvm_cpuid_entry2 *best;
+	u64 guest_supported_xcr0;
 
 	best = kvm_find_cpuid_entry(vcpu, 1, 0);
 	if (best && apic) {
@@ -293,10 +294,10 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 		kvm_apic_set_version(vcpu);
 	}
 
-	vcpu->arch.guest_supported_xcr0 =
+	guest_supported_xcr0 =
 		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
 
-	vcpu->arch.guest_fpu.fpstate->user_xfeatures = vcpu->arch.guest_supported_xcr0;
+	vcpu->arch.guest_fpu.fpstate->user_xfeatures = guest_supported_xcr0;
 
 	kvm_update_pv_runtime(vcpu);
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 641044db415d..92177e2ff664 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -984,6 +984,18 @@ void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_load_host_xsave_state);
 
+static inline u64 kvm_guest_supported_xcr(struct kvm_vcpu *vcpu)
+{
+	u64 guest_supported_xcr0 = vcpu->arch.guest_fpu.fpstate->user_xfeatures;
+
+	return guest_supported_xcr0;
+}
+
+static inline u64 kvm_guest_supported_xfd(struct kvm_vcpu *vcpu)
+{
+	return kvm_guest_supported_xcr(vcpu) & XFEATURE_MASK_USER_DYNAMIC;
+}
+
 static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 {
 	u64 xcr0 = xcr;
@@ -1003,7 +1015,7 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 	 * saving.  However, xcr0 bit 0 is always set, even if the
 	 * emulated CPU does not support XSAVE (see kvm_vcpu_reset()).
 	 */
-	valid_bits = vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FP;
+	valid_bits = kvm_guest_supported_xcr(vcpu) | XFEATURE_MASK_FP;
 	if (xcr0 & ~valid_bits)
 		return 1;
 
@@ -3706,8 +3718,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		    !guest_cpuid_has(vcpu, X86_FEATURE_XFD))
 			return 1;
 
-		if (data & ~(XFEATURE_MASK_USER_DYNAMIC &
-			     vcpu->arch.guest_supported_xcr0))
+		if (data & ~(kvm_guest_supported_xfd(vcpu)))
 			return 1;
 
 		fpu_update_guest_xfd(&vcpu->arch.guest_fpu, data);
@@ -3717,8 +3728,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		    !guest_cpuid_has(vcpu, X86_FEATURE_XFD))
 			return 1;
 
-		if (data & ~(XFEATURE_MASK_USER_DYNAMIC &
-			     vcpu->arch.guest_supported_xcr0))
+		if (data & ~(kvm_guest_supported_xfd(vcpu)))
 			return 1;
 
 		vcpu->arch.guest_fpu.xfd_err = data;
-- 
2.35.1

Re: [PATCH v4 2/2] x86/kvm/fpu: Remove kvm_vcpu_arch.guest_supported_xcr0

[David Edmondson]

On Thursday, 2022-02-17 at 02:30:30 -03, Leonardo Bras wrote:

> kvm_vcpu_arch currently contains the guest supported features in both
> guest_supported_xcr0 and guest_fpu.fpstate->user_xfeatures field.
>
> Currently both fields are set to the same value in
> kvm_vcpu_after_set_cpuid() and are not changed anywhere else after that.
>
> Since it's not good to keep duplicated data, remove guest_supported_xcr0.
>
> To keep the code more readable, introduce kvm_guest_supported_xcr()
> and kvm_guest_supported_xfd() to replace the previous usages of
> guest_supported_xcr0.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  1 -
>  arch/x86/kvm/cpuid.c            |  5 +++--
>  arch/x86/kvm/x86.c              | 20 +++++++++++++++-----
>  3 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 6dcccb304775..ec9830d2aabf 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -703,7 +703,6 @@ struct kvm_vcpu_arch {
>  	struct fpu_guest guest_fpu;
>
>  	u64 xcr0;
> -	u64 guest_supported_xcr0;
>
>  	struct kvm_pio_request pio;
>  	void *pio_data;
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 71125291c578..b8f8d268d058 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -282,6 +282,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  	struct kvm_cpuid_entry2 *best;
> +	u64 guest_supported_xcr0;

The intermediate variable seems unnecessary.

>
>  	best = kvm_find_cpuid_entry(vcpu, 1, 0);
>  	if (best && apic) {
> @@ -293,10 +294,10 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  		kvm_apic_set_version(vcpu);
>  	}
>
> -	vcpu->arch.guest_supported_xcr0 =
> +	guest_supported_xcr0 =
>  		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
>
> -	vcpu->arch.guest_fpu.fpstate->user_xfeatures = vcpu->arch.guest_supported_xcr0;
> +	vcpu->arch.guest_fpu.fpstate->user_xfeatures = guest_supported_xcr0;
>
>  	kvm_update_pv_runtime(vcpu);
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 641044db415d..92177e2ff664 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -984,6 +984,18 @@ void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_load_host_xsave_state);
>
> +static inline u64 kvm_guest_supported_xcr(struct kvm_vcpu *vcpu)
> +{
> +	u64 guest_supported_xcr0 = vcpu->arch.guest_fpu.fpstate->user_xfeatures;

...and here.

> +
> +	return guest_supported_xcr0;
> +}
> +
> +static inline u64 kvm_guest_supported_xfd(struct kvm_vcpu *vcpu)
> +{
> +	return kvm_guest_supported_xcr(vcpu) & XFEATURE_MASK_USER_DYNAMIC;
> +}
> +
>  static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>  {
>  	u64 xcr0 = xcr;
> @@ -1003,7 +1015,7 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>  	 * saving.  However, xcr0 bit 0 is always set, even if the
>  	 * emulated CPU does not support XSAVE (see kvm_vcpu_reset()).
>  	 */
> -	valid_bits = vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FP;
> +	valid_bits = kvm_guest_supported_xcr(vcpu) | XFEATURE_MASK_FP;
>  	if (xcr0 & ~valid_bits)
>  		return 1;
>
> @@ -3706,8 +3718,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		    !guest_cpuid_has(vcpu, X86_FEATURE_XFD))
>  			return 1;
>
> -		if (data & ~(XFEATURE_MASK_USER_DYNAMIC &
> -			     vcpu->arch.guest_supported_xcr0))
> +		if (data & ~(kvm_guest_supported_xfd(vcpu)))

Brackets could be removed...

>  			return 1;
>
>  		fpu_update_guest_xfd(&vcpu->arch.guest_fpu, data);
> @@ -3717,8 +3728,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		    !guest_cpuid_has(vcpu, X86_FEATURE_XFD))
>  			return 1;
>
> -		if (data & ~(XFEATURE_MASK_USER_DYNAMIC &
> -			     vcpu->arch.guest_supported_xcr0))
> +		if (data & ~(kvm_guest_supported_xfd(vcpu)))

...and here.

>  			return 1;
>
>  		vcpu->arch.guest_fpu.xfd_err = data;

dme.
-- 
But he said, leave me alone, I'm a family man.

Re: [PATCH v4 0/2] x86/kvm/fpu: Fix guest migration bugs that can crash guest

[Paolo Bonzini]

On 2/17/22 06:30, Leonardo Bras wrote:
> This patchset comes from a bug I found during qemu guest migration from a
> host with newer CPU to a host with an older version of this CPU, and thus
> having less FPU features.
> 
> When the guests were created, the one with less features is used as
> config, so migration is possible.
> 
> Patch 1 fix a bug that always happens during this migration, and is
> related to the fact that xsave saves all feature flags, but xrstor does
> not touch the PKRU flag. It also changes how fpstate->user_xfeatures
> is set, going from kvm_check_cpuid() to the later called
> kvm_vcpu_after_set_cpuid().
> 
> Patch 2 removes kvm_vcpu_arch.guest_supported_xcr0 since it now
> duplicates guest_fpu.fpstate->user_xfeatures. Some wrappers were
> introduced in order to make it easier to read the replaced version.
> 
> Patches were compile-tested, and could fix the bug found.

Queued, thanks (for 5.17 of course)!  For patch 2, I renamed the 
function to kvm_guest_supported_xcr0.

Paolo

> Please let me know of anything to improve!
> 
> Best regards,
> Leo
> 
> --
> Changes since v3:
> - Add new patch to remove the use of kvm_vcpu_arch.guest_supported_xcr0,
>    since it is now duplicating guest_fpu.fpstate->user_xfeatures.
> - On patch 1, also avoid setting user_xfeatures on kvm_check_cpuid(),
>    since it is already set in kvm_vcpu_after_set_cpuid() now.
> Changes since v2:
> - Fix building error because I forgot to EXPORT_SYMBOL(fpu_user_cfg)
> Changes since v1:
> - Instead of masking xfeatures, mask user_xfeatures instead. This will
>    only change the value sent to user, instead of the one saved in buf.
> - Above change removed the need of the patch 2/2
> - Instead of masking the current value of user_xfeatures, save on it
>    fpu_user_cfg.default_features & vcpu->arch.guest_supported_xcr0
> 
> Leonardo Bras (2):
>    x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0
>    x86/kvm/fpu: Remove kvm_vcpu_arch.guest_supported_xcr0
> 
>   arch/x86/include/asm/kvm_host.h |  1 -
>   arch/x86/kernel/fpu/xstate.c    |  5 ++++-
>   arch/x86/kvm/cpuid.c            |  5 ++++-
>   arch/x86/kvm/x86.c              | 20 +++++++++++++++-----
>   4 files changed, 23 insertions(+), 8 deletions(-)
> 

Re: [PATCH v4 0/2] x86/kvm/fpu: Fix guest migration bugs that can crash guest

[Leonardo Bras Soares Passos]

On Thu, Feb 17, 2022 at 11:52 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 2/17/22 06:30, Leonardo Bras wrote:
> > This patchset comes from a bug I found during qemu guest migration from a
> > host with newer CPU to a host with an older version of this CPU, and thus
> > having less FPU features.
> >
> > When the guests were created, the one with less features is used as
> > config, so migration is possible.
> >
> > Patch 1 fix a bug that always happens during this migration, and is
> > related to the fact that xsave saves all feature flags, but xrstor does
> > not touch the PKRU flag. It also changes how fpstate->user_xfeatures
> > is set, going from kvm_check_cpuid() to the later called
> > kvm_vcpu_after_set_cpuid().
> >
> > Patch 2 removes kvm_vcpu_arch.guest_supported_xcr0 since it now
> > duplicates guest_fpu.fpstate->user_xfeatures. Some wrappers were
> > introduced in order to make it easier to read the replaced version.
> >
> > Patches were compile-tested, and could fix the bug found.
>
> Queued, thanks (for 5.17 of course)!  For patch 2, I renamed the
> function to kvm_guest_supported_xcr0.
>
> Paolo
>

That's great!
Thanks Paolo!