Re: [PATCH v2 12/13] KVM: arm64: Prevent the host from programming new GITS_BASER tables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Sub-word (32-bit) MMIO writes to 64-bit ITS registers cause hypervisor crashes (Alignment Fault) and silent state corruption.
- [Critical] Incomplete attribute masking in baser_write and cbaser_write allows the host to alter ITS table Cacheability and Shareability.
- [Critical] The host can activate unvalidated ITS tables by toggling the GITS_BASER Valid bit (bit 63).
- [High] Incomplete physical address masking in baser_write on 52-bit PA systems (FEAT_LPA) allows physical address hijacking.
--

--- Patch [12]: [PATCH v2 12/13] KVM: arm64: Prevent the host from programming new GITS_BASER tables ---
commit ceb5f9bba1bc25bf01683842fe6461183f94c200
Author: Sebastian Ene <sebastianene@google.com>

KVM: arm64: Prevent the host from programming new GITS_BASER tables

Don't allow the host to change the layout of the tables or to modify the
address programmed in the GITS_BASER registers to point to new tables
and bypass the sanitization.
Prevent the host from updating the ITS tables while the ITS is enabled
and the tables are set to prevent undefined behavior.

> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> --- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c

[ ... ]

> @@ -460,10 +465,42 @@ static void cbaser_read(struct pkvm_protected_reg *region, u64 offset, u64 *read
>  	*read = readq_relaxed(its->base + GITS_CBASER);
>  }
>  
> +static void baser_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
> +{
> +	struct its_priv_state *its = region->priv;
> +	u32 ctlr = readl_relaxed(its->base + GITS_CTLR);
> +	int baser_idx;
> +	u64 baser;
> +
> +	if ((ctlr & GITS_CTLR_ENABLE) || !(ctlr & GITS_CTLR_QUIESCENT))
> +		return;
> +
> +	baser_idx = (offset - GITS_BASER) >> 3;
> +	baser = its->host_state->tables[baser_idx].val;
> +
> +	/* Prevent if it tries to change from direct layout to indirect layout */
> +	if ((value & GITS_BASER_INDIRECT) != (baser & GITS_BASER_INDIRECT))
> +		return;
> +
> +	/* Don't allow the host to point to new tables or new attributes */
> +	value &= ~(GENMASK_ULL(47, 12) | GENMASK_ULL(9, 0));
> +	value |= (baser & GENMASK_ULL(47, 12)) | (baser & GENMASK_ULL(9, 0));

[Severity: Critical]
Does this mask correctly restrict the cacheability and shareability attributes?

The mask only clears bits 47:12 and 9:0, which leaves Cacheability
(bits 61:53) and Shareability (bits 11:10) unmasked. This allows the host
to force the ITS hardware to bypass caches while EL2 accesses the same
memory via Cacheable mappings, potentially compromising memory coherency
and EL2 isolation.

The cbaser_write() function similarly fails to mask Cacheability and
Shareability attributes for the command queue base register.

[Severity: High]
Can this mask allow the host to hijack the physical address on 52-bit PA
systems?

For systems with FEAT_LPA, PA[51:48] are stored in bits 51:48 of GITS_BASER.
Since the mask only covers up to bit 47, the host can supply arbitrary
values in bits 51:48. This would redirect the ITS table physical address
by multiples of 256TB to untracked memory, bypassing pKVM stage-2 tracking.

[Severity: Critical]
Could this allow the host to activate unvalidated ITS tables by toggling the
Valid bit (bit 63)?

Because bit 63 is unmasked, a malicious host could boot with a fabricated PA
and VALID = 0, skipping KVM's validation and memory donation. If the host
subsequently writes to GITS_BASER with VALID = 1, this mask restores the
fabricated PA and writes VALID = 1 to the hardware, granting the ITS
hardware DMA access to arbitrary physical addresses.

> +
> +	writeq_relaxed(value, its->base + offset);

[Severity: Critical]
Could a 32-bit MMIO write to a 64-bit ITS register cause a hypervisor crash
or state corruption here?

Looking at pkvm_its_emulate_handler() in this file, it permits sub-word
accesses to 64-bit registers:

    if (reg_handler->access_size < reg_size)
            return;

If a host issues a 32-bit write to an unaligned offset (e.g.,
GITS_BASER + 4), this writeq_relaxed() will perform a 64-bit device memory
access at an unaligned address, triggering an Alignment Fault at EL2. Even
if aligned, writing a 32-bit payload via a 64-bit writeq_relaxed() will
overwrite the upper 32 bits of the hardware register with zeros, wiping
critical fields.

Should the payload be adapted for a read-modify-write, or alignment
enforced? The cbaser_write() function appears to share this same behavior.

> +}

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