Re: [PATCH v2 02/13] KVM: arm64: Track host-unmapped MMIO regions in a static array

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

Pre-existing issues:
- [High] The error rollback path incorrectly maps previously unmapped protected MMIO regions into the host stage-2 page tables, breaking isolation.
- [High] Invalid use of the linear map macro `__hyp_va()` for physical MMIO addresses causes hypervisor page table bounds checks to fail or collide with private VA regions.
--

--- Patch [2]: [PATCH v2 02/13] KVM: arm64: Track host-unmapped MMIO regions in a static array ---
commit 8dc1d17638f0ac1801faef9ffe3be3d70a9ddf5c
Author: Sebastian Ene <[email protected]>
Subject: KVM: arm64: Track host-unmapped MMIO regions in a static array

This commit introduces a registry to track protected MMIO regions that are
unmapped from the host stage-2 page tables. It stores these regions in a
fixed-size array and donates their ownership to the hypervisor during
initialization to ensure host-exclusion and persistent tracking.

> diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> index 75b00c3233102..64c0290da888f 100644
> --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
[ ... ]
> @@ -284,6 +284,26 @@ static int fix_hyp_pgtable_refcnt(void)
> +static int donate_protected_mmio_regions(void)
> +{
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < num_protected_reg; i++) {
> +		ret = __pkvm_host_donate_hyp_mmio(hyp_pfn_to_phys(pkvm_protected_regs[i].pfn),
> +						  pkvm_protected_regs[i].nr_pages << PAGE_SHIFT);
[Severity: High]
This is a pre-existing issue, but does calling __pkvm_host_donate_hyp_mmio()
here work for physical MMIO addresses?

Looking at __pkvm_host_donate_hyp_mmio(), it uses __hyp_va() to compute the
virtual address:

__pkvm_host_donate_hyp_mmio() {
	...
	virt = __hyp_va(addr + offset);
	...
}

Since __hyp_va() applies the linear map offset, which is sized and aligned
specifically for RAM, applying it to physical MMIO addresses (which reside
outside RAM boundaries) yields an invalid virtual address. This causes the
subsequent hypervisor page table bounds checks to fail with -ERANGE, or
silently corrupts EL2 private memory structures, reliably forcing the failure
path to execute.

> +		if (ret)
> +			goto err_setup;
> +	}
> +
> +	return 0;
> +err_setup:
> +	while (--i >= 0)
> +		__pkvm_hyp_donate_host_mmio(hyp_pfn_to_phys(pkvm_protected_regs[i].pfn),
> +					    pkvm_protected_regs[i].nr_pages << PAGE_SHIFT);
[Severity: High]
This isn't a bug introduced by this patch, but could this error rollback path
inadvertently map previously unmapped regions into the host stage-2?

The regions tracked in pkvm_protected_regs are intended to remain unmapped
from the host stage-2 page tables. However, __pkvm_hyp_donate_host_mmio()
unconditionally idmaps the region to the host at the end of the function:

__pkvm_hyp_donate_host_mmio() {
	...
	WARN_ON(host_stage2_idmap_locked(addr, size, PKVM_HOST_MMIO_PROT));
	...
}

Since the failure path is triggered by the __hyp_va() issue mentioned above,
this rollback will execute and forcibly install a valid mapping in the host
stage-2 page tables for regions that were originally unmapped, breaking the
hypervisor-host isolation boundary.

> +	return ret;
> +}

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