Re: [PATCH 03/20] KVM: arm64: Allow block-level stage-2 annotation

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Splitting a block-level annotation PTE clones the base GFN to all sub-pages, corrupting metadata for trailing pages. Additionally, multi-page annotations that fallback to page-level entries duplicate the base GFN.
- [High] Allowing block-level stage-2 annotations causes a hypervisor panic during page reclaim due to an unhandled `WARN_ON` at EL2 that expects page-level annotations.
- [Medium] Uninitialized `data->phys` in `kvm_pgtable_stage2_annotate()` causes artificial fragmentation into 4KB pages for unaligned annotation regions.
--

--- Patch [3]: [PATCH 03/20] KVM: arm64: Allow block-level stage-2 annotation ---
commit fa0abe8958af15457c75292a24fd1c05d6574ec0
Author: Vincent Donnefort <[email protected]>

KVM: arm64: Allow block-level stage-2 annotation

On host to guest donation, pKVM annotates the host stage-2 invalid
entries with the GFN. Now that the host stage-2 preserves annotations on
block split, we can allow these annotations at block-level. This enables
safe support for guest stage-2 huge mappings.

> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 0ca157ecee641..c4ebae0544d4e 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c

[ ... ]

> @@ -1178,7 +1175,6 @@ int kvm_pgtable_stage2_annotate(struct kvm_pgtable *pgt, u64 addr, u64 size,
>  	struct stage2_map_data map_data = {
>  		.mmu		= pgt->mmu,
>  		.memcache	= mc,

[Severity: Medium]
Does leaving data->phys uninitialized here cause artificial fragmentation
for unaligned annotation regions?

Since map_data is zero-initialized, map_data.phys defaults to 0. Later,
when stage2_leaf_mapping_allowed() checks if a block mapping is supported,
it calculates:

arch/arm64/kvm/hyp/pgtable.c:stage2_leaf_mapping_allowed() {
    ...
    u64 phys = stage2_map_walker_phys_addr(ctx, data);
    ...
    return kvm_block_mapping_supported(ctx, phys);
}

This evaluates to (0 + (ctx->addr - ctx->start)). The check then verifies
IS_ALIGNED(phys, granule). Because ctx->addr must already be block-aligned,
this effectively forces ctx->start to also be block-aligned.

If a huge annotation request starts at an unaligned address but spans
complete blocks internally, could this cause those internal blocks to be
forcibly mapped at page-granularity, using excess host memory?

> -		.force_pte	= true,

[Severity: High]
By removing the .force_pte assignment, this allows host-to-guest donations
to be mapped as block-level annotations. Does this conflict with
host_stage2_get_guest_info() in arch/arm64/kvm/hyp/nvhe/mem_protect.c?

That function appears to strictly assume annotations are mapped at page
granularity:

arch/arm64/kvm/hyp/nvhe/mem_protect.c:host_stage2_get_guest_info() {
    ...
    if (WARN_ON(level != KVM_PGTABLE_LAST_LEVEL))
    ...
}

Since WARN_ON() expands to a fatal BRK instruction at EL2 in pKVM, if
userspace triggers a stage-2 fault on a donated trailing page (e.g., by
passing it to a syscall), the fault handler invokes
pkvm_force_reclaim_guest_page() which eventually hits this WARN_ON.
Could this allow userspace to crash the hypervisor?

[Severity: Critical]
Also, when a block-level annotation is allowed, does this cause metadata
corruption if the block is later split?

When the generic split logic in stage2_map_prefault_idmap() splits a block,
it copies the block PTE verbatim into all child PTEs:

arch/arm64/kvm/hyp/pgtable.c:stage2_map_prefault_idmap() {
    ...
    kvm_pte_t pte = valid ? kvm_init_valid_leaf_pte(pa, block_pte, ctx->level + 1) : block_pte;
    ...
}

Since pKVM encodes the base GFN in the extra meta bits of the annotation,
this clones the base GFN into all 512 sub-pages without advancing it.

Similarly, if a multi-page annotation falls back to page-level entries in
stage2_map_walker_try_leaf(), it assigns the exact same GFN to all entries:

arch/arm64/kvm/hyp/pgtable.c:stage2_map_walker_try_leaf() {
    ...
    new = data->pte_annot;
    ...
}

If the host later reclaims a trailing page, EL2 might read the duplicated
base GFN, retrieve the wrong base physical address, and hit the
WARN_ON(pa != phys) panic in __pkvm_host_force_reclaim_page_guest().

>  		.annotation	= true,
>  		.pte_annot	= pte_annot |
>  				  FIELD_PREP(KVM_INVALID_PTE_TYPE_MASK, type),

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