Re: [PATCH v5 1/2] irqchip/riscv-imsic: Add nr_guest_files in per-HART local config

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The loop searching for the correct MMIO location uses `nr_mmios` as its continuation condition instead of `j < nr_mmios`. This acts as an infinite loop if `nr_mmios > 0` and can cause an out-of-bounds read on the `mmios` array.
- [High] The calculation of `nr_guest_files` can silently underflow to `0xFFFFFFFF` if the remaining MMIO resource size is smaller than a single page, leading to an incorrectly maximized guest file count.
--

commit 15182effc09715c1274ad73e7c779ccf2a70e30d
Author: Guo Ren (Alibaba DAMO Academy) <guoren@kernel.org>

irqchip/riscv-imsic: Add nr_guest_files in per-HART local config

This commit adds nr_guest_files to the per-HART local config to represent
the number of guest files available on a particular HART. This allows KVM
to use per-HART configurations for asymmetric big.Little systems instead
of relying on a global minimum.

> diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
> index e3ed874d89e73..b8d1bbbf42f73 100644
> --- a/drivers/irqchip/irq-riscv-imsic-state.c
> +++ b/drivers/irqchip/irq-riscv-imsic-state.c
> @@ -920,13 +920,12 @@ int __init imsic_setup_state(struct fwnode_handle *fwnode, void *opaque)
>  		local->msi_va = mmios_va[index] + reloff;

[Severity: High]
This is a pre-existing issue, but looking at the code just above this hunk
in imsic_setup_state(), the loop searching for the correct MMIO location
uses nr_mmios as its continuation condition:

	for (j = 0; nr_mmios; j++) {
		if (reloff < resource_size(&mmios[j])) {
			...

Since nr_mmios is a constant integer greater than 0, this acts as an
unconditional loop. If the condition is never satisfied, could j increment
past nr_mmios and cause an out-of-bounds read on the dynamically allocated
mmios array?

>  
>  		/*
> -		 * KVM uses global->nr_guest_files to determine the available guest
> -		 * interrupt files on each CPU. Take the minimum number of guest
> -		 * interrupt files across all CPUs to avoid KVM incorrectly allocating
> -		 * an unexisted or unmapped guest interrupt file on some CPUs.
> +		 * KVM uses both local->nr_guest_files and global->nr_guest_files
> +		 * to determine the available guest interrupt files on each CPU.
>  		 */
>  		nr_guest_files = (resource_size(&mmios[index]) - reloff) / IMSIC_MMIO_PAGE_SZ - 1;

[Severity: High]
This is a pre-existing issue, but can this calculation silently underflow to
0xFFFFFFFF if the remaining MMIO resource size is smaller than a single
page?

If the remaining resource size is strictly greater than 0 but less than
IMSIC_MMIO_PAGE_SZ, the division yields 0. Subtracting 1 from 0 causes an
unsigned underflow.

Because min() is used in the following lines, it compares both as unsigned
and selects the maximum allowed guest files instead of 0. Could this
artificially inflate the guest file count and lead KVM to make out-of-bounds
MMIO accesses?

> -		global->nr_guest_files = min(global->nr_guest_files, nr_guest_files);
> +		local->nr_guest_files = min((BIT(global->guest_index_bits) - 1), nr_guest_files);
> +		global->nr_guest_files = min(global->nr_guest_files, local->nr_guest_files);
>  
>  		nr_handlers++;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525094945.3721783-1-anup.patel@oss.qualcomm.com?part=1