linux-kernel - Re: [PATCH 1/3] KVM: x86: disconnect kvm_check_cpuid() from vcpu->arch.cpuid

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: <87imbo99hv.fsf@vitty.brq.redhat.com>
Date:   Mon, 05 Oct 2020 13:51:08 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     Sean Christopherson <sean.j.christopherson@...el.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        "Dr . David Alan Gilbert" <dgilbert@...hat.com>,
        Wei Huang <whuang2@....com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] KVM: x86: disconnect kvm_check_cpuid() from vcpu->arch.cpuid_entries

Maxim Levitsky <mlevitsk@...hat.com> writes:

> On Thu, 2020-10-01 at 15:05 +0200, Vitaly Kuznetsov wrote:
>> As a preparatory step to allocating vcpu->arch.cpuid_entries dynamically
>> make kvm_check_cpuid() check work with an arbitrary 'struct kvm_cpuid_entry2'
>> array.
>> 
>> Currently, when kvm_check_cpuid() fails we reset vcpu->arch.cpuid_nent to
>> 0 and this is kind of weird, i.e. one would expect CPUIDs to remain
>> unchanged when KVM_SET_CPUID[2] call fails.
> Since this specific patch doesn't fix this, maybe move this chunk to following patches,
> or to the cover letter?

Basically, this kind of pairs with what's after 'No functional change
intended' below: we admit the problem but don't fix it because we can't
(yet) and then in PATCH3 we do two things at once. It would be great to
separate these two changes but this doesn't seem to be possible without
an unneeded code churn.

That said, I'm completely fine with dropping this chunk from the commit
message if it sound inapropriate here.

>
>> 
>> No functional change intended. It would've been possible to move the updated
>> kvm_check_cpuid() in kvm_vcpu_ioctl_set_cpuid2() and check the supplied
>> input before we start updating vcpu->arch.cpuid_entries/nent but we
>> can't do the same in kvm_vcpu_ioctl_set_cpuid() as we'll have to copy
>> 'struct kvm_cpuid_entry' entries first. The change will be made when
>> vcpu->arch.cpuid_entries[] array becomes allocated dynamically.
>> 
>> Suggested-by: Sean Christopherson <sean.j.christopherson@...el.com>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
>> ---
>>  arch/x86/kvm/cpuid.c | 38 +++++++++++++++++++++++---------------
>>  1 file changed, 23 insertions(+), 15 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 37c3668a774f..529348ddedc1 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -54,7 +54,24 @@ static u32 xstate_required_size(u64 xstate_bv, bool compacted)
>>  
>>  #define F feature_bit
>>  
>> -static int kvm_check_cpuid(struct kvm_vcpu *vcpu)
>> +static inline struct kvm_cpuid_entry2 *cpuid_entry2_find(
>> +	struct kvm_cpuid_entry2 *entries, int nent, u32 function, u32 index)
>> +{
>> +	struct kvm_cpuid_entry2 *e;
>> +	int i;
>> +
>> +	for (i = 0; i < nent; i++) {
>> +		e = &entries[i];
>> +
>> +		if (e->function == function && (e->index == index ||
>> +		    !(e->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX)))
>> +			return e;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static int kvm_check_cpuid(struct kvm_cpuid_entry2 *entries, int nent)
>>  {
>>  	struct kvm_cpuid_entry2 *best;
>>  
>> @@ -62,7 +79,7 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu)
>>  	 * The existing code assumes virtual address is 48-bit or 57-bit in the
>>  	 * canonical address checks; exit if it is ever changed.
>>  	 */
>> -	best = kvm_find_cpuid_entry(vcpu, 0x80000008, 0);
>> +	best = cpuid_entry2_find(entries, nent, 0x80000008, 0);
>>  	if (best) {
>>  		int vaddr_bits = (best->eax & 0xff00) >> 8;
>>  
>> @@ -220,7 +237,7 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>>  		vcpu->arch.cpuid_entries[i].padding[2] = 0;
>>  	}
>>  	vcpu->arch.cpuid_nent = cpuid->nent;
>> -	r = kvm_check_cpuid(vcpu);
>> +	r = kvm_check_cpuid(vcpu->arch.cpuid_entries, cpuid->nent);
>>  	if (r) {
>>  		vcpu->arch.cpuid_nent = 0;
>>  		kvfree(cpuid_entries);
>> @@ -250,7 +267,7 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>>  			   cpuid->nent * sizeof(struct kvm_cpuid_entry2)))
>>  		goto out;
>>  	vcpu->arch.cpuid_nent = cpuid->nent;
>> -	r = kvm_check_cpuid(vcpu);
>> +	r = kvm_check_cpuid(vcpu->arch.cpuid_entries, cpuid->nent);
>>  	if (r) {
>>  		vcpu->arch.cpuid_nent = 0;
>>  		goto out;
>> @@ -940,17 +957,8 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>>  struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
>>  					      u32 function, u32 index)
>>  {
>> -	struct kvm_cpuid_entry2 *e;
>> -	int i;
>> -
>> -	for (i = 0; i < vcpu->arch.cpuid_nent; ++i) {
>> -		e = &vcpu->arch.cpuid_entries[i];
>> -
>> -		if (e->function == function && (e->index == index ||
>> -		    !(e->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX)))
>> -			return e;
>> -	}
>> -	return NULL;
>> +	return cpuid_entry2_find(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent,
>> +				 function, index);
>>  }
>>  EXPORT_SYMBOL_GPL(kvm_find_cpuid_entry);
>>  
>
> Other than minor note to the commit message, this looks fine, so
> Reviewed-by: Maxim Levitsky <mlevitsk@...hat.com>
>

Thanks!

-- 
Vitaly