Re: [PATCH v5 1/5] RISC-V: KVM: Add the split page cache for ioctl context

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Incomplete Implementation / Dead Code: `pgd_split_page_cache` is never used.

Pre-existing issues:
- [Critical] Use-After-Free of PGD in `kvm_riscv_mmu_map` concurrent with `kvm_mmu_notifier_release`.
- [High] NULL Pointer Dereference when ioctls are invoked after `kvm_mmu_notifier_release` clears the PGD.
- [Medium] Memory leak of Guest Stage PGD on VM creation failure path.
--

--- Patch [1]: [PATCH v5 1/5] RISC-V: KVM: Add the split page cache for ioctl context ---
commit 721380c0ab0188aeba8fad4993d1a8094c00948b
Author: Wang Yechao <wang.yechao255@zte.com.cn>
Subject: RISC-V: KVM: Add the split page cache for ioctl context

This patch adds a split page cache for dirty logging enablement and the
KVM_CLEAR_DIRTY_LOG ioctl. It introduces and initializes a new
pgd_split_page_cache in the kvm_arch structure.

> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index e2d5808169e44..71e455d166905 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
> @@ -86,6 +86,7 @@ struct kvm_arch {
>  	pgd_t *pgd;
>  	phys_addr_t pgd_phys;
>  	unsigned long pgd_levels;
> +	struct kvm_mmu_memory_cache pgd_split_page_cache;

[Severity: Low]
Is this pgd_split_page_cache actually utilized anywhere? I see it initialized
in kvm_riscv_mmu_alloc_pgd() and freed in kvm_riscv_mmu_free_pgd(), but there
do not seem to be any calls to populate or use this cache in the KVM RISC-V
code. Does this result in dead code?

>  
>  	/* Guest Timer */
>  	struct kvm_guest_timer timer;
> diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c
> index 8a0aa5e0e216e..2017d292f4ca7 100644
> --- a/arch/riscv/kvm/mmu.c
> +++ b/arch/riscv/kvm/mmu.c
[ ... ]
> @@ -704,6 +705,8 @@ void kvm_riscv_mmu_free_pgd(struct kvm *kvm)
>  
>  	if (pgd)
>  		free_pages((unsigned long)pgd, get_order(kvm_riscv_gstage_pgd_size));
> +
> +	kvm_mmu_free_memory_cache(&kvm->arch.pgd_split_page_cache);
>  }

[Severity: Critical]
This is a pre-existing issue, but can we hit a use-after-free of the PGD memory
here during concurrent execution?

If one process exits, it triggers kvm_mmu_notifier_release() which frees
kvm->arch.pgd via this path. However, kvm_mmu_notifier_release() does not
increment kvm->mmu_invalidate_seq:

virt/kvm/kvm_main.c:kvm_mmu_notifier_release() {
    idx = srcu_read_lock(&kvm->srcu);
    kvm_flush_shadow_all(kvm);
    srcu_read_unlock(&kvm->srcu, idx);
}

Simultaneously, if another process sharing the KVM fd triggers a page fault,
kvm_riscv_mmu_map() caches kvm->arch.pgd locklessly before checking the MMU
sequence number:

arch/riscv/kvm/mmu.c:kvm_riscv_mmu_map() {
    kvm_riscv_gstage_init(&gstage, kvm);
    ...
    mmu_seq = kvm->mmu_invalidate_seq;
    ...
    write_lock(&kvm->mmu_lock);
    if (mmu_invalidate_retry(kvm, mmu_seq))
        goto out_unlock;
    ...
    ret = kvm_riscv_gstage_map_page(&gstage, pcache, gpa, hfn << PAGE_SHIFT, ...);
}

Since the sequence number isn't incremented, would the retry check pass and
allow the freed PGD to be accessed by the page table walker?

[Severity: High]
This isn't a bug introduced by this patch, but does clearing the PGD here
expose us to a NULL pointer dereference?

If kvm_mmu_notifier_release() runs when the mm_struct exits, it sets
kvm->arch.pgd = NULL. If a concurrent or subsequent ioctl like
KVM_CLEAR_DIRTY_LOG is invoked, kvm_arch_mmu_enable_log_dirty_pt_masked()
caches the potentially NULL kvm->arch.pgd:

arch/riscv/kvm/mmu.c:kvm_arch_mmu_enable_log_dirty_pt_masked() {
    kvm_riscv_gstage_init(&gstage, kvm);
    flush = kvm_riscv_gstage_wp_range(&gstage, start, end);
}

Then kvm_riscv_gstage_get_leaf() calculates a PTE pointer offset relative to
the NULL PGD:

arch/riscv/kvm/gstage.c:kvm_riscv_gstage_get_leaf() {
    ptep = (pte_t *)gstage->pgd;
    ptep = &ptep[gstage_pte_index(gstage, addr, current_level)];
    while (ptep && pte_val(ptep_get(ptep))) {
}

Could this sequence trigger a kernel panic upon dereferencing ptep_get(ptep)?

[Severity: Medium]
This is a pre-existing issue, but does the PGD memory leak on the VM creation
failure path?

If kvm_create_vm() fails after calling kvm_arch_init_vm() but before
registering the MMU notifier (for example, if kvm_init_mmu_notifier() fails):

virt/kvm/kvm_main.c:kvm_create_vm() {
    r = kvm_init_mmu_notifier(kvm);
    if (r)
        goto out_err_no_mmu_notifier;
}

The error path calls kvm_arch_destroy_vm(), but KVM RISC-V's implementation
does not free the PGD:

arch/riscv/kvm/vm.c:kvm_arch_destroy_vm() {
    kvm_destroy_vcpus(kvm);
    kvm_riscv_aia_destroy_vm(kvm);
}

Because the MMU notifier was never registered, the release callback is not
invoked either. Does this lead to a persistent memory leak of the PGD?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731091215.1549430-1-wang.yechao255@zte.com.cn?part=1