Re: [RFC PATCH v2 24/25] KVM: nSVM: Use different ASIDs for L1 and L2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] Failure to reset `svm->nested.last_asid` during nested SVM teardown allows L2 guests to run with stale TLB entries from recycled ASIDs.

Pre-existing issues:
- [Critical] Dangling pointer `svm->nested.vmcb02.ptr` leaked on the `svm_allocate_nested()` error path leads to a Use-After-Free write during TLB flushes.
--

commit 0fdb61625f72b8737a6f7fe184ab78e2c15d5576
Author: Yosry Ahmed <yosry@kernel.org>
KVM: nSVM: Use different ASIDs for L1 and L2

This patch modifies nested SVM to allocate a separate hardware ASID for L2
for each vCPU. It drops the unconditional TLB flushes and MMU syncs on nested
transitions, replacing them with proper tracking for L1 vs L2 ASIDs.

Link: https://lore.kernel.org/kvm/20231109180646.2963718-1-khorenko@virtuozzo.com/

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index a226aca8f9108..56b769b603ca9 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c

[ ... ]

> @@ -1539,7 +1532,7 @@ int svm_allocate_nested(struct vcpu_svm *svm)
>  	if (!svm->nested.msrpm)
>  		goto err_free_vmcb02;
>  
> -	svm->nested.asid02 = svm->asid;
> +	svm->nested.asid02 = allocate_asid();
>  
>  	svm->nested.initialized = true;
>  	return 0;
>  
>  err_free_vmcb02:
>  	__free_page(vmcb02_page);
>  	return -ENOMEM;
>  }

[Severity: Critical]
This isn't a bug introduced by this patch, but does the error path here leave
a dangling pointer in svm->nested.vmcb02.ptr?

When msrpm allocation fails, the vmcb02_page is freed, but
svm->nested.vmcb02.ptr is not explicitly cleared. If a subsequent TLB flush
occurs on this vCPU, for example via MMU notifiers:

svm_flush_tlb_all() {
    ...
    if (svm->nested.vmcb02.ptr)
        vmcb_set_flush_asid(svm->nested.vmcb02.ptr);
}

Could this result in a use-after-free write that corrupts the freed memory?

[ ... ]

> @@ -1557,6 +1550,8 @@ void svm_free_nested(struct vcpu_svm *svm)
>  	if (WARN_ON_ONCE(svm->vmcb != svm->vmcb01.ptr))
>  		svm_switch_vmcb(svm, &svm->vmcb01);
>  
> +	free_asid(svm->nested.asid02);
> +

[Severity: Critical]
When the hardware ASID is freed here during nested SVM teardown, does
svm->nested.last_asid also need to be reset to 0?

If L1 disables SVM, svm_free_nested() is called, freeing asid02. This ASID tag
can then be reallocated. If L1 later re-enables SVM and uses the same L1-managed
ASID, nested_svm_entry_tlb_flush() will see:

nested_svm_entry_tlb_flush() {
    ...
    if (svm->nested.ctl.asid != svm->nested.last_asid) {
        svm->nested.last_asid = svm->nested.ctl.asid;
        new_asid = true;
    }
    ...
}

This check would fail to set new_asid = true. Since this patch removes the
unconditional TLB flush on nested entry, would this regression allow the L2
guest to run with stale TLB entries from the previous owner of the reallocated
hardware ASID?

>  	svm_vcpu_free_msrpm(svm->nested.msrpm);
>  	svm->nested.msrpm = NULL;
>

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