Re: [PATCH v2] KVM: riscv: Free detached G-stage page tables outside mmu_lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] kvm: premature MMU notifier abort during teardown allows use-after-free
- [High] kvm: stale TLB exposed when dropping mmu_lock with pending flushes
--

commit cab834ddbb040506be1fc3bc033afe85ed6f46cc
Author: Can Qi <qican5708@163.com>
Subject: KVM: riscv: Free detached G-stage page tables outside mmu_lock

This commit changes KVM RISC-V to detach the active G-stage root under
mmu_lock and free the page tables after dropping the lock. It also replaces
the KVM-wide split page cache with a per-operation cache, dropping the
lock to top it up.

> diff --git a/arch/riscv/kvm/gstage.c b/arch/riscv/kvm/gstage.c
> index e5002cb9cbef1..5ab3c4ba5a6a4 100644
> --- a/arch/riscv/kvm/gstage.c
> +++ b/arch/riscv/kvm/gstage.c
[ ... ]
> @@ -426,6 +457,12 @@ bool kvm_riscv_gstage_unmap_range(struct kvm_gstage *gstage,
>  	bool flush = false;
>  
>  	while (addr < end) {
> +		/* cond_resched_rwlock_write() may have let teardown detach us. */
> +		if (!(gstage->flags & KVM_GSTAGE_FLAGS_LOCAL) &&
> +		    (!gstage->kvm->arch.pgd ||
> +		     gstage->pgd != gstage->kvm->arch.pgd))
> +			break;
> +

[Severity: Critical]
Can this sequence cause a use-after-free by aborting an MMU notifier
prematurely?

If an in-progress kvm_riscv_gstage_unmap_range() loop yields and observes the
detached root here, it breaks out early and fails to complete the unmap
operation requested by the MMU notifier.

Since kvm_riscv_mmu_free_pgd() drops the mmu_lock before the vCPUs are
quiesced, the host MM could trigger an MMU notifier (e.g., page reclaim)
during this window. If the unmap aborts early, the host MM might free the
page while the guest is still executing and has it mapped.

>  		found_leaf = kvm_riscv_gstage_get_leaf(gstage, addr, &ptep, &ptep_level);
>  		ret = gstage_level_to_page_size(gstage, ptep_level, &page_size);
>  		if (ret)
> diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c
> index 6035b5ec95039..2d7ad70ff7654 100644
> --- a/arch/riscv/kvm/mmu.c
> +++ b/arch/riscv/kvm/mmu.c
[ ... ]
> @@ -141,19 +139,22 @@ static bool mmu_split_huge_pages(struct kvm_gstage *gstage,
>  			if (ret) {
>  				kvm_err("Failed to toup split page cache\n");
>  				write_lock(&kvm->mmu_lock);
> -				return flush;
> +				break;
>  			}
>  			write_lock(&kvm->mmu_lock);
>  		}
>  
> -		if (!kvm->arch.pgd)
> -			return flush;
> +		if (!kvm->arch.pgd || gstage->pgd != kvm->arch.pgd)
> +			break;
>  
>  		flush |= kvm_riscv_gstage_split_huge(gstage, pcache, addr, 0, false);
>  
>  		addr += PMD_SIZE;
>  	}
>  
> +	write_unlock(&kvm->mmu_lock);
> +	kvm_mmu_free_memory_cache(pcache);
> +	write_lock(&kvm->mmu_lock);
>  	return flush;
>  }

[Severity: High]
Does this code expose stale hardware translations to the guest?

If flush is true at the end of the loop, the mmu_lock is dropped to free the
memory cache before returning the flush status to the caller. This creates a
window where the mmu_lock is dropped, but the accumulated split pages have not
yet been flushed from the TLB.

Could a guest trigger huge page splits and access memory during this window,
retaining a stale mapping in the hardware TLB while the host MM subsystem
believes the page is unmapped?

[ ... ]
> @@ -772,41 +768,45 @@ int kvm_riscv_mmu_alloc_pgd(struct kvm *kvm)
>  void kvm_riscv_mmu_free_pgd(struct kvm *kvm)
>  {
>  	struct kvm_gstage gstage;
> -	void *pgd = NULL;
> -	bool flush = false;
>  
>  	write_lock(&kvm->mmu_lock);
> -	if (kvm->arch.pgd) {
> -		kvm_riscv_gstage_init(&gstage, kvm);
> -		flush = kvm_riscv_gstage_unmap_range(&gstage, 0UL,
> -			kvm_riscv_gstage_gpa_size(kvm->arch.pgd_levels), false);
> -		pgd = READ_ONCE(kvm->arch.pgd);
> -		kvm->arch.pgd = NULL;
> -		kvm->arch.pgd_phys = 0;
> -		kvm->arch.pgd_levels = 0;
> +	if (!kvm_riscv_gstage_init(&gstage, kvm)) {
> +		write_unlock(&kvm->mmu_lock);
> +		return;
>  	}
> +	/* Live walkers must acquire mmu_lock and check the active root. */
> +	WRITE_ONCE(kvm->arch.pgd, NULL);
> +	kvm->arch.pgd_phys = 0;
> +	kvm->arch.pgd_levels = 0;
>  	write_unlock(&kvm->mmu_lock);
>  
> -	if (flush)
> -		kvm_flush_remote_tlbs(kvm);
> -
> -	if (pgd)
> -		free_pages((unsigned long)pgd, get_order(kvm_riscv_gstage_pgd_size));
> +	/* Quiesce hardware users before freeing the detached page tables. */
> +	kvm_make_all_cpus_request(kvm, KVM_REQ_OUTSIDE_GUEST_MODE);

[Severity: Critical]
Does this code allow vCPUs to continue executing with a detached page table?

By setting kvm->arch.pgd to NULL and dropping the mmu_lock before
kvm_make_all_cpus_request() successfully kicks all vCPUs out of guest mode,
there is a window where vCPUs can still access memory.

If the host MM triggers an MMU notifier during this window,
kvm_unmap_gfn_range() could fail to initialize because kvm->arch.pgd is NULL,
silently returning without unmapping. This could lead to a host use-after-free
if the host MM frees the page while the guest is still executing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920140831.75097-1-qican5708@163.com?part=1