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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] KVM: arm64: gic-v5: missing vgic_initialized() check before LPI injection
- [Medium] KVM: arm64: gic-v5: unenforced IDR1/IDR2 limits and stale cache on write
- [Medium] KVM: arm64: gic-v5: GICV5_ITS_SYNCR registered as 32-bit instead of 64-bit
- [Low] KVM: arm64: gic-v5: 64-bit attr->attr offset truncated to 32-bit in ITS regs
--

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

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

This commit exposes the emulated GICv5 ITS MMIO register frame to userspace
via KVM_DEV_ARM_VGIC_GRP_ITS_REGS and adds support for resetting the ITS via
KVM_DEV_ARM_ITS_CTRL_RESET. It also adds dedicated userspace write handlers
for restoring IDR0-IDR2, DT_BASER, DT_CFGR, and READ_EVENT_DATAR.

> diff --git a/arch/arm64/kvm/vgic/vgic-its-v5.c b/arch/arm64/kvm/vgic/vgic-its-v5.c
> index b69a1d107c0ae..638ee47f6e27d 100644
> --- a/arch/arm64/kvm/vgic/vgic-its-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-its-v5.c
> @@ -688,6 +688,70 @@ static void vgic_v5_mmio_write_its_misc(struct kvm *kvm, void *dev, gpa_t addr,
>  	}
>  }
>  
> +static int vgic_v5_uaccess_write_its_idr(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);
> +	u64 mmfr0;
> +
> +	guard(mutex)(&its->its_lock);
> +
> +	switch (offset) {
[ ... ]
> +	case GICV5_ITS_IDR1:
> +		if ((val & ~(GICV5_ITS_IDR1_L2SZ |
> +			     GICV5_ITS_IDR1_ITT_LEVELS |
> +			     GICV5_ITS_IDR1_DT_LEVELS |
> +			     GICV5_ITS_IDR1_DEVICEID_BITS)) ||
> +		    FIELD_GET(GICV5_ITS_IDR1_DEVICEID_BITS, val) >
> +		    VITS_V5_DEVICE_ID_BITS)
> +			return -EINVAL;
> +
> +		/*
> +		 * Userspace can change the DeviceID limit after translations have
> +		 * been cached. Drop them so the old limit cannot remain in effect.
> +		 */
> +		if (its->idr1.device_id_bits !=
> +		    FIELD_GET(GICV5_ITS_IDR1_DEVICEID_BITS, val))
> +			its_cache_clear_translations(its);
> +
> +		its->idr1.device_id_bits = FIELD_GET(GICV5_ITS_IDR1_DEVICEID_BITS,
> +							 val);
> +		its->idr1.dt_levels = !!(val & GICV5_ITS_IDR1_DT_LEVELS);
> +		its->idr1.itt_levels = !!(val & GICV5_ITS_IDR1_ITT_LEVELS);
> +		its->idr1.l2sz = FIELD_GET(GICV5_ITS_IDR1_L2SZ, val);
> +		return 0;
> +	case GICV5_ITS_IDR2:
> +		if ((val & ~GICV5_ITS_IDR2_EVENTID_BITS) ||
> +		    FIELD_GET(GICV5_ITS_IDR2_EVENTID_BITS, val) >
> +		    VITS_V5_EVENT_ID_BITS)
> +			return -EINVAL;
> +
> +		its->idr2.event_id_bits = FIELD_GET(GICV5_ITS_IDR2_EVENTID_BITS,
> +							 val);
> +		return 0;

[Severity: Medium]
When userspace writes GICV5_ITS_IDR1 or GICV5_ITS_IDR2 in
vgic_v5_uaccess_write_its_idr(), changes to its->idr1.device_id_bits clear
the translation cache and are enforced in get_itt() via:

	device_id_bits = min(its->dt_cfgr.device_id_bits,
			     its->idr1.device_id_bits);

However, for GICV5_ITS_IDR2, updating its->idr2.event_id_bits does not call
its_cache_clear_translations(its), and __vgic_v5_its_resolve_lpi() checks
event_id against the unclamped event_id_bits read from the guest DTE without
clamping to its->idr2.event_id_bits:

	ret = get_itt(kvm, its, device_id, &itt_base, &two_level_itt,
		      &event_id_bits, &itt_l2sz);
	...
	if (event_id >= 1UL << event_id_bits) {
		kvm_debug("Event ID out of range\n");
		return VITS_V5_UNMAPPED;
	}

Similarly, in GICV5_ITS_IDR1, updates to its->idr1.dt_levels,
its->idr1.itt_levels, and its->idr1.l2sz do not invalidate cached
translations, and get_itt(), parse_two_level_dt(), get_itte(), and
parse_two_level_itt() do not check whether two-level tables or the selected
L2 sizes are enabled in its->idr1 (and vgic_v5_its_reset() initializes
its->idr1.l2sz to 0 while advertising dt_levels = 1 and itt_levels = 1).

Should changes to these IDR1 and IDR2 fields also flush the translation cache
and be enforced during DT/ITT translation walks?

[ ... ]
> @@ -1130,64 +1241,226 @@ static int vgic_v5_register_its_iodev(struct kvm *kvm, struct vgic_v5_its *its,
>  	return ret;
>  }
>  
> +static int vgic_v5_its_has_attr_regs(struct kvm_device *dev,
> +				     struct kvm_device_attr *attr)
> +{
> +	const struct vgic_register_region *region;
> +	gpa_t offset = attr->attr;
> +	int align;
> +
> +	region = vgic_find_mmio_region(vgic_v5_its_registers,
> +				       ARRAY_SIZE(vgic_v5_its_registers),
> +				       offset);

[Severity: Low]
In both vgic_v5_its_has_attr_regs() and vgic_v5_its_attr_regs_access(),
offset is a 64-bit gpa_t taken from attr->attr, but vgic_find_mmio_region()
takes a 32-bit unsigned int offset parameter:

const struct vgic_register_region *
vgic_find_mmio_region(const struct vgic_register_region *regions,
		      int nr_regions, unsigned int offset)

If userspace passes an attr->attr value with non-zero upper 32 bits and a
valid lower 32-bit register offset, vgic_find_mmio_region() truncates offset
to 32 bits, the alignment check only tests the low bits, and the register
callbacks mask addr with (SZ_64K - 1).

Should vgic_v5_its_has_attr_regs() and vgic_v5_its_attr_regs_access() reject
offsets outside the ITS register frame before calling vgic_find_mmio_region()?

> +	if (!region)
> +		return -ENXIO;
> +
> +	align = region->access_flags & VGIC_ACCESS_64bit ? 0x7 : 0x3;
> +	if (offset & align)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int vgic_v5_its_attr_regs_access(struct kvm_device *dev,
> +					struct kvm_device_attr *attr,
> +					u64 *reg, bool is_write)
> +{
> +	const struct vgic_register_region *region;
> +	struct vgic_v5_its *its = dev->private;
> +	gpa_t offset = attr->attr;
> +	unsigned int len;
> +	gpa_t addr;
> +	int align;
> +	int ret = 0;
> +
> +	mutex_lock(&dev->kvm->lock);
> +
> +	if (kvm_trylock_all_vcpus(dev->kvm)) {
> +		mutex_unlock(&dev->kvm->lock);
> +		return -EBUSY;
> +	}
> +
> +	mutex_lock(&dev->kvm->arch.config_lock);
> +
> +	if (IS_VGIC_ADDR_UNDEF(its->vgic_v5_its_base)) {
> +		ret = -ENXIO;
> +		goto out;
> +	}
> +
> +	region = vgic_find_mmio_region(vgic_v5_its_registers,
> +				       ARRAY_SIZE(vgic_v5_its_registers),
> +				       offset);
> +	if (!region) {
> +		ret = -ENXIO;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Although the spec supports upper/lower 32-bit accesses to
> +	 * 64-bit ITS registers, the userspace ABI requires 64-bit
> +	 * accesses to all 64-bit wide registers.
> +	 */
> +	align = region->access_flags & VGIC_ACCESS_64bit ? 0x7 : 0x3;
> +	len = region->access_flags & VGIC_ACCESS_64bit ? 8 : 4;

[Severity: Medium]
vgic_v5_its_has_attr_regs() and vgic_v5_its_attr_regs_access() determine the
userspace ABI alignment and access width from region->access_flags &
VGIC_ACCESS_64bit.

In vgic_v5_its_registers[], GICV5_ITS_SYNCR is declared with length 4 and
VGIC_ACCESS_32bit:

	REGISTER_ITS_V5_DESC(GICV5_ITS_SYNCR, vgic_v5_mmio_read_its_raz,
			     vgic_v5_mmio_write_its_dt_itt, 4,
			     VGIC_ACCESS_32bit),

However, in include/linux/irqchip/arm-gic-v5.h, GICV5_ITS_SYNCR is a 64-bit
register at offset 0x0140 with GICV5_ITS_SYNCR_SYNC at BIT_ULL(63) and
GICV5_ITS_SYNCR_SYNCALL at BIT_ULL(32), and the host driver in
drivers/irqchip/irq-gic-v5-its.c writes it with a 64-bit access.

Because its descriptor uses length 4 and VGIC_ACCESS_32bit, userspace
accesses treat GICV5_ITS_SYNCR as a 32-bit register (align 0x3, len 4), and
guest 64-bit MMIO writes or 32-bit writes to GICV5_ITS_SYNCR + 4 fail lookup
in vgic_get_mmio_region(). (Conversely, GICV5_ITS_SWERR_STATUSR,
GICV5_ITS_SWERR_SYNDROMER0, GICV5_ITS_SWERR_SYNDROMER1, and
GICV5_ITS_MPAM_PMGR are declared with VGIC_ACCESS_64bit without
VGIC_ACCESS_32bit.)

Should GICV5_ITS_SYNCR be registered with length 8 and
VGIC_ACCESS_64bit | VGIC_ACCESS_32bit?

> +
> +	if (offset & align) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	addr = its->vgic_v5_its_base + offset;
> +
> +	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);

[Severity: High]
Comparing all 32 register descriptors (covering 43 registers, including the
12 CoreSight PIDR/CIDR registers in the 48-byte GICV5_CORESIGHT_PIDR4 region)
in vgic_v5_its_registers[] between guest MMIO and userspace access in
vgic_v5_its_attr_regs_access():

- 6 descriptors (6 registers: GICV5_ITS_IDR0, GICV5_ITS_IDR1, GICV5_ITS_IDR2,
  GICV5_ITS_DT_BASER, GICV5_ITS_DT_CFGR, GICV5_ITS_READ_EVENT_DATAR) use a
  dedicated uaccess_its_write callback.
- 6 descriptors (6 registers: GICV5_ITS_CR0, GICV5_ITS_CR1, GICV5_ITS_DIDR,
  GICV5_ITS_EIDR, GICV5_ITS_GEN_EVENT_DIDR, GICV5_ITS_GEN_EVENT_EIDR) are RW
  state registers whose userspace writes fall back to region->its_write.
- 15 descriptors (26 registers: GICV5_ITS_IIDR, GICV5_ITS_AIDR,
  GICV5_ITS_STATUSR, GICV5_ITS_SYNC_STATUSR, GICV5_ITS_GEN_EVENT_STATUSR,
  GICV5_ITS_MEC_IDR, GICV5_ITS_MEC_MECID_R, GICV5_ITS_MPAM_IDR,
  GICV5_ITS_MPAM_PARTID_R, GICV5_ITS_SWERR_STATUSR,
  GICV5_ITS_SWERR_SYNDROMER0, GICV5_ITS_SWERR_SYNDROMER1,
  GICV5_ITS_MPAM_PMGR, GICV5_CORESIGHT_DEVARCH, and the 12 registers in
  GICV5_CORESIGHT_PIDR4) use vgic_v5_mmio_write_its_wi().
- 5 descriptors (5 registers: GICV5_ITS_INV_EVENTR, GICV5_ITS_INV_DEVICER,
  GICV5_ITS_READ_EVENTR, GICV5_ITS_SYNCR, GICV5_ITS_GEN_EVENTR) are action
  registers that read as zero via vgic_v5_mmio_read_its_raz() and have no
  uaccess_its_write callback, so userspace writes execute
  vgic_v5_mmio_write_its_dt_itt().

In particular, vgic_v5_its_attr_regs_access() only checks that
its->vgic_v5_its_base is defined; neither it nor the LPI translation and
injection paths (vgic_v5_its_handle_gen_event(), __vgic_v5_its_resolve_lpi(),
vgic_v5_its_inject_msi(), and vgic_v5_its_inject_lpi()) check
vgic_initialized(kvm) or irqchip_in_kernel(kvm):

static void vgic_v5_its_inject_lpi(struct kvm *kvm, struct vgic_irq *irq)
{
	kvm_call_hyp(__vgic_v5_vdpend, irq->intid, true,
		     kvm->arch.vgic.gicv5_vm.vm_id);
}

Because vgic_v5_its_create() allows creating the ITS device before
KVM_DEV_TYPE_ARM_VGIC_V5 is created, kvm->arch.vgic.gicv5_vm.vm_id is 0 from
the zero-allocation of struct kvm (since VGIC_V5_VM_ID_INVAL is only assigned
in kvm_vgic_create(), and vgic_v5_allocate_vm_id() allocates hardware VM IDs
starting from 0). If KVM_DEV_TYPE_ARM_VGIC_V5 has been created but not yet
initialized via KVM_DEV_ARM_VGIC_CTRL_INIT, vm_id is VGIC_V5_VM_ID_INVAL (-1,
truncated to 0xffff).

If userspace enables GICV5_ITS_CR0 via KVM_DEV_ARM_VGIC_GRP_ITS_REGS and
triggers LPI injection via GICV5_ITS_GEN_EVENTR (with GICV5_ITS_GEN_EVENTR_R
set) or KVM_SIGNAL_MSI before the VGICv5 is created or initialized, can this
execute __vgic_v5_vdpend at EL2 with hardware vm_id 0 (injecting LPIs into
another guest VM owning VM ID 0) or with an uninitialized vm_id of 0xffff?

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