linux-kernel - Re: [PATCH 4/7] KVM: TDX: restore host xsave state when exit from the guest TD

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: <a42183ab-a25a-423e-9ef3-947abec20561@intel.com>
Date: Mon, 25 Nov 2024 13:10:37 +0200
From: Adrian Hunter <adrian.hunter@...el.com>
To: Chao Gao <chao.gao@...el.com>
Cc: pbonzini@...hat.com, seanjc@...gle.com, kvm@...r.kernel.org,
 dave.hansen@...ux.intel.com, rick.p.edgecombe@...el.com,
 kai.huang@...el.com, reinette.chatre@...el.com, xiaoyao.li@...el.com,
 tony.lindgren@...ux.intel.com, binbin.wu@...ux.intel.com,
 dmatlack@...gle.com, isaku.yamahata@...el.com, nik.borisov@...e.com,
 linux-kernel@...r.kernel.org, x86@...nel.org, yan.y.zhao@...el.com,
 weijiang.yang@...el.com
Subject: Re: [PATCH 4/7] KVM: TDX: restore host xsave state when exit from the
 guest TD

On 22/11/24 07:49, Chao Gao wrote:
>> +static void tdx_restore_host_xsave_state(struct kvm_vcpu *vcpu)
>> +{
>> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
>> +
>> +	if (static_cpu_has(X86_FEATURE_XSAVE) &&
>> +	    kvm_host.xcr0 != (kvm_tdx->xfam & kvm_caps.supported_xcr0))
>> +		xsetbv(XCR_XFEATURE_ENABLED_MASK, kvm_host.xcr0);
>> +	if (static_cpu_has(X86_FEATURE_XSAVES) &&
>> +	    /* PT can be exposed to TD guest regardless of KVM's XSS support */
>> +	    kvm_host.xss != (kvm_tdx->xfam &
>> +			 (kvm_caps.supported_xss | XFEATURE_MASK_PT |
>> +			  XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL)))
> 
> Should we drop CET/PT from this series? I think they are worth a new
> patch/series.

This is not really about CET/PT

What is happening here is that we are calculating the current
MSR_IA32_XSS value based on the TDX Module spec which says the
TDX Module sets MSR_IA32_XSS to the XSS bits from XFAM.  The
TDX Module does that literally, from TDX Module source code:

	#define XCR0_SUPERVISOR_BIT_MASK            0x0001FD00
and
	ia32_wrmsr(IA32_XSS_MSR_ADDR, xfam & XCR0_SUPERVISOR_BIT_MASK);

For KVM, rather than:

			kvm_tdx->xfam &
			 (kvm_caps.supported_xss | XFEATURE_MASK_PT |
			  XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL)

it would be more direct to define the bits and enforce them
via tdx_get_supported_xfam() e.g.

/* 
 * Before returning from TDH.VP.ENTER, the TDX Module assigns:
 *   XCR0 to the TD’s user-mode feature bits of XFAM (bits 7:0, 9)
 *   IA32_XSS to the TD's supervisor-mode feature bits of XFAM (bits 8, 16:10)
 */
#define TDX_XFAM_XCR0_MASK	(GENMASK(7, 0) | BIT(9))
#define TDX_XFAM_XSS_MASK	(GENMASK(16, 10) | BIT(8))
#define TDX_XFAM_MASK		(TDX_XFAM_XCR0_MASK | TDX_XFAM_XSS_MASK)

static u64 tdx_get_supported_xfam(const struct tdx_sys_info_td_conf *td_conf)
{
	u64 val = kvm_caps.supported_xcr0 | kvm_caps.supported_xss;

	/* Ensure features are in the masks */
	val &= TDX_XFAM_MASK;

	if ((val & td_conf->xfam_fixed1) != td_conf->xfam_fixed1)
		return 0;

	val &= td_conf->xfam_fixed0;

	return val;
}

and then:

	if (static_cpu_has(X86_FEATURE_XSAVE) &&
	    kvm_host.xcr0 != (kvm_tdx->xfam & TDX_XFAM_XCR0_MASK))
		xsetbv(XCR_XFEATURE_ENABLED_MASK, kvm_host.xcr0);
	if (static_cpu_has(X86_FEATURE_XSAVES) &&
	    kvm_host.xss != (kvm_tdx->xfam & TDX_XFAM_XSS_MASK))
		wrmsrl(MSR_IA32_XSS, kvm_host.xss);

> 
>> +		wrmsrl(MSR_IA32_XSS, kvm_host.xss);
>> +	if (static_cpu_has(X86_FEATURE_PKU) &&
> 
> How about using cpu_feature_enabled()? It is used in kvm_load_host_xsave_state()
> It handles the case where CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS is not
> enabled.
> 
>> +	    (kvm_tdx->xfam & XFEATURE_MASK_PKRU))
>> +		write_pkru(vcpu->arch.host_pkru);
> 
> If host_pkru happens to match the hardware value after TD-exits, the write can
> be omitted, similar to what is done above for xss and xcr0.

True.  It might be better to make restoring PKRU a separate
patch so that the commit message can explain why it needs to
be done here.