lkml.org 
[lkml]   [2021]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH MANUALSEL 5.15 6/9] KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN
From
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

\
 
 \ /
  Last update: 2021-12-13 15:28    [W:0.160 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site