Re: [PATCH v2 26/29] KVM: nSVM: Flush L2's ASID when emulating INVLPGA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: host BUG_ON in shadow_walk_init_using_root on nested INVLPGA
--

commit 37c7f867aa6cefc5df5eab301ccc6b7b444c6816
Author: Yosry Ahmed <yosry@kernel.org>
Subject: KVM: nSVM: Flush L2's ASID when emulating INVLPGA

This commit handles INVLPGA differently depending on whether L1 is flushing
its own ASID or a different L2 ASID. If it's a different ASID, the GVA flush is
skipped for the current context, and the L2 ASID is flushed in hardware or
via VMCB fallback.

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index f37cba4f34c2b..4b95a05c37f60 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2468,17 +2468,51 @@ static int clgi_interception(struct kvm_vcpu *vcpu)
>  
[ ... ]
>  static int invlpga_interception(struct kvm_vcpu *vcpu)
>  {
> +	struct vcpu_svm *svm = to_svm(vcpu);
>  	/* FIXME: Handle an address size prefix. */
>  	gva_t gva = kvm_rax_read(vcpu);
>  	u32 asid = kvm_ecx_read(vcpu);
> +	int cpu;
>  
>  	if (nested_svm_check_permissions(vcpu))
>  		return 1;
>  
>  	trace_kvm_invlpga(to_svm(vcpu)->vmcb->save.rip, asid, gva);
>  
> -	/* Let's treat INVLPGA the same as INVLPG (can be optimized!) */
> -	kvm_mmu_invlpg(vcpu, gva);
> +	/*
> +	 * INVLPG on a non-canonical address is a NOP according to the SDM,
> +	 * assume the same behavior from INVLPGA since the APM doesn't specify.
> +	 */
> +	if (is_noncanonical_invlpg_address(gva, vcpu))
> +		return kvm_skip_emulated_instruction(vcpu);
> +
> +	/*
> +	 * Do nothing if L1 is flushing a different L2 ASID than the one KVM is
> +	 * currently tracking.  KVM tracks a single L2 ASID, and performs a TLB
> +	 * flush (and MMU resync if needed) when L1 switches ASIDs anyway.
> +	 */
> +	if (asid && asid != svm->nested.last_asid)
> +		return kvm_skip_emulated_instruction(vcpu);
> +
> +	/*
> +	 * Handle INVLPGA similar to INVLPG, with one caveat. If the specified
> +	 * ASID is non-zero (i.e. L1 is not flushing its own ASID), skip
> +	 * flushing the TLB for the current context (L1's), and use INVLPGA to
> +	 * flush L2's ASID in hardware if running on the same CPU (otherwise
> +	 * fallback to a full ASID flush).
> +	 *
> +	 * Note, if NPT is disabled, this will sync all the shadow page tables.
> +	 * This can be optimized by keying off guest_mode.
> +	 */
> +	__kvm_mmu_invlpg(vcpu, gva, !asid);

[Severity: High]
Does this code trigger a host kernel panic when a 64-bit L1 guest switches to
a 32-bit PAE L2 guest?

When KVM uses shadow paging (e.g. nested NPT disabled), L1 and L2 share the
same MMU context (vcpu->arch.root_mmu).  If a 64-bit L1 guest switches to a
32-bit PAE L2 guest, KVM's fast_pgd_switch() will cache the old 64-bit root in
vcpu->arch.mmu->prev_roots.

If L2 then executes INVLPGA, or L1 intercepts it and KVM emulates it, we call
into __kvm_mmu_invlpg():

arch/x86/kvm/mmu/mmu.c:__kvm_mmu_invlpg() {
    ...
	__kvm_mmu_invalidate_addr(vcpu, &vcpu->arch.gva_walk, gva,
				  KVM_MMU_ROOTS_ALL, flush_gva);
}

Which blindly iterates over all prev_roots:

arch/x86/kvm/mmu/mmu.c:__kvm_mmu_invalidate_addr() {
    ...
	for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++) {
		if (roots & KVM_MMU_ROOT_PREVIOUS(i))
			kvm_mmu_sync_addr(vcpu, mmu, addr, mmu->prev_roots[i].hpa);
	}
}

kvm_mmu_sync_addr() then calls shadow_walk_init_using_root() via
for_each_shadow_entry_using_root().  Because the active MMU role is PAE,
iterator->level evaluates to PT32E_ROOT_LEVEL (3), which hits a hardcoded
assertion:

arch/x86/kvm/mmu/mmu.c:shadow_walk_init_using_root() {
    ...
	if (iterator->level == PT32E_ROOT_LEVEL) {
		/*
		 * prev_root is currently only used for 64-bit hosts. So only
		 * the active root_hpa is valid here.
		 */
		BUG_ON(root != vcpu->arch.mmu->root.hpa);
    ...
}

Because the 64-bit root from prev_roots does not match the currently active PAE
root, it appears this will panic the host, resulting in a Denial of Service
directly triggerable by a nested guest.

> +	if (asid) {
> +		cpu = get_cpu();
> +		if (cpu == svm->nested.vmcb02.cpu)
> +			invlpga(gva, svm->nested.asid02);
> +		else
> +			vmcb_set_flush_asid(svm->nested.vmcb02.ptr);
> +		put_cpu();
> +	}
>  
>  	return kvm_skip_emulated_instruction(vcpu);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008001425.2458927-1-yosry@kernel.org?part=26