linux-kernel - Re: [PATCH v9 13/27] KVM: x86: Refresh CPUID on write to guest MSR_IA32

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZbI+pexl9Th0KiiU@chao-email>
Date: Thu, 25 Jan 2024 18:57:41 +0800
From: Chao Gao <chao.gao@...el.com>
To: Yang Weijiang <weijiang.yang@...el.com>
CC: <seanjc@...gle.com>, <pbonzini@...hat.com>, <dave.hansen@...el.com>,
	<kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <x86@...nel.org>,
	<yuan.yao@...ux.intel.com>, <peterz@...radead.org>,
	<rick.p.edgecombe@...el.com>, <mlevitsk@...hat.com>, <john.allen@....com>,
	Zhang Yi Z <yi.z.zhang@...ux.intel.com>
Subject: Re: [PATCH v9 13/27] KVM: x86: Refresh CPUID on write to guest
 MSR_IA32_XSS

On Tue, Jan 23, 2024 at 06:41:46PM -0800, Yang Weijiang wrote:
>Update CPUID.(EAX=0DH,ECX=1).EBX to reflect current required xstate size
>due to XSS MSR modification.
>CPUID(EAX=0DH,ECX=1).EBX reports the required storage size of all enabled
>xstate features in (XCR0 | IA32_XSS). The CPUID value can be used by guest
>before allocate sufficient xsave buffer.
>
>Note, KVM does not yet support any XSS based features, i.e. supported_xss
>is guaranteed to be zero at this time.
>
>Opportunistically modify XSS write access logic as:
>If XSAVES is not enabled in the guest CPUID, forbid setting IA32_XSS msr
>to anything but 0, even if the write is host initiated.

any reason to allow host to write 0? looks we are not doing this for many
other MSRs.

>
>Suggested-by: Sean Christopherson <seanjc@...gle.com>
>Co-developed-by: Zhang Yi Z <yi.z.zhang@...ux.intel.com>
>Signed-off-by: Zhang Yi Z <yi.z.zhang@...ux.intel.com>
>Signed-off-by: Yang Weijiang <weijiang.yang@...el.com>
>Reviewed-by: Maxim Levitsky <mlevitsk@...hat.com>
>---
> arch/x86/include/asm/kvm_host.h |  3 ++-
> arch/x86/kvm/cpuid.c            | 15 ++++++++++++++-
> arch/x86/kvm/x86.c              | 16 ++++++++++++----
> 3 files changed, 28 insertions(+), 6 deletions(-)
>
>diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>index 40dd796ea085..6efaaaa15945 100644
>--- a/arch/x86/include/asm/kvm_host.h
>+++ b/arch/x86/include/asm/kvm_host.h
>@@ -772,7 +772,6 @@ struct kvm_vcpu_arch {
> 	bool at_instruction_boundary;
> 	bool tpr_access_reporting;
> 	bool xfd_no_write_intercept;
>-	u64 ia32_xss;
> 	u64 microcode_version;
> 	u64 arch_capabilities;
> 	u64 perf_capabilities;
>@@ -828,6 +827,8 @@ struct kvm_vcpu_arch {
> 
> 	u64 xcr0;
> 	u64 guest_supported_xcr0;
>+	u64 guest_supported_xss;
>+	u64 ia32_xss;
> 
> 	struct kvm_pio_request pio;
> 	void *pio_data;
>diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>index acc360c76318..3ab133530573 100644
>--- a/arch/x86/kvm/cpuid.c
>+++ b/arch/x86/kvm/cpuid.c
>@@ -275,7 +275,8 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
> 	best = cpuid_entry2_find(entries, nent, 0xD, 1);
> 	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
> 		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
>-		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>+		best->ebx = xstate_required_size(vcpu->arch.xcr0 |
>+						 vcpu->arch.ia32_xss, true);
> 
> 	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
> 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
>@@ -312,6 +313,17 @@ static u64 vcpu_get_supported_xcr0(struct kvm_vcpu *vcpu)
> 	return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0;
> }
> 
>+static u64 vcpu_get_supported_xss(struct kvm_vcpu *vcpu)
>+{
>+	struct kvm_cpuid_entry2 *best;
>+
>+	best = kvm_find_cpuid_entry_index(vcpu, 0xd, 1);
>+	if (!best)
>+		return 0;
>+
>+	return (best->ecx | ((u64)best->edx << 32)) & kvm_caps.supported_xss;
>+}
>+
> static bool kvm_cpuid_has_hyperv(struct kvm_cpuid_entry2 *entries, int nent)
> {
> #ifdef CONFIG_KVM_HYPERV
>@@ -362,6 +374,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> 	}
> 
> 	vcpu->arch.guest_supported_xcr0 = vcpu_get_supported_xcr0(vcpu);
>+	vcpu->arch.guest_supported_xss = vcpu_get_supported_xss(vcpu);
> 
> 	kvm_update_pv_runtime(vcpu);
> 
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index b3a39886e418..7b7a15aab3aa 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -3924,20 +3924,28 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> 			vcpu->arch.ia32_tsc_adjust_msr += adj;
> 		}
> 		break;
>-	case MSR_IA32_XSS:
>-		if (!msr_info->host_initiated &&
>-		    !guest_cpuid_has(vcpu, X86_FEATURE_XSAVES))
>+	case MSR_IA32_XSS: {

unnecessary bracket.

>+		/*
>+		 * If KVM reported support of XSS MSR, even guest CPUID doesn't

IIUC, below code doesn't check if KVM reported support of XSS MSR. so, the comment
doesn't match what the code does.

>+		 * support XSAVES, still allow userspace to set default value(0)
>+		 * to this MSR.
>+		 */
>+		if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVES) &&
>+		    !(msr_info->host_initiated && data == 0))
> 			return 1;
> 		/*
> 		 * KVM supports exposing PT to the guest, but does not support
> 		 * IA32_XSS[bit 8]. Guests have to use RDMSR/WRMSR rather than
> 		 * XSAVES/XRSTORS to save/restore PT MSRs.
> 		 */
>-		if (data & ~kvm_caps.supported_xss)
>+		if (data & ~vcpu->arch.guest_supported_xss)
> 			return 1;
>+		if (vcpu->arch.ia32_xss == data)
>+			break;
> 		vcpu->arch.ia32_xss = data;
> 		kvm_update_cpuid_runtime(vcpu);
> 		break;
>+	}
> 	case MSR_SMI_COUNT:
> 		if (!msr_info->host_initiated)
> 			return 1;
>-- 
>2.39.3
>