Re: [PATCH 17/20] KVM: arm64: Extend pKVM page ownership selftests to cover guest block split

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] __pkvm_host_split_guest fails to roll back host stage-2 metadata mutations if the subsequent guest stage-2 split fails, permanently desynchronizing EL2 metadata.
- [High] __pkvm_host_split_guest does not verify that the guest page is exclusively owned before splitting, allowing the host to crash EL2.
--

--- Patch [17]: [PATCH 17/20] KVM: arm64: Extend pKVM page ownership selftests to cover guest block split ---
commit 44307942b6acd5f1f5aeff005f1df6122f7a4e1e
Author: Vincent Donnefort <[email protected]>

KVM: arm64: Extend pKVM page ownership selftests to cover guest block split

This commit extends the pKVM page ownership selftests to cover the split
of a guest stage-2 block. It also adds tests for the per-page reclaim of
that same block.

> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index 71ebd1856cb0c..e22a7991f8ec7 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -2070,6 +2070,38 @@ int __pkvm_ownership_selftest(u64 pfn, u64 nr_pages, u64 pgtable_pfn,
>  	assert_transition_res(-EPERM,	__pkvm_host_donate_guest, pfn, gfn, nr_pages, vcpu);
>  	assert_transition_res(-EPERM,	__pkvm_host_share_guest, pfn, gfn, nr_pages, vcpu, prot);
>  
> +	if (size == PMD_SIZE) {
> +		/* [gfn, gfn + nr_pages) was poisoned. Skip it */
> +		gfn += nr_pages;
> +
> +		selftest_state.host = PKVM_NOPAGE;
> +		selftest_state.guest[1] = PKVM_PAGE_OWNED;
> +		assert_transition_res(0,	__pkvm_host_donate_guest, pfn, gfn, nr_pages, vcpu);
> +		assert_transition_res(0,	__pkvm_host_split_guest, gfn, nr_pages, vcpu);

[Severity: High]
Is a rollback missing in __pkvm_host_split_guest() if the guest stage-2 split
fails?

Looking at __pkvm_host_split_guest(), if kvm_pgtable_stage2_split() fails
(e.g., due to -ENOMEM from an empty memcache), the function returns without
undoing the host_mmu.pgt mutation performed by host_stage2_split_gfn_meta():

    ret = host_stage2_split_gfn_meta(phys, ipa, size, vm);
    if (ret)
        goto unlock;

    ret = kvm_pgtable_stage2_split(&vm->pgt, ipa, size, mc);

    unlock:

This would leave the host tracking split into PAGE_SIZE annotations while the
guest stage-2 retains an unsplit PMD block.

Since host_stage2_split_gfn_meta() fails if the host block is already split,
any future attempt to split the block would fail with -EINVAL, permanently
desynchronizing EL2 metadata.

[Severity: High]
Can a guest page that is not exclusively owned be split by
__pkvm_host_split_guest()?

If the host issues the __pkvm_host_split_guest HVC for a guest page that is
currently shared with the host (guest PTE state is PKVM_PAGE_SHARED_OWNED):

    ret = get_valid_guest_pte(vm, ipa, size, &pte, &phys);
    if (ret)
        goto unlock;

    ret = host_stage2_split_gfn_meta(phys, ipa, size, vm);

get_valid_guest_pte() only verifies that the PTE is valid, so it succeeds
without checking the actual ownership state.

Then, host_stage2_split_gfn_meta() reads the host stage-2 leaf PTE (which is
valid because the page is shared and ID mapped) and passes it to
host_stage2_decode_gfn_meta().

Could this trigger WARN_ON(kvm_pte_valid(pte)), crashing the hypervisor and
allowing the host to take down EL2?

> +
> +		assert_transition_res(-E2BIG, __pkvm_host_split_guest, gfn, nr_pages, vcpu);
[ ... ]

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