linux-kernel - Re: [PATCH v3 12/62] KVM: SVM: Inhibit AVIC if ID is too big instead of rejecting vCPU creation

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: <bmx43lru6mwdyun3ktcmoeezqw6baih7vgtykolsrmu5q2x7vu@xp5jrbbqwzej>
Date: Tue, 17 Jun 2025 19:55:01 +0530
From: Naveen N Rao <naveen@...nel.org>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Marc Zyngier <maz@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>, 
	Paolo Bonzini <pbonzini@...hat.com>, Joerg Roedel <joro@...tes.org>, 
	David Woodhouse <dwmw2@...radead.org>, Lu Baolu <baolu.lu@...ux.intel.com>, 
	linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev, kvm@...r.kernel.org, 
	iommu@...ts.linux.dev, linux-kernel@...r.kernel.org, Sairaj Kodilkar <sarunkod@....com>, 
	Vasant Hegde <vasant.hegde@....com>, Maxim Levitsky <mlevitsk@...hat.com>, 
	Joao Martins <joao.m.martins@...cle.com>, Francesco Lavra <francescolavra.fl@...il.com>, 
	David Matlack <dmatlack@...gle.com>
Subject: Re: [PATCH v3 12/62] KVM: SVM: Inhibit AVIC if ID is too big instead
 of rejecting vCPU creation

On Wed, Jun 11, 2025 at 03:45:15PM -0700, Sean Christopherson wrote:
> Inhibit AVIC with a new "ID too big" flag if userspace creates a vCPU with
> an ID that is too big, but otherwise allow vCPU creation to succeed.
> Rejecting KVM_CREATE_VCPU with EINVAL violates KVM's ABI as KVM advertises
> that the max vCPU ID is 4095, but disallows creating vCPUs with IDs bigger
> than 254 (AVIC) or 511 (x2AVIC).
> 
> Alternatively, KVM could advertise an accurate value depending on which
> AVIC mode is in use, but that wouldn't really solve the underlying problem,
> e.g. would be a breaking change if KVM were to ever try and enable AVIC or
> x2AVIC by default.
> 
> Cc: Maxim Levitsky <mlevitsk@...hat.com>
> Tested-by: Sairaj Kodilkar <sarunkod@....com>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  9 ++++++++-
>  arch/x86/kvm/svm/avic.c         | 14 ++++++++++++--
>  arch/x86/kvm/svm/svm.h          |  3 ++-
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 2a6ef1398da7..a9b709db7c59 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1314,6 +1314,12 @@ enum kvm_apicv_inhibit {
>  	 */
>  	APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED,
>  
> +	/*
> +	 * AVIC is disabled because the vCPU's APIC ID is beyond the max
> +	 * supported by AVIC/x2AVIC, i.e. the vCPU is unaddressable.
> +	 */
> +	APICV_INHIBIT_REASON_PHYSICAL_ID_TOO_BIG,
> +
>  	NR_APICV_INHIBIT_REASONS,
>  };
>  
> @@ -1332,7 +1338,8 @@ enum kvm_apicv_inhibit {
>  	__APICV_INHIBIT_REASON(IRQWIN),			\
>  	__APICV_INHIBIT_REASON(PIT_REINJ),		\
>  	__APICV_INHIBIT_REASON(SEV),			\
> -	__APICV_INHIBIT_REASON(LOGICAL_ID_ALIASED)
> +	__APICV_INHIBIT_REASON(LOGICAL_ID_ALIASED),	\
> +	__APICV_INHIBIT_REASON(PHYSICAL_ID_TOO_BIG)
>  
>  struct kvm_arch {
>  	unsigned long n_used_mmu_pages;
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index ab228872a19b..f0a74b102c57 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -277,9 +277,19 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>  	int id = vcpu->vcpu_id;
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> +	/*
> +	 * Inhibit AVIC if the vCPU ID is bigger than what is supported by AVIC
> +	 * hardware.  Immediately clear apicv_active, i.e. don't wait until the
> +	 * KVM_REQ_APICV_UPDATE request is processed on the first KVM_RUN, as
> +	 * avic_vcpu_load() expects to be called if and only if the vCPU has
> +	 * fully initialized AVIC.
> +	 */
>  	if ((!x2avic_enabled && id > AVIC_MAX_PHYSICAL_ID) ||
> -	    (id > X2AVIC_MAX_PHYSICAL_ID))
> -		return -EINVAL;
> +	    (id > X2AVIC_MAX_PHYSICAL_ID)) {
> +		kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_TOO_BIG);
> +		vcpu->arch.apic->apicv_active = false;

This bothers me a bit. kvm_create_lapic() does this:
          if (enable_apicv) {
                  apic->apicv_active = true;
                  kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
	  }

But, setting apic->apicv_active to false here means KVM_REQ_APICV_UPDATE 
is going to be a no-op.

This does not look to be a big deal given that kvm_create_lapic() itself 
is called just a bit before svm_vcpu_create() (which calls the above 
function through avic_init_vcpu()) in kvm_arch_vcpu_create(), so there 
isn't that much done before apicv_active is toggled.

But, this made me wonder if introducing a kvm_x86_op to check and 
enable/disable apic->apicv_active in kvm_create_lapic() might be cleaner 
overall. Maybe even have it be the initialization point for APICv: 
apicv_init(), so we can invoke avic_init_vcpu() right away?


- Naveen