Re: [PATCH v2 07/16] KVM: arm64: gic-v5: Add ITS guest table translation emulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] KVM: arm64: gic-v5: missing vgic init check in __vgic_v5_its_resolve_lpi()
- [Low] KVM: arm64: gic-v5: unclamped event_id_bits in __vgic_v5_its_resolve_lpi()
- [Low] KVM: arm64: gic-v5: duplicated L2SZ decoding in parse_two_level_itt()
--

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

KVM: arm64: gic-v5: Add ITS guest table translation emulation

This commit adds helpers to walk linear and two-level guest Device Tables
and Interrupt Translation Tables for GICv5 ITS emulation, resolving
DeviceID/EventID pairs to LPIs and wiring up the READ_EVENTR and
GEN_EVENTR MMIO handlers.

> diff --git a/arch/arm64/kvm/vgic/vgic-its-v5.c b/arch/arm64/kvm/vgic/vgic-its-v5.c
> index f04161c315b78..caa68c4680d02 100644
> --- a/arch/arm64/kvm/vgic/vgic-its-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-its-v5.c
> @@ -19,6 +19,302 @@
>  #define VITS_V5_DEVICE_ID_BITS		16
>  #define VITS_V5_EVENT_ID_BITS		16
>  
> +static int parse_linear_dt(struct kvm *kvm, gpa_t dt_base, u32 device_id,
> +			   gpa_t *itt_base, bool *two_level_itt,
> +			   unsigned int *event_id_bits, unsigned int *itt_l2sz)
> +{
> +	__le64 dt_l2_entry;
> +	gpa_t entry_addr;
> +	u64 val;
> +	int ret;
> +
> +	entry_addr = dt_base + device_id * sizeof(__le64);
> +	ret = kvm_read_guest_lock(kvm, entry_addr, &dt_l2_entry,
> +				  sizeof(dt_l2_entry));
> +	if (ret) {
> +		kvm_debug("Failed to read guest memory for L2 DT\n");
> +		return ret;
> +	}
> +
> +	val = le64_to_cpu(dt_l2_entry);
> +	if (!FIELD_GET(GICV5_DTL2E_VALID, val)) {
> +		kvm_debug("Guest DT entry is invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	*itt_base = FIELD_GET(GICV5_DTL2E_ITT_ADDR_MASK, val) << 3;
> +	*two_level_itt = FIELD_GET(GICV5_DTL2E_ITT_STRUCTURE, val);
> +	*event_id_bits = FIELD_GET(GICV5_DTL2E_EVENT_ID_BITS, val);
> +	*itt_l2sz = FIELD_GET(GICV5_DTL2E_ITT_L2SZ, val);
> +
> +	return 0;
> +}
> +
> +static int parse_two_level_dt(struct kvm *kvm, gpa_t dt_base,
> +			      unsigned int dt_l2sz, u32 device_id,
> +			      gpa_t *itt_base, bool *two_level_itt,
> +			      unsigned int *event_id_bits,
> +			      unsigned int *itt_l2sz)
> +{
> +	unsigned int device_id_l1_shift, l2_device_id;
> +	__le64 dt_l1_entry;
> +	gpa_t dt_l2_base;
> +	gpa_t entry_addr;
> +	u64 val;
> +	int ret;
> +
> +	switch (dt_l2sz) {
> +	case 0:
> +		device_id_l1_shift = 9;
> +		break;
> +	case 1:
> +		device_id_l1_shift = 11;
> +		break;
> +	case 2:
> +		device_id_l1_shift = 13;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	entry_addr = dt_base +
> +		     (device_id >> device_id_l1_shift) * sizeof(__le64);
> +
> +	ret = kvm_read_guest_lock(kvm, entry_addr, &dt_l1_entry,
> +				  sizeof(dt_l1_entry));
> +	if (ret) {
> +		kvm_debug("Failed to read guest memory for L1 DT\n");
> +		return ret;
> +	}
> +
> +	val = le64_to_cpu(dt_l1_entry);
> +	if (!FIELD_GET(GICV5_DTL1E_VALID, val)) {
> +		kvm_debug("Guest DT L1 entry is invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	dt_l2_base = FIELD_GET(GICV5_DTL1E_L2_ADDR_MASK, val) << 3;
> +	l2_device_id = device_id % (1UL << device_id_l1_shift);
> +	if (l2_device_id >= (1UL << FIELD_GET(GICV5_DTL1E_SPAN, val))) {
> +		kvm_debug("DeviceID 0x%x out of range of guest DT\n",
> +			  device_id);
> +		return -ENXIO;
> +	}
> +
> +	return parse_linear_dt(kvm, dt_l2_base, l2_device_id, itt_base,
> +			       two_level_itt, event_id_bits, itt_l2sz);
> +}

[ ... ]

> +static int parse_two_level_itt(struct kvm *kvm, u32 event_id, gpa_t itt_base,
> +			       unsigned int itt_l2sz, __le64 *itt_l2_entry)
> +{
> +	unsigned int itt_split_bits, l2_event_id_bits;
> +	gpa_t itt_l2_base, entry_addr;
> +	u32 l1_event_id, l2_event_id;
> +	__le64 itt_l1_entry;
> +	u64 val;
> +	int ret;
> +
> +	switch (itt_l2sz) {
> +	case 0:
> +		itt_split_bits = 9;
> +		break;
> +	case 1:
> +		itt_split_bits = 11;
> +		break;
> +	case 2:
> +		itt_split_bits = 13;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	l1_event_id = event_id >> itt_split_bits;
> +	entry_addr = itt_base + l1_event_id * sizeof(itt_l1_entry);
> +	ret = kvm_read_guest_lock(kvm, entry_addr, &itt_l1_entry,
> +				  sizeof(itt_l1_entry));
> +	if (ret) {
> +		kvm_debug("Failed to read guest memory for L1 ITT\n");
> +		return ret;
> +	}
> +
> +	val = le64_to_cpu(itt_l1_entry);
> +	if (!FIELD_GET(GICV5_ITTL1E_VALID, val)) {
> +		kvm_debug("Guest ITT L1 entry is invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	itt_l2_base = FIELD_GET(GICV5_ITTL1E_L2_ADDR_MASK, val) << 3;
> +	l2_event_id_bits =
> +		min(FIELD_GET(GICV5_ITTL1E_SPAN, val), itt_split_bits);
> +	l2_event_id = event_id % (1UL << l2_event_id_bits);
> +
> +	if ((itt_split_bits != l2_event_id_bits) &&
> +	    (event_id & ((1UL << itt_split_bits) - 1)) != l2_event_id) {
> +		kvm_debug("EventID is out of range of L2 ITT\n");
> +		return -ENXIO;
> +	}
> +
> +	return parse_linear_itt(kvm, l2_event_id, itt_l2_base, itt_l2_entry);
> +}

[Severity: Low]
This isn't a bug, but both parse_two_level_dt() and parse_two_level_itt()
duplicate the same switch statement mapping L2SZ encodings (0, 1, 2) to bit
shifts (9, 11, 13).

For validating the L2 index against the L1 entry's SPAN field,
parse_two_level_dt() computes l2_device_id = device_id % (1UL <<
device_id_l1_shift) and directly checks l2_device_id >= (1UL <<
FIELD_GET(GICV5_DTL1E_SPAN, val)), whereas parse_two_level_itt() clamps
l2_event_id_bits with min() and compares (event_id & ((1UL << itt_split_bits)
- 1)) against l2_event_id.

Could the L2SZ-to-shift conversion be factored into a small helper, and could
parse_two_level_itt() use the same direct SPAN bounds check as
parse_two_level_dt()?

[ ... ]

> +static int __vgic_v5_its_resolve_lpi(struct kvm *kvm, struct vgic_v5_its *its,
> +				     u32 device_id, u16 event_id,
> +				     struct vgic_irq **irq)
> +{
> +	unsigned int itt_l2sz, event_id_bits;
> +	__le64 itt_l2_entry;
> +	bool two_level_itt;
> +	gpa_t itt_base;
> +	u32 lpi;
> +	u64 val;
> +	int ret;
> +
> +	lockdep_assert_held(&its->its_lock);
> +
> +	if (!its->enabled) {
> +		kvm_debug("Cannot look up LPI; vITS is disabled\n");
> +		return -ENXIO;
> +	}
> +
> +	if (WARN_ON_ONCE(kvm->mm != get_current()->mm))
> +		return -EWOULDBLOCK;
> +
> +	ret = get_itt(kvm, its, device_id, &itt_base, &two_level_itt,
> +		      &event_id_bits, &itt_l2sz);
> +	if (ret) {
> +		kvm_debug("Error parsing guest's DT\n");
> +		return ret;
> +	}
> +
> +	if (event_id >= 1UL << event_id_bits) {
> +		kvm_debug("Event ID out of range\n");
> +		return -E2BIG;
> +	}

[Severity: Low]
In get_itt(), the guest-configured DeviceID width is clamped to the maximum
supported by the ITS in IDR1 via min(its->dt_cfgr.device_id_bits,
its->idr1.device_id_bits).

In parse_linear_dt(), *event_id_bits = FIELD_GET(GICV5_DTL2E_EVENT_ID_BITS,
val) extracts the 5-bit EVENT_ID_BITS field (0..31) from the guest DTL2E, and
__vgic_v5_its_resolve_lpi() checks event_id >= 1UL << event_id_bits without
clamping event_id_bits to or validating it against its->idr2.event_id_bits.

This rests on a GICv5 architectural detail that could not be confirmed from
the kernel source: when GICV5_DTL2E_EVENT_ID_BITS exceeds
GICV5_ITS_IDR2_EVENTID_BITS, does the architecture require clamping the
effective EventID width to ITS_IDR2.EVENTID_BITS or treating the DTL2E as
invalid? If yes, parse_linear_dt() and __vgic_v5_its_resolve_lpi() will accept
an oversized DTL2E.EVENT_ID_BITS without capping or rejecting it when
its->idr2.event_id_bits is smaller than DTL2E.EVENT_ID_BITS.

> +
> +	ret = get_itte(kvm, event_id, itt_base, two_level_itt, itt_l2sz,
> +		       &itt_l2_entry);
> +	if (ret)
> +		return ret;
> +
> +	val = le64_to_cpu(itt_l2_entry);
> +	if (!FIELD_GET(GICV5_ITTL2E_VALID, val)) {
> +		kvm_debug("ITTE is not valid\n");
> +		return -EINVAL;
> +	}
> +
> +	lpi = FIELD_GET(GICV5_ITTL2E_LPI_ID, val);
> +
> +	*irq = vgic_add_lpi(kvm, vgic_v5_make_lpi(lpi), NULL);
> +	if (IS_ERR(*irq))
> +		return PTR_ERR(*irq);
> +
> +	return 0;
> +}
> +
> +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);
> +}

[Severity: Critical]
What happens if userspace creates a KVM_DEV_TYPE_ARM_VGIC_V5_ITS device, sets
KVM_VGIC_V5_ADDR_TYPE_ITS, and runs a vCPU without ever creating a
KVM_DEV_TYPE_ARM_VGIC_V5 irqchip device?

In vgic_v5_its_create(), creating the ITS device is permitted when
irqchip_in_kernel(dev->kvm) is false:

arch/arm64/kvm/vgic/vgic-its-v5.c:vgic_v5_its_create() {
	...
	if ((irqchip_in_kernel(dev->kvm) && !vgic_is_v5(dev->kvm)) ||
	    (dev->kvm->arch.vgic.its_model &&
	     dev->kvm->arch.vgic.its_model != KVM_DEV_TYPE_ARM_VGIC_V5)) {
		kfree(its);
		return -ENODEV;
	}
	...
}

Setting KVM_VGIC_V5_ADDR_TYPE_ITS via vgic_v5_its_set_attr() immediately
registers the ITS MMIO device on KVM_MMIO_BUS via
vgic_v5_register_its_iodev(). When userspace runs a vCPU via KVM_RUN,
kvm_arch_vcpu_run_pid_change() only calls kvm_vgic_map_resources(kvm) when
irqchip_in_kernel(kvm) is true:

arch/arm64/kvm/arm.c:kvm_arch_vcpu_run_pid_change() {
	...
	if (likely(irqchip_in_kernel(kvm))) {
		/*
		 * Map the VGIC hardware resources before running a vcpu the
		 * first time on this VM.
		 */
		ret = kvm_vgic_map_resources(kvm);
		if (ret)
			return ret;
	}
	...
}

Because kvm_vgic_create() was never called to initialize
kvm->arch.vgic.gicv5_vm.vm_id to VGIC_V5_VM_ID_INVAL (and
vgic_v5_allocate_vm_id() was never called), kvm->arch.vgic.gicv5_vm.vm_id
remains 0 from the zero-initialized struct kvm allocation.

When the guest enables the vITS via GICV5_ITS_CR0 and writes
GICV5_ITS_GEN_EVENTR_R to GICV5_ITS_GEN_EVENTR,
vgic_v5_its_handle_gen_event() calls __vgic_v5_its_resolve_lpi() and
vgic_v5_its_inject_lpi(). Neither function checks vgic_initialized(kvm) or
vgic_has_its(kvm), so __vgic_v5_its_resolve_lpi() allocates LPIs in lpi_xa and
vgic_v5_its_inject_lpi() executes kvm_call_hyp(__vgic_v5_vdpend, irq->intid,
true, kvm->arch.vgic.gicv5_vm.vm_id) with vm_id == 0.

Since vgic_v5_allocate_vm_id() allocates valid hardware VM IDs from
vmt_info->vm_id_ida starting at 0, won't the EL2 VDPEND instruction inject
guest-controlled LPIs directly into whichever active GICv5 VM on the host owns
hardware VM ID 0? Should __vgic_v5_its_resolve_lpi() (or vgic_v5_its_create())
verify that the in-kernel GICv5 irqchip is initialized before resolving and
injecting LPIs?

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