Re: [PATCH MANUALSEL 5.15 6/9] KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/13/21 15:19, Sasha Levin wrote:
> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> [ Upstream commit feb627e8d6f69c9a319fe279710959efb3eba873 ]
> 
> Commit 63f5a1909f9e ("KVM: x86: Alert userspace that KVM_SET_CPUID{,2}
> after KVM_RUN is broken") officially deprecated KVM_SET_CPUID{,2} ioctls
> after first successful KVM_RUN and promissed to make this sequence forbiden
> in 5.16. It's time to fulfil the promise.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Message-Id: <20211122175818.608220-3-vkuznets@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>   arch/x86/kvm/mmu/mmu.c | 28 +++++++++++-----------------
>   arch/x86/kvm/x86.c     | 19 +++++++++++++++++++
>   2 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 0a88cb4f731f4..31392e492ce5e 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5022,6 +5022,14 @@ void kvm_mmu_after_set_cpuid(struct kvm_vcpu *vcpu)
>   	/*
>   	 * Invalidate all MMU roles to force them to reinitialize as CPUID
>   	 * information is factored into reserved bit calculations.
> +	 *
> +	 * Correctly handling multiple vCPU models with respect to paging and
> +	 * physical address properties) in a single VM would require tracking
> +	 * all relevant CPUID information in kvm_mmu_page_role. That is very
> +	 * undesirable as it would increase the memory requirements for
> +	 * gfn_track (see struct kvm_mmu_page_role comments).  For now that
> +	 * problem is swept under the rug; KVM's CPUID API is horrific and
> +	 * it's all but impossible to solve it without introducing a new API.
>   	 */
>   	vcpu->arch.root_mmu.mmu_role.ext.valid = 0;
>   	vcpu->arch.guest_mmu.mmu_role.ext.valid = 0;
> @@ -5029,24 +5037,10 @@ void kvm_mmu_after_set_cpuid(struct kvm_vcpu *vcpu)
>   	kvm_mmu_reset_context(vcpu);
>   
>   	/*
> -	 * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
> -	 * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't
> -	 * tracked in kvm_mmu_page_role.  As a result, KVM may miss guest page
> -	 * faults due to reusing SPs/SPTEs.  Alert userspace, but otherwise
> -	 * sweep the problem under the rug.
> -	 *
> -	 * KVM's horrific CPUID ABI makes the problem all but impossible to
> -	 * solve, as correctly handling multiple vCPU models (with respect to
> -	 * paging and physical address properties) in a single VM would require
> -	 * tracking all relevant CPUID information in kvm_mmu_page_role.  That
> -	 * is very undesirable as it would double the memory requirements for
> -	 * gfn_track (see struct kvm_mmu_page_role comments), and in practice
> -	 * no sane VMM mucks with the core vCPU model on the fly.
> +	 * Changing guest CPUID after KVM_RUN is forbidden, see the comment in
> +	 * kvm_arch_vcpu_ioctl().
>   	 */
> -	if (vcpu->arch.last_vmentry_cpu != -1) {
> -		pr_warn_ratelimited("KVM: KVM_SET_CPUID{,2} after KVM_RUN may cause guest instability\n");
> -		pr_warn_ratelimited("KVM: KVM_SET_CPUID{,2} will fail after KVM_RUN starting with Linux 5.16\n");
> -	}
> +	KVM_BUG_ON(vcpu->arch.last_vmentry_cpu != -1, vcpu->kvm);
>   }
>   
>   void kvm_mmu_reset_context(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b7aa845f7beee..2a584070833f9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5088,6 +5088,17 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>   		struct kvm_cpuid __user *cpuid_arg = argp;
>   		struct kvm_cpuid cpuid;
>   
> +		/*
> +		 * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
> +		 * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't
> +		 * tracked in kvm_mmu_page_role.  As a result, KVM may miss guest page
> +		 * faults due to reusing SPs/SPTEs.  In practice no sane VMM mucks with
> +		 * the core vCPU model on the fly, so fail.
> +		 */
> +		r = -EINVAL;
> +		if (vcpu->arch.last_vmentry_cpu != -1)
> +			goto out;
> +
>   		r = -EFAULT;
>   		if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
>   			goto out;
> @@ -5098,6 +5109,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>   		struct kvm_cpuid2 __user *cpuid_arg = argp;
>   		struct kvm_cpuid2 cpuid;
>   
> +		/*
> +		 * KVM_SET_CPUID{,2} after KVM_RUN is forbidded, see the comment in
> +		 * KVM_SET_CPUID case above.
> +		 */
> +		r = -EINVAL;
> +		if (vcpu->arch.last_vmentry_cpu != -1)
> +			goto out;
> +
>   		r = -EFAULT;
>   		if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
>   			goto out;
> 

NACK

the error says "starting with Linux 5.16" for a reason... :)

Paolo