Re: [PATCH v2 10/13] KVM: arm64: Restrict host updates to GITS_CTLR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Missing interception of GITS_CBASER and GITS_BASER<n> allows the host to bypass ITS emulation and execute unvalidated commands.
- [High] Concurrent modification of the host's `snapshot_table` during ITS command rollback leads to arbitrary memory donation attempts.
- [High] Widespread use of `WARN_ON` for host-reachable error conditions in ITS emulation panics the hypervisor.
- [High] Incorrect sub-word MMIO handling in `ctlr_write` and `ctlr_read` leads to register corruption.
- [Medium] 64-bit accesses to GITS_CTLR are incorrectly dropped, causing reads to return uninitialized memory and writes to be silently ignored.
- [Medium] The handler fails to mask reserved (RES0) bits from the host-provided `value` before writing to the physical `GITS_CTLR` register.
--

--- Patch [10]: [PATCH v2 10/13] KVM: arm64: Restrict host updates to GITS_CTLR ---
commit 8547f0737e8d44ccf52c0d09c7367b0079543481
Author: Sebastian Ene <sebastianene@google.com>

    KVM: arm64: Restrict host updates to GITS_CTLR
    
    Prevent unpredictable hardware behavior when the host tries to enable
    the ITS while it is not in quiescent state.

This commit adds emulation for the GITS_CTLR register to restrict the host
from enabling the ITS while it is not in a quiescent state. It adds
ctlr_read and ctlr_write handlers and registers them in the its_handlers
array.

> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> index 5629e2a070df5..b9b71aa18d48b 100644
> --- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> @@ -403,8 +403,35 @@ static void cwriter_read(struct pkvm_protected_reg *region, u64 offset, u64 *rea
>  	*read = readq_relaxed(its->base + GITS_CWRITER);
>  }
>  
> +static void ctlr_read(struct pkvm_protected_reg *region, u64 offset, u64 *read)
> +{
> +	struct its_priv_state *its = region->priv;
> +	*read = readl_relaxed(its->base + GITS_CTLR);
> +}

[Severity: High]
Does this read handler account for the access offset? When the host performs
a partial sub-word read at a non-zero offset, it appears we return the full
32-bit register value instead of the requested bytes.

> +
> +static void ctlr_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
> +{
> +	struct its_priv_state *its = region->priv;
> +	bool is_quiescent, is_enabled;
> +	u32 ctlr;
> +
> +	ctlr = readl_relaxed(its->base + GITS_CTLR);
> +	is_quiescent = !!(ctlr & GITS_CTLR_QUIESCENT);
> +	is_enabled = !!(ctlr & GITS_CTLR_ENABLE);
> +
> +	/*
> +	 * If it's disabled and not in quiescent state and it tries to enable
> +	 * it, bail out.
> +	 */
> +	if (!is_enabled && (value & GITS_CTLR_ENABLE) && !is_quiescent)
> +		return;
> +
> +	writel_relaxed(value, its->base + GITS_CTLR);

[Severity: High]
Can a sub-word write corrupt the register state here? If the host performs a
partial 8-bit or 16-bit write at a non-zero offset, this writes the provided
value directly to offset 0 as a 32-bit word, which would zero-extend the value
and wipe out other bits in GITS_CTLR.

[Severity: Medium]
Should we mask out reserved (RES0) bits from the host-provided value before
writing it? The host could write non-zero values to RES0 bits, which the
architecture states can lead to unpredictable hardware behavior.

> +}
> +
>  static struct its_handler its_handlers[] = {
>  	ITS_HANDLER(GITS_CWRITER, sizeof(u64), cwriter_write, cwriter_read),
> +	ITS_HANDLER(GITS_CTLR, sizeof(u32), ctlr_write, ctlr_read),
>  	{},
>  };

[Severity: Critical]
Does this registry need to intercept GITS_CBASER and GITS_BASER<n>? Because
they are omitted, writes to these registers fall through and are forwarded
directly to hardware. This could allow the host to bypass the ITS emulation
completely by configuring the hardware to read from host-controlled memory,
leading to a potential DMA attack against EL2 memory.


I also noticed a few other issues in the ITS emulation code that might need
attention.

[Severity: High]
In check_table_update(), could there be a TOCTOU race with the host?

	snapshot_table = kern_hyp_va(table->base_snapshot);
...
	new_entry = snapshot_table[new_entry_index];
...
	if (rollback)
		new_entry = new_entry ^ GITS_BASER_VALID;

	if (new_entry & GITS_BASER_VALID)
		ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(new_entry & PHYS_MASK),

The snapshot_table is in host-shared memory. If the host concurrently
modifies the entry between the initial check and the rollback, couldn't it
force the rollback path to donate an arbitrary PFN to the hypervisor?

[Severity: High]
Can an untrusted host trigger the WARN_ON() in cwriter_write()?

		if (submit_single_cmd(its, retry)) {
			WARN_ON(process_cmd(its, &cmd, /* rollback */ true));
			return;
		}

If the host intentionally stalls the ITS to force a command timeout, this
rollback path is executed. If the rollback fails, the WARN_ON() will panic
the hypervisor in EL2 nVHE, bringing down the host and all protected VMs.

[Severity: Medium]
Are 64-bit accesses to GITS_CTLR incorrectly dropped by pkvm_its_emulate_handler()?

		if (reg_handler->access_size < reg_size)
			return;

Since GITS_CTLR is configured with an access_size of 4, a valid 64-bit access
from the host (which is permitted by the GICv3 architecture) would be silently
ignored, potentially returning uninitialized memory on reads.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=10