Re: [PATCH 01/16] irqchip/riscv-imsic: fix MMIO lookup OOB and NULL cleanup

[Anup Patel]

On Tue, Jul 14, 2026 at 5:54 PM Haofeng Li <lihaofeng@xxxxxxxxxx> wrote:
>
> The MSI page lookup loop uses:
>
>         for (j = 0; nr_mmios; j++)
>
> When nr_mmios is non-zero the condition is always true, so j is never
> bounded. If reloff does not fall in any MMIO region the loop indexes
> past mmios[] and may hang or fault.

It is indeed an ugly typo in for-loop. I guess on most platforms
with AIA the below "if (reloff < resource_size(&mmios[j])) {" takes
care of loop termination but we can't rely on it.

>
> Also, mmios_va starts as NULL. If its allocation fails, the out_iounmap
> path indexes mmios_va[i] and NULL-dereferences.
>
> Bound the loop with j < nr_mmios, and guard the iounmap/kfree cleanup
> with if (mmios_va).
>
> Fixes: 21a8f8a0eb35 ("irqchip: Add RISC-V incoming MSI controller early driver")
> Signed-off-by: Haofeng Li <lihaofeng@xxxxxxxxxx>

LGTM.

Reviewed-by: Anup Patel <anup@xxxxxxxxxxxxxx>

Thanks,
Anup

> ---
>  drivers/irqchip/irq-riscv-imsic-state.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
> index b8d1bbbf42f7..19f74cf79988 100644
> --- a/drivers/irqchip/irq-riscv-imsic-state.c
> +++ b/drivers/irqchip/irq-riscv-imsic-state.c
> @@ -896,7 +896,7 @@ int __init imsic_setup_state(struct fwnode_handle *fwnode, void *opaque)
>                 index = nr_mmios;
>                 reloff = i * BIT(global->guest_index_bits) *
>                          IMSIC_MMIO_PAGE_SZ;
> -               for (j = 0; nr_mmios; j++) {
> +               for (j = 0; j < nr_mmios; j++) {
>                         if (reloff < resource_size(&mmios[j])) {
>                                 index = j;
>                                 break;
> @@ -953,11 +953,13 @@ int __init imsic_setup_state(struct fwnode_handle *fwnode, void *opaque)
>  out_local_cleanup:
>         imsic_local_cleanup();
>  out_iounmap:
> -       for (i = 0; i < nr_mmios; i++) {
> -               if (mmios_va[i])
> -                       iounmap(mmios_va[i]);
> +       if (mmios_va) {
> +               for (i = 0; i < nr_mmios; i++) {
> +                       if (mmios_va[i])
> +                               iounmap(mmios_va[i]);
> +               }
> +               kfree(mmios_va);
>         }
> -       kfree(mmios_va);
>         kfree(mmios);
>  out_free_local:
>         free_percpu(imsic->global.local);
> --
> 2.25.1
>