Re: [PATCH 06/16] KVM: arm64: gic-v5: Add ITS MMIO state emulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: arm64: Data race and TOCTOU due to missing its_lock in MMIO handlers
- [High] kvm: arm64: gic-v5: Userspace writes to ITS READ_EVENT_DATAR and ID registers are silently dropped
--

commit cda638a2b9c74f59afeb5648c53ce1083d96d5c3
Author: Sascha Bischoff <sascha.bischoff@arm.com>

KVM: arm64: gic-v5: Add ITS MMIO state emulation

In order to expose a virtual GICv5 ITS to the guest, KVM needs to
emulate the ITS CONFIG_FRAME and its translation-control registers.
Replace the placeholder ITS IODEV region with the GICv5 ITS register
layout and handlers for the architected MMIO state.

> diff --git a/arch/arm64/kvm/vgic/vgic-its-v5.c b/arch/arm64/kvm/vgic/vgic-its-v5.c
> index f8184b4ae0ccd..fda75314229a5 100644
> --- a/arch/arm64/kvm/vgic/vgic-its-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-its-v5.c

[ ... ]

> +static void vgic_v5_mmio_write_its_misc(struct kvm *kvm, void *dev, gpa_t addr,
> +					unsigned int len, unsigned long val)
> +{
> +	struct vgic_v5_its *its = dev;
> +	size_t offset = addr & (SZ_64K - 1);
> +
> +	switch (offset) {
> +	case GICV5_ITS_CR0:
> +		its->enabled = FIELD_GET(GICV5_ITS_CR0_ITSEN, val);
> +		return;
> +	case GICV5_ITS_CR1:
> +		its->cr1.sh = FIELD_GET(GICV5_ITS_CR1_SH, val);
> +		its->cr1.oc = FIELD_GET(GICV5_ITS_CR1_OC, val);
> +		its->cr1.ic = FIELD_GET(GICV5_ITS_CR1_IC, val);
> +		its->cr1.dt_ra = FIELD_GET(GICV5_ITS_CR1_DT_RA, val);
> +		its->cr1.itt_ra = FIELD_GET(GICV5_ITS_CR1_ITT_RA, val);
> +		return;
> +	default:
> +		return;
> +	}
> +}

[Severity: High]
Does this code introduce a data race or TOCTOU vulnerability?

The vgic_v5_mmio_write_its_misc() function modifies the its->enabled flag
and its->cr1 fields without acquiring its->its_lock. Concurrently,
vgic_v5_mmio_write_its_dt_itt() uses guard(mutex)(&its->its_lock) to safely
evaluate !its->enabled and modify table bases.

Allowing lockless modification of its->enabled could bypass the check
if another vCPU is simultaneously writing to translation base registers.
Additionally, this permits torn reads of the multi-field cr1 struct. Should
this function acquire its->its_lock?

[ ... ]

>  static const struct vgic_register_region vgic_v5_its_registers[] = {
> -	REGISTER_ITS_V5_DESC(0, vgic_v5_mmio_read_its_raz,
> -			     vgic_v5_mmio_write_its_wi, KVM_VGIC_V5_ITS_SIZE,
> +	REGISTER_ITS_V5_DESC(GICV5_ITS_IDR0, vgic_v5_mmio_read_its_misc,
> +			     vgic_v5_mmio_write_its_wi, 4, VGIC_ACCESS_32bit),
> +	REGISTER_ITS_V5_DESC(GICV5_ITS_IDR1, vgic_v5_mmio_read_its_misc,
> +			     vgic_v5_mmio_write_its_wi, 4, VGIC_ACCESS_32bit),
> +	REGISTER_ITS_V5_DESC(GICV5_ITS_IDR2, vgic_v5_mmio_read_its_misc,
> +			     vgic_v5_mmio_write_its_wi, 4, VGIC_ACCESS_32bit),

[ ... ]

> +	REGISTER_ITS_V5_DESC(GICV5_ITS_READ_EVENT_DATAR,
> +			     vgic_v5_mmio_read_its_dt_itt,
> +			     vgic_v5_mmio_write_its_wi, 8,
> +			     VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),

[Severity: High]
Are userspace writes to restore migration state being silently dropped?

Both the ID registers (e.g. IDR0) and READ_EVENT_DATAR are configured
with the vgic_v5_mmio_write_its_wi write-ignore handler. Since there is no
explicit userspace override provided for these registers, a userspace write
will fall back to the guest MMIO write handler, accepting the write and
returning success (0) but discarding the value.

For ID registers, ignoring the write prevents the VMM from detecting
incompatibility across migration. For READ_EVENT_DATAR, the result of an
inflight READ_EVENTR command is lost. Does this violate the ABI contract
that userspace writes must either take effect or return an error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016155741.979985-1-sascha.bischoff@arm.com?part=6