linux-kernel - Re: [PATCH v2] KVM: x86: Update KVM_GET

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] [day] [month] [year] [list]

Message-ID: <ZHEVrowZsi2j6dVI@google.com>
Date:   Fri, 26 May 2023 13:25:18 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Takahiro Itazuri <itazur@...zon.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        linux-kernel@...r.kernel.org, Takahiro Itazuri <zulinx86@...il.com>
Subject: Re: [PATCH v2] KVM: x86: Update KVM_GET_CPUID2 to return valid entry count

On Thu, Apr 13, 2023, Takahiro Itazuri wrote:
> Modify the KVM_GET_CPUID2 API to return the number of valid entries in
> nent field of kvm_cpuid2 even on success.
> 
> Previously, the KVM_GET_CPUID2 API only updated the nent field when an
> error was returned. If the API was called with an entry count larger
> than necessary (e.g., KVM_MAX_CPUID_ENTRIES), it would succeed, but the
> nent field would continue to show a value larger than the actual number
> of entries filled by the KVM_GET_CPUID2 API. With this change, users can
> rely on the updated nent field and there is no need to traverse
> unnecessary entries and check whether an entry is valid or not.
> 
> Suggested-by: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Takahiro Itazuri <itazur@...zon.com>
> ---
> Changes
> v1 -> v2
> * Capitalize "kvm" in the commit title.
> * Remove a scratch "nent" variable.
> * Link to v1: https://lore.kernel.org/all/20230410141820.57328-1-itazur@amazon.com/
> 
>  arch/x86/kvm/cpuid.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 599aebec2d52..20d28ebdc672 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -523,18 +523,18 @@ int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
>  			      struct kvm_cpuid2 *cpuid,
>  			      struct kvm_cpuid_entry2 __user *entries)
>  {
> -	int r;
> +	int r = 0;
>  
> -	r = -E2BIG;
>  	if (cpuid->nent < vcpu->arch.cpuid_nent)
> -		goto out;
> -	r = -EFAULT;
> -	if (copy_to_user(entries, vcpu->arch.cpuid_entries,
> +		r = -E2BIG;
> +	else if (copy_to_user(entries, vcpu->arch.cpuid_entries,
>  			 vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
> -		goto out;
> -	return 0;
> +		r = -EFAULT;
>  
> -out:
> +	/*
> +	 * Update "nent" even on failure, e.g. so that userspace can fix an
> +	 * -E2BIG issue by allocating a larger array.
> +	 */

Gah, this is wrong.  The caller only copies @cpuid to userspace on success.  My
fault for not seeing this in v1, but this is exactly why we write tests.

I've written a test and a fix, I'll post 'em shortly.