Re: [PATCH v4 05/17] KVM: arm64: Allow kvm_hyp_memcache usage outside of stage-2

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

Pre-existing issues:
- [High] In pKVM, `user_mem_abort()` skips memcache topup during permission faults, leading to a NULL pointer dereference in `pkvm_pgtable_stage2_map()` if the fault triggers a THP block mapping upgrade.
--

--- Patch [5]: [PATCH v4 05/17] KVM: arm64: Allow kvm_hyp_memcache usage outside of stage-2 ---
commit 005835cc03d158de34bf975706ada05a2388636e
Author: Vincent Donnefort <[email protected]>

KVM: arm64: Allow kvm_hyp_memcache usage outside of stage-2

This commit introduces init_hyp_memcache() and init_hyp_stage2_memcache()
to initialize generic and stage-2 specific memcaches. It modifies the
allocator to use GFP_KERNEL_ACCOUNT conditionally based on the new KMEMCG
flag.

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 6c941aaa10c63..1e37f2f56c680 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
[ ... ]
> @@ -1160,8 +1160,10 @@ static void *hyp_mc_alloc_fn(void *mc)
>  {
>  	struct kvm_hyp_memcache *memcache = mc;
>  	void *addr;
> +	gfp_t gfp;
>  
> -	addr = (void *)__get_free_page(GFP_KERNEL_ACCOUNT);
> +	gfp = memcache->flags & HYP_MEMCACHE_ACCOUNT_KMEMCG ? GFP_KERNEL_ACCOUNT : GFP_KERNEL;
> +	addr = (void *)__get_free_page(gfp);

[Severity: High]
This is a pre-existing issue, but while reviewing these memcache changes,
I noticed a potential NULL pointer dereference related to the memcache topup
logic across user_mem_abort() and pkvm_pgtable_stage2_map().

In arch/arm64/kvm/mmu.c:user_mem_abort(), topup_mmu_memcache() is skipped
during a permission fault:

	memcache = get_mmu_memcache(s2fd->vcpu);
	if (!perm_fault || (memslot_is_logging(s2fd->memslot) &&
			    kvm_is_write_fault(s2fd->vcpu))) {
		ret = topup_mmu_memcache(s2fd->vcpu, memcache);
		if (ret)
			return ret;
	}

If transparent_hugepage_adjust() then detects a THP and upgrades the mapping
to PMD_SIZE, pkvm_pgtable_stage2_map() will be called to install the block
mapping.

Since the memcache wasn't topped up, cache->mapping is NULL, and this code
subsequently dereferences it in arch/arm64/kvm/pkvm.c:pkvm_pgtable_stage2_map():

	swap(mapping, cache->mapping);
	mapping->gfn = gfn;

Could a guest trigger this by forcing a permission fault on a page that
khugepaged has collapsed into a THP, leading to a host kernel panic?

>  	if (addr && memcache->flags & HYP_MEMCACHE_ACCOUNT_STAGE2)
>  		kvm_account_pgtable_pages(addr, 1);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5