Re: [PATCH 01/20] KVM: arm64: Prefault host stage-2 entries on block split

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing page table reference count increments for prefaulted PTEs leads to a Use-After-Free of the page table and hypervisor panics.
- [Critical] Early return on invalid PTEs silently drops pKVM page ownership metadata, reverting guest memory to host ownership.
--

--- Patch [1]: [PATCH 01/20] KVM: arm64: Prefault host stage-2 entries on block split ---
commit ddbd017ec6a553952eac0f275c98da410f8a6cdb
Author: Keir Fraser <[email protected]>

KVM: arm64: Prefault host stage-2 entries on block split

For the host stage-2 that is idmap, we can avoid lazy-mapping on block
splitting by propagate existing valid mappings to neighbouring entries
of the new table.

> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index b74dd5ce1efd3..34f78a58cded0 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1030,10 +1030,39 @@ static int stage2_map_walk_table_pre(const struct kvm_pgtable_visit_ctx *ctx,
>  	return 0;
>  }
>  
> +static void stage2_map_prefault_idmap(const struct kvm_pgtable_visit_ctx *ctx, kvm_pte_t *ptep)
> +{
> +	kvm_pte_t block_pte = ctx->old;
> +	u64 pa;
> +	int i;
> +
> +	if (!kvm_pte_valid(block_pte))
> +		return;

[Severity: Critical]
Does this early return risk dropping pKVM page ownership metadata?

In pKVM, host stage-2 page tables encode page ownership and sharing metadata
using invalid but "counted" PTEs. If a block mapping containing such metadata
needs to be split, returning early here skips populating the child table for
these PTEs. The child table entries would remain zero-filled, which in pKVM
denotes unannotated host-owned memory, potentially allowing the host to
re-acquire guest memory.

> +
> +	pa = ALIGN_DOWN(ctx->addr, kvm_granule_size(ctx->level));
> +	for (i = 0; i < PTRS_PER_PTE; ++i, ++ptep, pa += kvm_granule_size(ctx->level + 1)) {
> +		kvm_pte_t pte = kvm_init_valid_leaf_pte(pa, block_pte, ctx->level + 1);
> +
> +		/*
> +		 * Skip ptes in the range being modified by the caller if we're
> +		 * installing last level entries. Otherwise, we need to
> +		 * temporarily put in a valid mapping to make sure the
> +		 * prefaulting logic is triggered on the next
> +		 * stage2_map_walk_leaf(). This adds an unnecessary TLBI as
> +		 * we'll presumably re-break the freshly installed block, but
> +		 * that should happen very infrequently.
> +		 */
> +		if ((ctx->level < (KVM_PGTABLE_LAST_LEVEL - 1)) ||
> +		    (pa < ctx->addr) || (pa >= ctx->end))
> +			*ptep = pte;

[Severity: Critical]
Could this lead to a premature freeing of the child page table?

When pre-populating the new child table with valid (and therefore "counted")
PTEs, the refcount of the newly allocated child page table is not incremented
via ctx->mm_ops->get_page(ptep).

Since the refcount remains at 1 regardless of the number of valid PTEs
written here, any subsequent unmap or break of one of these PTEs will drop
the refcount to 0. This could cause the EL2 buddy allocator to free the page
table while it is still linked and in use by the hardware MMU.

> +	}
> +}

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