Re: [PATCH v2 06/13] KVM: arm64: Shadow the ITS command queue and setup emulation

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] The hypervisor ITS emulation fails to trap critical configuration registers (e.g., GITS_CBASER, GITS_CTLR), allowing the host to bypass emulation and execute arbitrary ITS commands.
- [Critical] Rolling back the emulation state when hardware command processing times out causes a critical state desynchronization, leading to memory corruption.
- [High] Unvalidated `cmd_offset` initialization from hardware allows the host to cause out-of-bounds memory accesses in the hypervisor.
- [High] An unsigned loop counter underflow in the error path of `pkvm_setup_its_shadow_cmdq` causes an infinite loop and out-of-bounds memory unsharing.
- [High] The ITS emulation uses `__hyp_va()` on physical MMIO device addresses, causing a Translation Fault.
- [High] The error path in `pkvm_its_emulate_setup` leaks randomized hypervisor virtual addresses (hyp tags) to the host kernel.
- [Medium] The ITS emulation mishandles 32-bit MMIO accesses to the 64-bit `GITS_CWRITER` and `GITS_CREADR` registers, applying bitmasks to the wrong payload bits.
- [Medium] The error path in `pkvm_its_emulate_setup` hardcodes a 1-page return for private memory, permanently leaking hypervisor pages if more were allocated.
- [Medium] Mismatched size calculations in `pkvm_setup_its_shadow_cmdq` cause it to attempt pinning unshared pages when addresses or lengths are unaligned.
--

--- Patch [6]: [PATCH v2 06/13] KVM: arm64: Shadow the ITS command queue and setup emulation ---
commit 44d4bb1ba5e1c6603582c76567c28f1efdb0f865
Author: Sebastian Ene <[email protected]>

KVM: arm64: Shadow the ITS command queue and setup emulation

This patch introduces pKVM ITS command queue emulation. It sets up private
and shared memory regions to track the ITS command queue and intercepts
writes to GITS_CWRITER to validate commands before pushing them to the
hardware queue.

> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> index 63a42f520ed25..e943ab972aa5e 100644
> --- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
[ ... ]
> @@ -35,3 +38,285 @@ void its_emulate_forward_req(struct pkvm_protected_reg *region, u64 offset, bool
> +static int submit_single_cmd(struct its_priv_state *its, bool retry)
> +{
[ ... ]
> +		/*
> +		 * We can't spin here forever and we can't roll back
> +		 * the cmd queue pointer. Let's revert the cmd effects in the
> +		 * emulation layer and then go back to the driver to let it
> +		 * decide what to do next.
> +		 */
> +		if (!timeout--)
> +			return -EBUSY;

[Severity: Critical]
Can this state rollback cause memory corruption?

If the hardware takes too long to process the command and we time out here,
returning -EBUSY causes cwriter_write() to call process_cmd(..., true)
to revert the hypervisor's internal tracking state. 

However, the hardware is still asynchronously processing the command and cannot
be aborted. Will the hardware operate on memory that the hypervisor believes
has already been unmapped and returned to the host?

> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static void cwriter_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
> +{
> +	struct its_priv_state *its = region->priv;
> +	struct its_cmd_block cmd, raw;
> +	u64 new_offset;
> +	bool retry;
> +	int i;
> +
> +	new_offset = value & GITS_CWRITER_OFFSET;

[Severity: Medium]
Does this properly handle 32-bit partial MMIO accesses?

The pkvm_its_emulate_handler() passes the 32-bit payload directly to 
the handler as the value parameter, but cwriter_write() ignores the 
offset parameter and treats the incoming 32-bit payload as the absolute
64-bit value. This might extract the wrong bits. 

cwriter_read() below also ignores the offset and always returns the full
64-bit register.

> +	if (new_offset >= its->host_state->cmdq_len)
> +		return;
> +
> +	retry = FIELD_GET(GITS_CWRITER_RETRY, value);
> +	while (its->cmd_offset != new_offset) {
> +		memcpy(&raw, its->cmd_host_copy + its->cmd_offset, sizeof(raw));

[Severity: High]
Can the host exploit the initial cmd_offset to cause out-of-bounds memory
accesses here?

The cmd_offset is initialized directly from the hardware's GITS_CREADR in
pkvm_its_emulate_setup(), which the host could have manipulated prior to
setup. This cmd_offset is used as an index for memcpy() operations without
checking if it exceeds the provided cmdq_len.

> +
> +		for (i = 0; i < ARRAY_SIZE(cmd.raw_cmd); i++)
> +			cmd.raw_cmd[i] = le64_to_cpu(raw.raw_cmd_le[i]);
> +
> +		if (process_cmd(its, &cmd, /* rollback */ false))
> +			return;
> +
> +		memcpy(its->cmd_original + its->cmd_offset, &raw, sizeof(struct its_cmd_block));
> +
> +		if (its->needs_flush)
> +			gic_flush_dcache_to_poc(its->cmd_original + its->cmd_offset, sizeof(cmd));
> +		else
> +			dsb(ishst);
> +
> +		if (submit_single_cmd(its, retry)) {
> +			WARN_ON(process_cmd(its, &cmd, /* rollback */ true));
> +			return;
> +		}
> +	}
> +}
> +
> +static void cwriter_read(struct pkvm_protected_reg *region, u64 offset, u64 *read)
> +{
> +	struct its_priv_state *its = region->priv;
> +	*read = readq_relaxed(its->base + GITS_CWRITER);
> +}
> +
> +static struct its_handler its_handlers[] = {
> +	ITS_HANDLER(GITS_CWRITER, sizeof(u64), cwriter_write, cwriter_read),
> +	{},
> +};

[Severity: Critical]
Is it safe to only intercept GITS_CWRITER in this handler array? 

It appears all other configuration registers (like GITS_CBASER and
GITS_CTLR) fall through to its_emulate_forward_req() which passes
the unintercepted writes directly to the hardware. 

Could a malicious host directly modify GITS_CBASER to point the hardware queue
to an unvalidated memory page, bypassing all emulation checks and compromising
hypervisor isolation?

[ ... ]
> +static int pkvm_setup_its_shadow_cmdq(struct its_host_state *host_state)
> +{
> +	u64 start_pfn, num_pages, i;
> +	int ret;
> +
> +	start_pfn = hyp_virt_to_pfn(host_state->cmd_host_copy);
> +	num_pages = host_state->cmdq_len >> PAGE_SHIFT;
> +
> +	for (i = 0; i < num_pages; i++) {
> +		ret = __pkvm_host_share_hyp(start_pfn + i);
> +		if (ret)
> +			goto unshare_cmd_host;
> +	}
> +
> +	ret = hyp_pin_shared_mem(host_state->cmd_host_copy,
> +				 host_state->cmd_host_copy + host_state->cmdq_len);

[Severity: Medium]
Could this cause a mismatch in the number of pages pinned vs shared?

The calculation "num_pages = host_state->cmdq_len >> PAGE_SHIFT" truncates
the size, sharing exactly num_pages. However, hyp_pin_shared_mem() page-aligns
the boundaries up and down. 

If cmd_host_copy or cmdq_len are unaligned, the pin operation could span
num_pages + 1 pages, attempting to pin an unshared page and failing the
ownership check.

> +	if (ret)
> +		goto unshare_cmd_host;
[ ... ]
> +unshare_cmd_host:
> +	if (i == 0)
> +		return ret;
> +
> +	for (i = i - 1; i >= 0; i--)
> +		__pkvm_host_unshare_hyp(start_pfn + i);

[Severity: High]
Will this loop underflow and cause an infinite loop? 

The variable 'i' is defined as an unsigned 64-bit integer (u64) at the top of 
the function. The condition 'i >= 0' is always true for unsigned variables, 
meaning when 'i' reaches 0 it will wrap around to UINT64_MAX. 

This could unshare wildly out-of-bounds PFNs and crash the hypervisor.

> +	return ret;
> +}
[ ... ]
> +int pkvm_its_emulate_setup(phys_addr_t dev_addr, struct its_host_state *host_state, void *priv,
> +			   size_t priv_num_pages)
> +{
[ ... ]
> +	host_state->cmd_original = kern_hyp_va(host_state->cmd_original);
> +	host_state->cmd_host_copy = kern_hyp_va(host_state->cmd_host_copy);
> +
> +	ret = pkvm_setup_its_shadow_cmdq(host_state);
> +	if (ret)
> +		goto err_with_host_state;

[Severity: High]
Does this leak the randomized hypervisor virtual address to the host?

We mutate the host-provided host_state page in-place using kern_hyp_va()
which applies the hypervisor's KASLR tag. If pkvm_setup_its_shadow_cmdq()
fails, this modified page is returned directly to the host unscrubbed in
the err_with_host_state cleanup path, exposing the hypervisor's linear map tag.

> +
> +	hyp_spin_lock_init(&priv_state->its_lock);
> +
> +	priv_state->host_state = host_state;
> +	priv_state->base = (void __iomem *)__hyp_va(dev_addr);

[Severity: High]
Is it correct to use __hyp_va() on a physical MMIO device address?

In ARM64 pKVM, __hyp_va() applies a fixed offset intended only for 
mapping system RAM into the hypervisor's linear map. It does not work 
for physical MMIO regions. 

Attempting to access these virtual addresses will likely cause a Translation
Fault and crash the hypervisor during setup. its_emulate_forward_req() also
appears to use this macro.

> +	priv_state->cmd_original = host_state->cmd_original;
> +	priv_state->cmd_host_copy = host_state->cmd_host_copy;
> +
> +	priv_state->cmd_offset = readq_relaxed(priv_state->base + GITS_CREADR) &
> +		GITS_CREADR_OFFSET;
> +	priv_state->needs_flush =
> +		(readq_relaxed(priv_state->base + GITS_CBASER) & GITS_CBASER_SHAREABILITY_MASK) !=
> +		GITS_CBASER_InnerShareable;
> +
> +	its_reg->priv = priv_state;
> +
> +	hyp_spin_unlock(&its_setup_lock);
> +
> +	return 0;
> +err_with_host_state:
> +	WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(host_state), 1));
> +err_with_priv:
> +	WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(priv_state), 1));

[Severity: Medium]
Should this use priv_num_pages instead of a hardcoded 1?

The hypervisor allocates priv_num_pages for the private state tracking,
but if an error occurs here, it explicitly returns only 1 page to the
host. If priv_num_pages > 1, won't the remaining pages permanently leak?

> +err_unlock:
> +	hyp_spin_unlock(&its_setup_lock);
> +	return ret;
> +}

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