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:   Mon, 26 Jun 2023 17:24:36 +0800
From:   "Yang, Weijiang" <weijiang.yang@...el.com>
To:     Sean Christopherson <seanjc@...gle.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 6/24/2023 7:21 AM, Sean Christopherson wrote:
> 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.

OK, I'll add a new selftest app which initially only includes CET MSRs 
testing but practice

the above ideas.

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