Re: [PATCH v2 13/13] KVM: arm64: Implement HVC interface for ITS emulation setup

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] The `psz` (page size) field read from the untrusted `host_state` is not validated, allowing a malicious host to bypass ownership transfer of Level 2 ITS tables.
- [High] Calling `on_each_cpu()` with local interrupts disabled causes a system-wide deadlock and triggers a kernel warning.
- [High] Allocating memory via `alloc_pages_exact(..., GFP_ATOMIC)` while holding a `raw_spin_lock` violates locking hierarchy and PREEMPT_RT constraints.
- [High] The error path in `pkvm_its_emulate_setup()` rolls back the `priv_state` donation using a hardcoded size of 1 page instead of `priv_num_pages`, leaking hypervisor memory.
- [High] An error rollback loop in `pkvm_setup_its_shadow_cmdq()` uses an unsigned 64-bit integer, resulting in an infinite loop.
--

--- Patch [13]: [PATCH v2 13/13] KVM: arm64: Implement HVC interface for ITS emulation setup ---
commit b5e04e3693da3938b3ebc14d5487e70eddc3c941
Author: Sebastian Ene <[email protected]>

KVM: arm64: Implement HVC interface for ITS emulation setup

This patch introduces a new HVC to allow the host to trigger the ITS emulation
setup. It uses the API in the GIC ITS driver to lock the ITS before pKVM
finalize, switches to a different command queue, and allocates memory to track
internal state. The initial MMIO handler is replaced with a full-featured
emulation handler.

> diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
> index 5e80da18f3120..9cd562349f249 100644
> --- a/arch/arm64/kvm/pkvm.c
> +++ b/arch/arm64/kvm/pkvm.c
[ ... ]
> @@ -312,8 +312,28 @@ static void __init _kvm_host_prot_finalize(void *arg)
[ ... ]
> +static int pkvm_init_its_emulation(phys_addr_t dev_addr, struct its_host_state *host)
> +{
> +	size_t priv_state_sz = ITS_PAGES << PAGE_SHIFT;
> +	void *priv_state;
> +	int ret;
> +
> +	priv_state = alloc_pages_exact(priv_state_sz, GFP_ATOMIC);

[Severity: High]
Does this allocation violate locking hierarchy constraints?

This function is invoked as a callback from its_emulate_release_locks() in
drivers/irqchip/irq-gic-v3-its.c, which iterates over ITS nodes and calls this
while holding both its_lock and the per-node its->lock raw spinlocks.

On PREEMPT_RT kernels, internal page allocator spinlocks are sleeping
rt_mutexes. Furthermore, GFP_ATOMIC contains __GFP_KSWAPD_RECLAIM, which can
wake kswapd and acquire scheduler runqueue locks while holding a true raw
spinlock.

> +	if (!priv_state)
> +		return -ENOMEM;
[ ... ]
>  static int __init pkvm_drop_host_privileges(void)
>  {
> +	unsigned long its_flags;
>  	int ret = 0;
>  
>  	/*
> @@ -321,8 +341,10 @@ static int __init pkvm_drop_host_privileges(void)
>  	 * once the host stage 2 is installed.
>  	 */
>  	static_branch_enable(&kvm_protected_mode_initialized);
> +
> +	its_emulate_acquire_locks(&its_flags);
>  	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);

[Severity: High]
Can this sequence cause a system-wide deadlock?

its_emulate_acquire_locks() explicitly disables local hardware interrupts via
raw_spin_lock_irqsave(). Immediately after, on_each_cpu() is called, which
relies on smp_call_function_many() to broadcast an IPI and synchronously wait
for completion.

Waiting for an IPI while local IRQs are disabled is an AB-BA deadlock
scenario.

> -	return ret;
> +	return its_emulate_release_locks(ret, &its_flags, pkvm_init_its_emulation);
>  }

[ ... ]

For the items identified in arch/arm64/kvm/hyp/nvhe/its_emulate.c, the
following observations were made based on the underlying emulation setup:

arch/arm64/kvm/hyp/nvhe/its_emulate.c:pkvm_setup_its_shadow_cmdq() {
    ...
    for (i = i - 1; i >= 0; i--)
        __pkvm_host_unshare_hyp(start_pfn + i);
}

[Severity: High]
Can this error rollback trigger an infinite loop?

If the loop counter i is declared as an unsigned 64-bit integer, the condition
i >= 0 is universally true. When i wraps around to 0xFFFFFFFFFFFFFFFF, it will
continually execute out-of-bounds __pkvm_host_unshare_hyp() calls instead of
terminating.


arch/arm64/kvm/hyp/nvhe/its_emulate.c:pkvm_host_unmap_last_level() {
    ...
    ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(table_addr), psz >> PAGE_SHIFT);
}

[Severity: Critical]
Does this bypass pKVM isolation guarantees by trusting the host-provided page
size?

The psz field appears to be read directly from the untrusted host_state
without validation. If a malicious host sets psz to a value strictly less than
PAGE_SIZE (e.g., 0), psz >> PAGE_SHIFT evaluates to 0. The donation will
silently succeed without actually transferring page ownership, allowing the
host to retain ownership and concurrently modify the tables.


arch/arm64/kvm/hyp/nvhe/its_emulate.c:pkvm_its_emulate_setup() {
    ...
err_with_priv:
    WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(priv_state), 1));
}

[Severity: High]
Does this error path leak hypervisor memory?

The setup successfully donates priv_num_pages from the host to EL2, but if a
subsequent operation fails, this rollback hardcodes 1 instead of using
priv_num_pages. If priv_num_pages is 2 for ITS setup, the remaining pages are
permanently left in the HYP-owned state.

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