Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Sean Christopherson <seanjc@google.com>]

On Fri, Dec 30, 2022, Aaron Lewis wrote:
> Be a good citizen and don't allow any of the supported MPX xfeatures[1]
> to be set if they can't all be set.  That way userspace or a guest
> doesn't fail if it attempts to set them in XCR0.
> 
> [1] CPUID.(EAX=0DH,ECX=0):EAX.BNDREGS[bit-3]
>     CPUID.(EAX=0DH,ECX=0):EAX.BNDCSR[bit-4]
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index c4e8257629165..2431c46d456b4 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -855,6 +855,16 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
>  	return 0;
>  }
>  
> +static u64 sanitize_xcr0(u64 xcr0)
> +{
> +	u64 mask;
> +
> +	mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
> +	if ((xcr0 & mask) != mask)
> +		xcr0 &= ~mask;
> +
> +	return xcr0;
> +}
> +
>  static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  {
>  	struct kvm_cpuid_entry2 *entry;
> @@ -982,6 +992,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
>  		u64 permitted_xss = kvm_caps.supported_xss;
>  
> +		permitted_xcr0 = sanitize_xcr0(permitted_xcr0);


This isn't 100% correct, all usage needs to be sanitized so that KVM provides a
consistent view.  E.g. KVM_CAP_XSAVE2 would report the wrong size.

	case KVM_CAP_XSAVE2: {
		u64 guest_perm = xstate_get_guest_group_perm();

		r = xstate_required_size(kvm_caps.supported_xcr0 & guest_perm, false);
		if (r < sizeof(struct kvm_xsave))
			r = sizeof(struct kvm_xsave);
		break;
	}

Barring a kernel bug, xstate_get_guest_group_perm() will never report partial
support, so I think the easy solution is to sanitize kvm_caps.suport_xcr0.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2480b8027a45..7ea06c58eaf6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9344,6 +9344,10 @@ int kvm_arch_init(void *opaque)
        if (boot_cpu_has(X86_FEATURE_XSAVE)) {
                host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
                kvm_caps.supported_xcr0 = host_xcr0 & KVM_SUPPORTED_XCR0;
+               if (!(kvm_caps.supported_xcr0 & XFEATURE_MASK_BNDREGS) ||
+                   !(kvm_caps.supported_xcr0 & XFEATURE_MASK_BNDCSR))
+                       kvm_caps.supported_xcr0 &= ~(XFEATURE_MASK_BNDREGS |
+                                                    XFEATURE_MASK_BNDCSR);
        }
 
        if (pi_inject_timer == -1)


> +
>  		entry->eax &= permitted_xcr0;
>  		entry->ebx = xstate_required_size(permitted_xcr0, false);
>  		entry->ecx = entry->ebx;
> -- 
> 2.39.0.314.g84b9a713c41-goog
>