Re: [PATCH 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:
- [High] kvm: typed INTID used directly as XArray index in vgic_add_lpi()
- [Medium] kvm: arm64: gic-v5: Guest can bypass VITS_V5_DEVICE_ID_BITS policy cap
- [Low] kvm: compilation failure due to mismatched types in min() macro
--

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

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

The GICv5 ITS consumes guest-owned translation tables. Unlike the
GICv3 ITS, KVM does not maintain a separate software device/table
model for GICv5 translations; the guest's DT and ITTs are the
authoritative source of DeviceID/EventID to LPI mappings.

> diff --git a/arch/arm64/kvm/vgic/vgic-its-v5.c b/arch/arm64/kvm/vgic/vgic-its-v5.c
> --- a/arch/arm64/kvm/vgic/vgic-its-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-its-v5.c
[ ... ]
> +static int get_itt(struct kvm *kvm, struct vgic_v5_its *its, u32 device_id,
> +		   gpa_t *itt_base, bool *two_level_itt,
> +		   unsigned int *event_id_bits, unsigned int *itt_l2sz)
> +{
> +	unsigned int dt_l2sz, device_id_bits;
> +	bool two_level_dt;
> +	gpa_t dt_base;
> +
> +	dt_base = its->dt_baser.addr;
> +	two_level_dt = its->dt_cfgr.structure;
> +	dt_l2sz = its->dt_cfgr.l2sz;
> +	device_id_bits = its->dt_cfgr.device_id_bits;
> +
> +	if (device_id >= 1UL << device_id_bits) {

[Severity: Medium]
Does this check allow the guest to bypass the VITS_V5_DEVICE_ID_BITS policy
cap?

KVM advertises a maximum of 16 device ID bits (VITS_V5_DEVICE_ID_BITS) and a
single-level DT in IDR1. However, when the guest writes to GICV5_ITS_DT_CFGR,
the fields are stored directly in its->dt_cfgr without being clamped to
these policy limits.

Since this out-of-bounds check compares against the un-clamped guest value
instead of the VITS_V5_DEVICE_ID_BITS cap, the guest could configure a
2-level DT and process Device IDs up to 31 bits, bypassing the advertised
policy cap.

> +		kvm_debug("Device ID out of range\n");
> +		return -E2BIG;
> +	}
[ ... ]
> +static int parse_two_level_itt(struct kvm *kvm, u32 event_id, gpa_t itt_base,
> +			       unsigned int itt_l2sz, __le64 *itt_l2_entry)
> +{
[ ... ]
> +	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);

[Severity: Low]
Will this min() macro invocation cause a compilation failure due to mismatched
types?

The arguments to min() are FIELD_GET(GICV5_ITTL1E_SPAN, val) and
itt_split_bits. val is a 64-bit u64, making the FIELD_GET evaluation an
unsigned long long, while itt_split_bits is a 32-bit unsigned int. 

The Linux kernel's min() macro enforces strict type equality and will cause
a build failure when comparing a 64-bit and 32-bit type. Could we use min_t()
here instead to resolve the mismatch?

> +	l2_event_id = event_id % (1UL << l2_event_id_bits);
[ ... ]
> +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)
> +{
[ ... ]
> +	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);

[Severity: High]
Does this pass a fully typed INTID directly to vgic_add_lpi()?

Here, the hardware LPI ID is extracted and converted to a full typed INTID
using vgic_v5_make_lpi(), which sets the GICv5 type bits in the upper nibble.
This typed INTID is passed to vgic_add_lpi(), which then uses it directly
as the array index for the lpi_xa XArray:

arch/arm64/kvm/vgic/vgic.c:vgic_add_lpi() {
    ...
	ret = xa_reserve_irq(&dist->lpi_xa, intid, GFP_KERNEL_ACCOUNT);
    ...
}

Using a typed INTID as an array index violates the GICv5 architectural rule
to strip type bits before indexing. It forces the XArray to allocate
unnecessary depth to reach indices over 1 billion, leading to memory waste
and incorrect type dispatch separation.

> +	if (IS_ERR(*irq))
> +		return PTR_ERR(*irq);
> +
> +	return 0;
> +}

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