linux-kernel - Re: [PATCH 17/67] KVM: SVM: Drop redundant check in AVIC code on ID during 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: <a1f32fa7-2946-4efa-be74-cfb5c7d5dd22@amd.com>
Date: Tue, 15 Apr 2025 16:46:11 +0530
From: Sairaj Kodilkar <sarunkod@....com>
To: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
	<pbonzini@...hat.com>, Joerg Roedel <joro@...tes.org>, David Woodhouse
	<dwmw2@...radead.org>, Lu Baolu <baolu.lu@...ux.intel.com>
CC: <kvm@...r.kernel.org>, <iommu@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, Maxim Levitsky <mlevitsk@...hat.com>, Joao
 Martins <joao.m.martins@...cle.com>, David Matlack <dmatlack@...gle.com>
Subject: Re: [PATCH 17/67] KVM: SVM: Drop redundant check in AVIC code on ID
 during vCPU creation

On 4/5/2025 1:08 AM, Sean Christopherson wrote:
> Drop avic_get_physical_id_entry()'s compatibility check on the incoming
> ID, as its sole caller, avic_init_backing_page(), performs the exact same
> check.  Drop avic_get_physical_id_entry() entirely as the only remaining
> functionality is getting the address of the Physical ID table, and
> accessing the array without an immediate bounds check is kludgy.
> 
> Opportunistically add a compile-time assertion to ensure the vcpu_id can't
> result in a bounds overflow, e.g. if KVM (really) messed up a maximum
> physical ID #define, as well as run-time assertions so that a NULL pointer
> dereference is morphed into a safer WARN().
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>   arch/x86/kvm/svm/avic.c | 47 +++++++++++++++++------------------------
>   1 file changed, 19 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index ba8dfc8a12f4..344541e418c3 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -265,35 +265,19 @@ void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb)
>   		avic_deactivate_vmcb(svm);
>   }
>   
> -static u64 *avic_get_physical_id_entry(struct kvm_vcpu *vcpu,
> -				       unsigned int index)
> -{
> -	u64 *avic_physical_id_table;
> -	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
> -
> -	if ((!x2avic_enabled && index > AVIC_MAX_PHYSICAL_ID) ||
> -	    (index > X2AVIC_MAX_PHYSICAL_ID))
> -		return NULL;
> -
> -	avic_physical_id_table = page_address(kvm_svm->avic_physical_id_table_page);
> -
> -	return &avic_physical_id_table[index];
> -}
> -
>   static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>   {
> -	u64 *entry, new_entry;
> -	int id = vcpu->vcpu_id;
> +	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
>   	struct vcpu_svm *svm = to_svm(vcpu);
> +	u32 id = vcpu->vcpu_id;
> +	u64 *table, new_entry;
>   
>   	/*
>   	 * 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.  Immediately clear apicv_active,
> -	 * as avic_vcpu_load() assumes avic_physical_id_cache is valid, i.e.
> -	 * waiting until KVM_REQ_APICV_UPDATE is processed on the first KVM_RUN
> -	 * will result in an NULL pointer deference when loading the vCPU.
> +	 * 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.
>   	 */

Hi Sean,
I think above change in the comment belongs to patch 16.

Regards
Sairaj
>   	if ((!x2avic_enabled && id > AVIC_MAX_PHYSICAL_ID) ||
>   	    (id > X2AVIC_MAX_PHYSICAL_ID)) {
> @@ -302,6 +286,9 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>   		return 0;
>   	}
>   
> +	BUILD_BUG_ON((AVIC_MAX_PHYSICAL_ID + 1) * sizeof(*table) > PAGE_SIZE ||
> +		     (X2AVIC_MAX_PHYSICAL_ID + 1) * sizeof(*table) > PAGE_SIZE);
> +
>   	if (WARN_ON_ONCE(!vcpu->arch.apic->regs))
>   		return -EINVAL;
>   
> @@ -320,9 +307,7 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>   	}
>   
>   	/* Setting AVIC backing page address in the phy APIC ID table */
> -	entry = avic_get_physical_id_entry(vcpu, id);
> -	if (!entry)
> -		return -EINVAL;
> +	table = page_address(kvm_svm->avic_physical_id_table_page);
>   
>   	/* Note, fls64() returns the bit position, +1. */
>   	BUILD_BUG_ON(__PHYSICAL_MASK_SHIFT >
> @@ -330,9 +315,9 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>   
>   	new_entry = avic_get_backing_page_address(svm) |
>   		    AVIC_PHYSICAL_ID_ENTRY_VALID_MASK;
> -	WRITE_ONCE(*entry, new_entry);
> +	WRITE_ONCE(table[id], new_entry);
>   
> -	svm->avic_physical_id_cache = entry;
> +	svm->avic_physical_id_cache = &table[id];
>   
>   	return 0;
>   }
> @@ -1018,6 +1003,9 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   	if (WARN_ON(h_physical_id & ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK))
>   		return;
>   
> +	if (WARN_ON_ONCE(!svm->avic_physical_id_cache))
> +		return;
> +
>   	/*
>   	 * No need to update anything if the vCPU is blocking, i.e. if the vCPU
>   	 * is being scheduled in after being preempted.  The CPU entries in the
> @@ -1058,6 +1046,9 @@ void avic_vcpu_put(struct kvm_vcpu *vcpu)
>   
>   	lockdep_assert_preemption_disabled();
>   
> +	if (WARN_ON_ONCE(!svm->avic_physical_id_cache))
> +		return;
> +
>   	/*
>   	 * Note, reading the Physical ID entry outside of ir_list_lock is safe
>   	 * as only the pCPU that has loaded (or is loading) the vCPU is allowed