linux-kernel - Re: [PATCH v3 07/21] 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]

Date:   Fri, 23 Jun 2023 16:21:13 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Weijiang Yang <weijiang.yang@...el.com>
Cc:     Chao Gao <chao.gao@...el.com>, pbonzini@...hat.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        peterz@...radead.org, rppt@...nel.org, binbin.wu@...ux.intel.com,
        rick.p.edgecombe@...el.com, john.allen@....com,
        Zhang Yi Z <yi.z.zhang@...ux.intel.com>
Subject: Re: [PATCH v3 07/21] KVM:x86: Refresh CPUID on write to guest MSR_IA32_XSS

On Fri, Jun 16, 2023, Weijiang Yang wrote:
> 
> On 6/16/2023 7:45 AM, Sean Christopherson wrote:
> > On Wed, May 31, 2023, Weijiang Yang wrote:
> > > On 5/30/2023 8:08 PM, Chao Gao wrote:
> > > > > > > --- a/arch/x86/kvm/x86.c
> > > > > > > +++ b/arch/x86/kvm/x86.c
> > > > > > > @@ -3776,8 +3776,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > > > > > > 		 */
> > > > > > > 		if (data & ~kvm_caps.supported_xss)
> > > > > > Shouldn't we check against the supported value of _this_ guest? similar to
> > > > > > guest_supported_xcr0.
> > > > > I don't think it requires an extra variable to serve per guest purpose.
> > > > > 
> > > > > For guest XSS settings, now we don't add extra constraints like XCR0, thus
> > > > QEMU can impose constraints by configuring guest CPUID.0xd.1 to indicate
> > > > certain supervisor state components cannot be managed by XSAVES, even
> > > > though KVM supports them. IOW, guests may differ in the supported values
> > > > for the IA32_XSS MSR.
> > > OK, will change this part to align with xcr0 settings. Thanks!
> > Please write KVM-Unit-Tests to verify KVM correctly handles the various MSRs related
> > to CET, e.g. a test_cet_msrs() subtest in msr.c would do nicely.  Hmm, though testing
> > the combinations of CPUID bits will require multiple x86/unittests.cfg entries.
> > Might be time to split up msr.c into a library and then multiple tests.
> 
> Since there's already a CET specific unit test app, do you mind adding all
> CET related stuffs to the app to make it inclusive? e.g.,ï¿½validate constraints
> between CET CPUIDs vs. CET/XSS MSRs?

Hmm, that will get a bit kludgy since the MSR testcases will want to toggle IBT
and SHSTK on and off.

Actually, I take back my suggestion to add a KUT test.  Except for a few special
cases, e.g. 32-bit support, selftests is a better framework for testing MSRs than
KUT, as it's relatively easy to create a custom vCPU model in selftests, whereas
in KUT it requires handcoding an entry in unittests.cfg, and having corresponding
code in the test itself.

The biggest gap in selftests was the lack of decent reporting in guest code, but
Aaron is working on closing that gap[*].

I'm thinking something like this as a framework.  

	struct msr_data {
		const uint32_t idx;
		const char *name;
		const struct kvm_x86_cpu_feature feature1;
		const struct kvm_x86_cpu_feature feature2;
		const uint32_t nr_values;
		const uint64_t *values;
	};

	#define TEST_MSR2(msr, f1, f2) { .idx = msr, .name = #msr, .feature1 = f1, .feature2 = f2, .nr_values = ARRAY_SIZE(msr_VALUES), .values = msr_VALUES }
	#define TEST_MSR(msr, f) TEST_MSR2(msr, f, <a dummy value?>)
	#define TEST_MSR0(msr) TEST_MSR(msr, <a dummy value?>)

With CET usage looking like

	static const uint64_t MSR_IA32_S_CET_VALUES[] = {
		<super interesting values>
	};

	TEST_MSR2(MSR_IA32_S_CET, X86_FEATURE_IBT, X86_FEATURE_SHSTK);

Then the test could iterate over each entry and test the various combinations of
features being enabled (if supported by KVM).  And it could also test ioctls(),
which are all but impossible to test in KUT, e.g. verify that supported MSRs are
reported in KVM_GET_MSR_INDEX_LIST, verify that userspace can read/write MSRs
regardless of guest CPUID, etc.  Ooh, and we can even test MSR filtering.

I don't know that we'd want to cram all of those things in a single test, but we
can worry about that later as it shouldn't be difficult to put the framework and
MSR definitions in common code.

[*] https://lore.kernel.org/all/20230607224520.4164598-1-aaronlewis@google.com