Re: [PATCH 10/16] KVM: arm64: gic-v5: Add ITS userspace register access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] gic-v5: userspace override omits guest-side guard without post-restore validation
- [High] gic-v5: userspace writes to stateful and capability registers are silently discarded
- [Medium] gic-v5: missing state rollback on kvm_io_bus_register_dev failure
- [Medium] kvm: arm64: KVM_HAS_DEVICE_ATTR fails on ITS registers before base address is set
- [Medium] kvm: arm64: gic-v5: userspace writes to ITS ID registers are silently discarded
--

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

KVM: arm64: gic-v5: Add ITS userspace register access

This commit exposes the GICv5 ITS register frame to userspace for inspection
and restoration around VM reset and migration. It applies userspace access
constraints and adds control handling for reset while preserving ITS
defaults.

> diff --git a/arch/arm64/kvm/vgic/vgic-its-v5.c b/arch/arm64/kvm/vgic/vgic-its-v5.c
> index 5920fa1bcb2cf..62c1ca8004007 100644
> --- a/arch/arm64/kvm/vgic/vgic-its-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-its-v5.c
> @@ -851,6 +851,19 @@ static u64 vgic_v5_read_its_dt_baser(struct vgic_v5_its *its)
>  			  its->dt_baser.addr >> GICV5_ITS_DT_BASER_ADDR_SHIFT);
>  }
>  
> +static int vgic_v5_uaccess_write_its_dt_itt(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);
> +
> +	guard(mutex)(&its->its_lock);
> +
> +	vgic_v5_write_its_dt_reg(its, offset, val);
> +	return 0;
> +}

[Severity: High]
This userspace override in arch/arm64/kvm/vgic/vgic-its-v5.c intentionally
omits the guest-side !its->enabled guard, which makes sense for permitting
out-of-order restoration. However, when a guest-side refusal is omitted, a
final validation step is required at the end of the restore sequence to
re-establish the invariant.

Because KVM_DEV_ARM_VGIC_CTRL_INIT is handled as a no-op below and there
are no RESTORE_TABLES controls exposed for GICv5, KVM never validates the
final configuration. Could this allow userspace to leave the device in an
illegal state (e.g., base address modified while active) that the guest
could never produce?

[ ... ]
> @@ -1018,6 +1031,13 @@ static void vgic_v5_mmio_write_its_wi(struct kvm *kvm, void *dev, gpa_t addr,
>  		.its_read = rd, .its_write = wr,                       \
>  	}
>  
> +#define REGISTER_ITS_V5_DESC_UACCESS(off, rd, wr, uwr, length, acc)    \
> +	{                                                              \
> +		.reg_offset = off, .len = length, .access_flags = acc, \
> +		.its_read = rd, .its_write = wr,                       \
> +		.uaccess_its_write = uwr,                              \
> +	}
> +
>  static const struct vgic_register_region vgic_v5_its_registers[] = {
>  	REGISTER_ITS_V5_DESC(GICV5_ITS_IDR0, vgic_v5_mmio_read_its_misc,
>  			     vgic_v5_mmio_write_its_wi, 4, VGIC_ACCESS_32bit),

[Severity: Medium]
IDR0 is registered without a userspace write override, falling back to the
guest's write-ignore handler vgic_v5_mmio_write_its_wi(). Because userspace
writes are silently discarded here in arch/arm64/kvm/vgic/vgic-its-v5.c, a
VM migrated to a new host will adopt the destination host's physical ITS
capabilities (like pa_range in vgic_v5_its_reset()) rather than retaining
its original configuration.

Will this break migration for heterogeneous clusters since capability
mismatches are swallowed instead of rejected or applied?

[ ... ]
>  	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]
Stateful registers like READ_EVENT_DATAR also lack a uaccess_its_write
override in vgic_v5_its_registers and use the guest write-ignore handler
vgic_v5_mmio_write_its_wi().

