linux-kernel - Re: [PATCH] KVM: x86: Update Xen-specific CPUID leaves during mangling

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: <87r04u7ng7.fsf@redhat.com>
Date: Wed, 22 Jan 2025 18:44:40 +0100
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: paul@....org, Fred Griffoul <fgriffo@...zon.co.uk>, kvm@...r.kernel.org
Cc: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
 <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar
 <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
 <dave.hansen@...ux.intel.com>, x86@...nel.org, "H. Peter Anvin"
 <hpa@...or.com>, David Woodhouse <dwmw2@...radead.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: x86: Update Xen-specific CPUID leaves during mangling

Paul Durrant <xadimgnik@...il.com> writes:

> On 22/01/2025 17:16, Vitaly Kuznetsov wrote:
>> Fred Griffoul <fgriffo@...zon.co.uk> writes:
>> 
>>> Previous commit ee3a5f9e3d9b ("KVM: x86: Do runtime CPUID update before
>>> updating vcpu->arch.cpuid_entries") implemented CPUID data mangling in
>>> KVM_SET_CPUID2 support before verifying that no changes occur on running
>>> vCPUs. However, it overlooked the CPUID leaves that are modified by
>>> KVM's Xen emulation.
>>>
>>> Fix this by calling a Xen update function when mangling CPUID data.
>>>
>>> Fixes: ee3a5f9e3d9b ("KVM: x86: Do runtime CPUID update before
>>> updating vcpu->arch.cpuid_entries")
>> 
>> Well, kvm_xen_update_tsc_info() was added with
>> 
>> commit f422f853af0369be27d2a9f1b20079f2bc3d1ca2
>> Author: Paul Durrant <pdurrant@...zon.com>
>> Date:   Fri Jan 6 10:36:00 2023 +0000
>> 
>>      KVM: x86/xen: update Xen CPUID Leaf 4 (tsc info) sub-leaves, if present
>> 
>> and the commit you mention in 'Fixes' is older:
>> 
>> commit ee3a5f9e3d9bf94159f3cc80da542fbe83502dd8
>> Author: Vitaly Kuznetsov <vkuznets@...hat.com>
>> Date:   Mon Jan 17 16:05:39 2022 +0100
>> 
>>      KVM: x86: Do runtime CPUID update before updating vcpu->arch.cpuid_entries
>> 
>> so I guess we should be 'Fixing' f422f853af03 instead :-)
>> 
>>> Signed-off-by: Fred Griffoul <fgriffo@...zon.co.uk>
>>> ---
>>>   arch/x86/kvm/cpuid.c | 1 +
>>>   arch/x86/kvm/xen.c   | 5 +++++
>>>   arch/x86/kvm/xen.h   | 5 +++++
>>>   3 files changed, 11 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>> index edef30359c19..432d8e9e1bab 100644
>>> --- a/arch/x86/kvm/cpuid.c
>>> +++ b/arch/x86/kvm/cpuid.c
>>> @@ -212,6 +212,7 @@ static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2
>>>   	 */
>>>   	kvm_update_cpuid_runtime(vcpu);
>>>   	kvm_apply_cpuid_pv_features_quirk(vcpu);
>>> +	kvm_xen_update_cpuid_runtime(vcpu);
>> 
>> This one is weird as we update it in runtime (kvm_guest_time_update())
>> and values may change when we e.g. migrate the guest. First, I do not
>> understand how the guest is supposed to notice the change as CPUID data
>> is normally considered static. Second, I do not see how the VMM is
>> supposed to track it as if it tries to supply some different data for
>> these Xen leaves, kvm_cpuid_check_equal() will still fail.
>> 
>> Would it make more sense to just ignore these Xen CPUID leaves with TSC
>> information when we do the comparison?
>> 
>
> What is the purpose of the comparison anyway? IIUC we want to ensure 
> that a VMM does not change its mind after KVM_RUN so should we not be 
> stashing what was set by the VMM and comparing against that *before* 
> mangling any values?
>

I guess it can be done this way but we will need to keep these 'original'
unmangled values for the lifetime of the vCPU with very little gain (IMO):
KVM_SET_CPUID{,2} either fails (if the data is different) or does (almost)
nothing when the data is the same.

-- 
Vitaly