linux-kernel - Re: [PATCH 07/10] 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: <08a44cace7c5da396b5b81415a66eaa1737c2a8b.camel@redhat.com>
Date:   Thu, 05 Oct 2023 15:51:25 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Joerg Roedel <joro@...tes.org>
Cc:     kvm@...r.kernel.org, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/10] KVM: SVM: Inhibit AVIC if ID is too big instead
 of rejecting vCPU creation

У вт, 2023-08-15 у 14:35 -0700, Sean Christopherson пише:
> 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>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  6 ++++++
>  arch/x86/kvm/svm/avic.c         | 16 ++++++++++++++--
>  arch/x86/kvm/svm/svm.h          |  3 ++-
>  3 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 60d430b4650f..4c2d659a1269 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1243,6 +1243,12 @@ enum kvm_apicv_inhibit {
>  	 * mapping between logical ID and vCPU.
>  	 */
>  	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_ID_TOO_BIG,

I prefer APICV_INHIBIT_REASON_PHYSICAL_ID_TOO_BIG

>  };
>  
>  struct kvm_arch {
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index bd81e3517838..522feaa711b4 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -284,9 +284,21 @@ 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.  Do so immediately, i.e. don't defer the update via a
> +	 * request, as avic_vcpu_load() expects to be called if and only if the
> +	 * vCPU has fully initialized AVIC.  Bypass all of the helpers and just
> +	 * clear apicv_active directly, the vCPU isn't reachable and the VMCB
> +	 * isn't even initialized at this point, i.e. there is no possibility
> +	 * of needing to deal with the n

Which helpers do you bypass? I see a call to normal kvm_set_apicv_inhibit() as it should be.

Note that userspace can add vCPUs at will so this can happen any time during 
the guest's lifetime so I don't think that this code can bypass anything.

> +	 */
>  	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_ID_TOO_BIG);
> +		vcpu->arch.apic->apicv_active = false;
> +		return 0;
> +	}
>  
>  	if (!vcpu->arch.apic->regs)
>  		return -EINVAL;
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index a9fde1bb85ee..8b798982e5d0 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -632,7 +632,8 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
>  	BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |	\
>  	BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |	\
>  	BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED) |	\
> -	BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED)	\
> +	BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED) |	\
> +	BIT(APICV_INHIBIT_REASON_ID_TOO_BIG)		\

>  )
>  
>  bool avic_hardware_setup(void);


Best regards,
	Maxim Levitsky