Since the write-ignore handler silently accepts the write and discards the
value, wouldn't pending event data be permanently lost during migration?

[ ... ]
> +static int vgic_v5_its_has_attr_regs(struct kvm_device *dev,
> +				     struct kvm_device_attr *attr)
> +{
> +	struct vgic_v5_its *its = dev->private;
> +	const struct vgic_register_region *region;
> +	gpa_t offset = attr->attr;
> +	int align;
> +
> +	if (IS_VGIC_ADDR_UNDEF(its->vgic_v5_its_base))
> +		return -ENXIO;

[Severity: Medium]
This runtime state check in vgic_v5_its_has_attr_regs() enforces a base
address requirement during a purely capability-probing ioctl.

Userspace VMMs probing for supported GICv5 ITS registers via
KVM_HAS_DEVICE_ATTR will receive -ENXIO if they probe before configuring
the ITS base address. Could this cause the VMM to incorrectly assume the
registers or device capabilities are unsupported, breaking device
initialization or migration setup? Other VGIC implementations explicitly
omit the base address check in their has_attr handlers to permit capability
probing without prior configuration.

[ ... ]
> +static int vgic_v5_its_attr_regs_access(struct kvm_device *dev,
> +					struct kvm_device_attr *attr,
> +					u64 *reg, bool is_write)
> +{
...
> +	if (is_write) {
> +		if (region->uaccess_its_write)
> +			ret = region->uaccess_its_write(dev->kvm, its, addr,
> +							len, *reg);
> +		else
> +			region->its_write(dev->kvm, its, addr, len, *reg);
> +	} else {
> +		*reg = region->its_read(dev->kvm, its, addr, len);
> +	}

[Severity: High]
As mentioned above, userspace writes to several registers (IDR0, CR1,
SYNCR, READ_EVENT_DATAR) fall back to the guest-side its_write handler in
vgic_v5_its_attr_regs_access() when no override is provided. Because these
guest handlers silently accept the write and discard the value, mutable
guest state is lost across migration and invalid capability settings are
silently ignored instead of being rejected.

[ ... ]
> +static int vgic_v5_its_ctrl(struct kvm *kvm, struct vgic_v5_its *its, u64 attr)
> +{
> +	int ret = 0;
> +
> +	if (attr == KVM_DEV_ARM_VGIC_CTRL_INIT)
> +		return 0;

[Severity: High]
The initialization/finalization control in vgic_v5_its_ctrl() is a no-op,
meaning no end-of-restore validation is performed to check the invariants
that were bypassed by the userspace overrides (like the !its->enabled
check).

[ ... ]
> +static int vgic_v5_its_set_attr(struct kvm_device *dev,
> +				struct kvm_device_attr *attr)
> +{
...
> +		ret = vgic_check_iorange(dev->kvm, its->vgic_v5_its_base,
> +					 addr, SZ_64K, KVM_VGIC_V5_ITS_SIZE);
> +		if (ret)
> +			return ret;
> +
> +		return vgic_v5_register_its_iodev(dev->kvm, its, addr);

[Severity: Medium]
In vgic_v5_register_its_iodev() in arch/arm64/kvm/vgic/vgic-its-v5.c,
its->vgic_v5_its_base is populated with the requested addr before
attempting to register the IO bus device. If kvm_io_bus_register_dev()
fails (e.g. due to ENOMEM), the error path fails to clear
its->vgic_v5_its_base upon failure:

	its->vgic_v5_its_base = addr;
	...
	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, iodev->base_addr,
				      KVM_VGIC_V5_ITS_SIZE, &iodev->dev);
out:
	mutex_unlock(&kvm->slots_lock);

	return ret;

Will this leave the ITS permanently unusable? Subsequent attempts by
userspace to correctly set the base address will fail with -EBUSY because
IS_VGIC_ADDR_UNDEF() evaluates to false, leaving the ITS permanently
unmapped.

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