Re: [PATCH] KVM: selftests: arm64: Add test for cross-vCPU LPI disable race

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] kvm: selftests: buffer overflow in configure_lpis() via large event_ids
- [Medium] kvm: selftests: ITS command error due to INVALL/SYNC on unmapped collection
- [Medium] kvm: selftests: silently ignored MSI injection failures in signal_lpi()
--

commit c5682a23bdbe7c62a7e95d362e2d340ff25c1e42
Author: Yuchao Zhang <ndaugoing@gmail.com>

KVM: selftests: arm64: Add test for cross-vCPU LPI disable race

This commit adds a selftest to validate the behavior of remote LPI
disabling while the target vCPU has in-flight or overflowing LPIs. It
configures an ITS with multiple LPIs targeting vCPU 0, while vCPU 1
toggles GICR_CTLR.EnableLPIs.

> diff --git a/tools/testing/selftests/kvm/arm64/vgic_lpi_disable.c b/tools/testing/selftests/kvm/arm64/vgic_lpi_disable.c
> new file mode 100644
> index 0000000000000..0781349192288
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/arm64/vgic_lpi_disable.c
> @@ -0,0 +1,401 @@
[ ... ]
> +static void guest_setup_its_mappings(void)
> +{
> +	u32 device_id, event_id, intid = GIC_LPI_OFFSET;
> +	u32 nr_events = test_data.nr_event_ids;
> +	u32 nr_devices = test_data.nr_devices;
> +
> +	/* Map collection 0 to TARGET_VCPU_ID */
> +	its_send_mapc_cmd(test_data.cmdq_base_va, TARGET_VCPU_ID, TARGET_VCPU_ID, true);
> +
> +	/* Map all LPIs to TARGET_VCPU_ID to force LR overflow */
[ ... ]
> +static void guest_invalidate_all_rdists(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < test_data.nr_cpus; i++)
> +		its_send_invall_cmd(test_data.cmdq_base_va, i);
> +}

[Severity: Medium]
Will sending an INVALL command for CPU ID 1 act as a command error since
collection 1 was never mapped? 

In guest_setup_its_mappings(), only collection 0 is mapped to TARGET_VCPU_ID,
but this loop iterates over all CPUs (collections 0 and 1) and sends INVALL.

[ ... ]
> +static void guest_setup_gic(void)
> +{
[ ... ]
> +	guest_setup_its_mappings();
> +	guest_invalidate_all_rdists();
> +
> +	/* SYNC to ensure ITS setup is complete */
> +	for (cpuid = 0; cpuid < test_data.nr_cpus; cpuid++)
> +		its_send_sync_cmd(test_data.cmdq_base_va, cpuid);
> +}

[Severity: Medium]
Similarly, does issuing a SYNC command for cpuid 1 cause an architectural
command error here, given that collection 1 remains unmapped? 

This might cause the virtual ITS command queue to stall.

[ ... ]
> +static void configure_lpis(void)
> +{
> +	size_t nr_lpis = test_data.nr_devices * test_data.nr_event_ids;
> +	u8 *tbl = addr_gpa2hva(vm, test_data.lpi_prop_table);
> +	size_t i;
> +
> +	for (i = 0; i < nr_lpis; i++) {
> +		tbl[i] = LPI_PROP_DEFAULT_PRIO |
> +			 LPI_PROP_GROUP1 |
> +			 LPI_PROP_ENABLED;
> +	}
> +}

[Severity: Medium]
Can this loop overflow the lpi_prop_table allocation? 

The table is allocated a fixed 64KB size in setup_test_data(), but
test_data.nr_event_ids is read directly from the command line argument in
main() via atoi_positive(). A large user input could cause an out-of-bounds
write here since the value isn't bounds-checked to fit within the region.

[ ... ]
> +static void signal_lpi(u32 device_id, u32 event_id)
> +{
> +	gpa_t db_addr = GITS_BASE_GPA + GITS_TRANSLATER;
> +
> +	struct kvm_msi msi = {
> +		.address_lo	= db_addr,
> +		.address_hi	= db_addr >> 32,
> +		.data		= event_id,
> +		.devid		= device_id,
> +		.flags		= KVM_MSI_VALID_DEVID,
> +	};
> +
> +	__vm_ioctl(vm, KVM_SIGNAL_MSI, &msi);
> +}

[Severity: Medium]
Is it safe to ignore the return value of __vm_ioctl() here? 

If KVM_SIGNAL_MSI fails (for example, if the ITS command queue stalls due
to the unmapped collection mentioned earlier), the failure is silently masked. 
This could cause the test to spin without triggering the intended overflow
path, falsely reporting success on a broken test environment.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922101632.39497-1-ndaugoing@gmail.com?part=1