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: <ZMk14YiPw9l7ZTXP@google.com>
Date:   Tue, 1 Aug 2023 09:42:09 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     John Allen <john.allen@....com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, weijiang.yang@...el.com,
        rick.p.edgecombe@...el.com, x86@...nel.org,
        thomas.lendacky@....com, bp@...en8.de
Subject: Re: [RFC PATCH v2 3/6] KVM: x86: SVM: Pass through shadow stack MSRs

On Tue, Aug 01, 2023, John Allen wrote:
> On Fri, Jun 23, 2023 at 05:05:18PM -0700, Sean Christopherson wrote:
> > On Wed, May 24, 2023, John Allen wrote:
> > > If kvm supports shadow stack, pass through shadow stack MSRs to improve
> > > guest performance.
> > > 
> > > Signed-off-by: John Allen <john.allen@....com>
> > > ---
> > >  arch/x86/kvm/svm/svm.c | 17 +++++++++++++++++
> > >  arch/x86/kvm/svm/svm.h |  2 +-
> > >  2 files changed, 18 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > index 6df486bb1ac4..cdbce20989b8 100644
> > > --- a/arch/x86/kvm/svm/svm.c
> > > +++ b/arch/x86/kvm/svm/svm.c
> > > @@ -136,6 +136,13 @@ static const struct svm_direct_access_msrs {
> > >  	{ .index = X2APIC_MSR(APIC_TMICT),		.always = false },
> > >  	{ .index = X2APIC_MSR(APIC_TMCCT),		.always = false },
> > >  	{ .index = X2APIC_MSR(APIC_TDCR),		.always = false },
> > > +	{ .index = MSR_IA32_U_CET,                      .always = false },
> > > +	{ .index = MSR_IA32_S_CET,                      .always = false },
> > > +	{ .index = MSR_IA32_INT_SSP_TAB,                .always = false },
> > > +	{ .index = MSR_IA32_PL0_SSP,                    .always = false },
> > > +	{ .index = MSR_IA32_PL1_SSP,                    .always = false },
> > > +	{ .index = MSR_IA32_PL2_SSP,                    .always = false },
> > > +	{ .index = MSR_IA32_PL3_SSP,                    .always = false },
> > >  	{ .index = MSR_INVALID,				.always = false },
> > >  };
> > >  
> > > @@ -1181,6 +1188,16 @@ static inline void init_vmcb_after_set_cpuid(struct kvm_vcpu *vcpu)
> > >  		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_SYSENTER_EIP, 1, 1);
> > >  		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_SYSENTER_ESP, 1, 1);
> > >  	}
> > > +
> > > +	if (kvm_cet_user_supported() && guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) {
> > > +		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_U_CET, 1, 1);
> > > +		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_S_CET, 1, 1);
> > > +		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_INT_SSP_TAB, 1, 1);
> > > +		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_PL0_SSP, 1, 1);
> > > +		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_PL1_SSP, 1, 1);
> > > +		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_PL2_SSP, 1, 1);
> > > +		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_PL3_SSP, 1, 1);
> > > +	}
> > 
> > This is wrong, KVM needs to set/clear interception based on SHSKT, i.e. it can't
> > be a one-way street.  Userspace *probably* won't toggle SHSTK in guest CPUID, but
> > weirder things have happened.
> 
> Can you clarify what you mean by that? Do you mean that we need to check
> both guest_cpuid_has and kvm_cpu_cap_has like the guest_can_use function
> that is used in Weijiang Yang's series? Or is there something else I'm
> omitting here?

When init_vmcb_after_set_cpuid() is called, KVM must not assume that the MSRs are
currently intercepted, i.e. KVM can't just handle the case where userspace enables
SHSTK, KVM must also handle the case where userspace disables SHSTK.

Using guest_can_use() is also a good idea, but it would likely lead to extra work
on CPUs that don't support CET/SHSTK.  This isn't a fastpath, but toggling
interception for MSRs that don't exist would be odd.  It's probably better to
effectively open code guest_can_use(), which the KVM check gating the MSR toggling.

E.g. something like

	if (kvm_cpu_cap_has(X86_FEATURE_SHSTK)) {
		bool shstk_enabled = guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);

		set_msr_inteception(vcpu, svm->msrpm, MSR_IA32_BLAH,
				    shstk_enabled, shstk_enabled);
	}

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