* [PATCH 0/2] irqchip/riscv-imsic: Validate group index geometry
@ 2026-09-06 8:07 Pengpeng Hou
2026-09-06 8:08 ` [PATCH 1/2] irqchip/riscv-imsic: Ignore unused group index shifts Pengpeng Hou
2026-09-06 8:09 ` [PATCH 2/2] irqchip/riscv-imsic: Reject overlapping group index fields Pengpeng Hou
0 siblings, 2 replies; 5+ messages in thread
From: Pengpeng Hou @ 2026-09-06 8:07 UTC (permalink / raw)
To: Anup Patel, Thomas Gleixner
Cc: Pengpeng Hou, Radu Rendec, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, linux-riscv, linux-kernel,
Andrew Jones, Bjorn Topel
This series handles absent and overlapping IMSIC group fields.
Patch 1 skips group-shift validation and masking when the group width is
zero, including the APLIC consumer. Patch 2 rejects nonzero fields that
overlap the lower address fields and makes the upper-bound check safe.
The series is based on tip/irq/drivers, which already contains
bd7d57a03c4b ("irqchip/riscv-imsic: Use GENMASK for base address masks").
That prerequisite is not included again.
The two patches have not previously been submitted. The earlier local
draft versions do not constitute public revisions.
The issues were found by our static-analysis tool and manually reviewed.
Assisted-by: GPT-5
Pengpeng Hou (2):
irqchip/riscv-imsic: Ignore unused group index shifts
irqchip/riscv-imsic: Reject overlapping group index fields
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/2] irqchip/riscv-imsic: Ignore unused group index shifts 2026-09-06 8:07 [PATCH 0/2] irqchip/riscv-imsic: Validate group index geometry Pengpeng Hou @ 2026-09-06 8:08 ` Pengpeng Hou 2026-09-06 9:26 ` Anup Patel 2026-09-06 8:09 ` [PATCH 2/2] irqchip/riscv-imsic: Reject overlapping group index fields Pengpeng Hou 1 sibling, 1 reply; 5+ messages in thread From: Pengpeng Hou @ 2026-09-06 8:08 UTC (permalink / raw) To: Anup Patel, Thomas Gleixner Cc: Pengpeng Hou, Radu Rendec, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, linux-riscv, linux-kernel, Andrew Jones, Bjorn Topel The IMSIC parser validates and uses group_index_shift even when the group index has zero width. The binding permits a zero-width group with shift zero, but bits + shift - 1 underflows and rejects that layout. APLIC MSI setup also validates HHXS although HHXW is zero. Only validate and clear group-address bits when the group width is nonzero. Use zero for APLIC HHXS when there is no group field. This also avoids shifting an empty mask by an otherwise unused shift value. The issue was found by our static-analysis tool and manually reviewed. Fixes: 21a8f8a0eb35 ("irqchip: Add RISC-V incoming MSI controller early driver") Assisted-by: GPT-5 Signed-off-by: Pengpeng Hou <hppiscas@163.com> --- diff --git a/drivers/irqchip/irq-riscv-aplic-msi.c b/drivers/irqchip/irq-riscv-aplic-msi.c index fb8d1838..569b3ff4 100644 --- a/drivers/irqchip/irq-riscv-aplic-msi.c +++ b/drivers/irqchip/irq-riscv-aplic-msi.c @@ -223,17 +223,21 @@ int aplic_msi_setup(struct device *dev, void __iomem *regs) return -EINVAL; } - /* Find first bit position of group index (HHXS) */ - mc->hhxs = imsic_global->group_index_shift; - if (mc->hhxs < (2 * APLIC_xMSICFGADDR_PPN_SHIFT)) { - dev_err(dev, "IMSIC group index shift should be >= %d\n", - (2 * APLIC_xMSICFGADDR_PPN_SHIFT)); - return -EINVAL; - } - mc->hhxs -= (2 * APLIC_xMSICFGADDR_PPN_SHIFT); - if (APLIC_xMSICFGADDRH_HHXS_MASK < mc->hhxs) { - dev_err(dev, "IMSIC group index shift big for APLIC HHXS\n"); - return -EINVAL; + if (mc->hhxw) { + /* Find first bit position of group index (HHXS) */ + mc->hhxs = imsic_global->group_index_shift; + if (mc->hhxs < (2 * APLIC_xMSICFGADDR_PPN_SHIFT)) { + dev_err(dev, "IMSIC group index shift should be >= %d\n", + (2 * APLIC_xMSICFGADDR_PPN_SHIFT)); + return -EINVAL; + } + mc->hhxs -= (2 * APLIC_xMSICFGADDR_PPN_SHIFT); + if (APLIC_xMSICFGADDRH_HHXS_MASK < mc->hhxs) { + dev_err(dev, "IMSIC group index shift big for APLIC HHXS\n"); + return -EINVAL; + } + } else { + mc->hhxs = 0; } /* Compute PPN base */ diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c index 9505ddbd..44b9c34c 100644 --- a/drivers/irqchip/irq-riscv-imsic-state.c +++ b/drivers/irqchip/irq-riscv-imsic-state.c @@ -741,10 +741,12 @@ static int __init imsic_parse_fwnode(struct fwnode_handle *fwnode, } /* Sanity check group index shift */ - i = global->group_index_bits + global->group_index_shift - 1; - if (i >= BITS_PER_LONG) { - pr_err("%pfwP: group index shift too big\n", fwnode); - return -EINVAL; + if (global->group_index_bits) { + i = global->group_index_bits + global->group_index_shift - 1; + if (i >= BITS_PER_LONG) { + pr_err("%pfwP: group index shift too big\n", fwnode); + return -EINVAL; + } } /* Sanity check number of interrupt identities */ @@ -773,8 +775,9 @@ static int __init imsic_parse_fwnode(struct fwnode_handle *fwnode, global->base_addr &= ~GENMASK(global->guest_index_bits + global->hart_index_bits + IMSIC_MMIO_PAGE_SHIFT - 1, 0); - global->base_addr &= ~((BIT(global->group_index_bits) - 1) << - global->group_index_shift); + if (global->group_index_bits) + global->base_addr &= ~((BIT(global->group_index_bits) - 1) << + global->group_index_shift); /* Find number of MMIO register sets */ while (!imsic_get_mmio_resource(fwnode, *nr_mmios, &res)) @@ -854,8 +857,9 @@ int __init imsic_setup_state(struct fwnode_handle *fwnode, void *opaque) base_addr &= ~GENMASK(global->guest_index_bits + global->hart_index_bits + IMSIC_MMIO_PAGE_SHIFT - 1, 0); - base_addr &= ~((BIT(global->group_index_bits) - 1) << - global->group_index_shift); + if (global->group_index_bits) + base_addr &= ~((BIT(global->group_index_bits) - 1) << + global->group_index_shift); if (base_addr != global->base_addr) { rc = -EINVAL; pr_err("%pfwP: address mismatch for regset %d\n", fwnode, i); base-commit: 13ca1c0b7d8a3ab1e59cb3e780c1dd7441e22515 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] irqchip/riscv-imsic: Ignore unused group index shifts 2026-09-06 8:08 ` [PATCH 1/2] irqchip/riscv-imsic: Ignore unused group index shifts Pengpeng Hou @ 2026-09-06 9:26 ` Anup Patel 0 siblings, 0 replies; 5+ messages in thread From: Anup Patel @ 2026-09-06 9:26 UTC (permalink / raw) To: Pengpeng Hou Cc: Thomas Gleixner, Radu Rendec, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, linux-riscv, linux-kernel, Andrew Jones, Bjorn Topel On Sun, Sep 6, 2026 at 1:38 PM Pengpeng Hou <hppiscas@163.com> wrote: > > The IMSIC parser validates and uses group_index_shift even when the > group index has zero width. The binding permits a zero-width group with > shift zero, but bits + shift - 1 underflows and rejects that layout. > APLIC MSI setup also validates HHXS although HHXW is zero. > > Only validate and clear group-address bits when the group width is > nonzero. Use zero for APLIC HHXS when there is no group field. This also > avoids shifting an empty mask by an otherwise unused shift value. > > The issue was found by our static-analysis tool and manually reviewed. > > Fixes: 21a8f8a0eb35 ("irqchip: Add RISC-V incoming MSI controller early driver") > Assisted-by: GPT-5 > Signed-off-by: Pengpeng Hou <hppiscas@163.com> > --- > > diff --git a/drivers/irqchip/irq-riscv-aplic-msi.c b/drivers/irqchip/irq-riscv-aplic-msi.c > index fb8d1838..569b3ff4 100644 > --- a/drivers/irqchip/irq-riscv-aplic-msi.c > +++ b/drivers/irqchip/irq-riscv-aplic-msi.c > @@ -223,17 +223,21 @@ int aplic_msi_setup(struct device *dev, void __iomem *regs) > return -EINVAL; > } > > - /* Find first bit position of group index (HHXS) */ > - mc->hhxs = imsic_global->group_index_shift; > - if (mc->hhxs < (2 * APLIC_xMSICFGADDR_PPN_SHIFT)) { > - dev_err(dev, "IMSIC group index shift should be >= %d\n", > - (2 * APLIC_xMSICFGADDR_PPN_SHIFT)); > - return -EINVAL; > - } > - mc->hhxs -= (2 * APLIC_xMSICFGADDR_PPN_SHIFT); > - if (APLIC_xMSICFGADDRH_HHXS_MASK < mc->hhxs) { > - dev_err(dev, "IMSIC group index shift big for APLIC HHXS\n"); > - return -EINVAL; > + if (mc->hhxw) { > + /* Find first bit position of group index (HHXS) */ > + mc->hhxs = imsic_global->group_index_shift; > + if (mc->hhxs < (2 * APLIC_xMSICFGADDR_PPN_SHIFT)) { > + dev_err(dev, "IMSIC group index shift should be >= %d\n", > + (2 * APLIC_xMSICFGADDR_PPN_SHIFT)); > + return -EINVAL; > + } > + mc->hhxs -= (2 * APLIC_xMSICFGADDR_PPN_SHIFT); > + if (APLIC_xMSICFGADDRH_HHXS_MASK < mc->hhxs) { > + dev_err(dev, "IMSIC group index shift big for APLIC HHXS\n"); > + return -EINVAL; > + } > + } else { > + mc->hhxs = 0; The IMSIC driver already ensures that group_index_shift is atleast 24 irrespective to the value of group_index_bits so this change is not needed because in the "bits + shift - 1" the shift is always >= 24. > } > > /* Compute PPN base */ > diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c > index 9505ddbd..44b9c34c 100644 > --- a/drivers/irqchip/irq-riscv-imsic-state.c > +++ b/drivers/irqchip/irq-riscv-imsic-state.c > @@ -741,10 +741,12 @@ static int __init imsic_parse_fwnode(struct fwnode_handle *fwnode, > } > > /* Sanity check group index shift */ > - i = global->group_index_bits + global->group_index_shift - 1; > - if (i >= BITS_PER_LONG) { > - pr_err("%pfwP: group index shift too big\n", fwnode); > - return -EINVAL; > + if (global->group_index_bits) { > + i = global->group_index_bits + global->group_index_shift - 1; > + if (i >= BITS_PER_LONG) { > + pr_err("%pfwP: group index shift too big\n", fwnode); > + return -EINVAL; > + } > } > > /* Sanity check number of interrupt identities */ > @@ -773,8 +775,9 @@ static int __init imsic_parse_fwnode(struct fwnode_handle *fwnode, > global->base_addr &= ~GENMASK(global->guest_index_bits + > global->hart_index_bits + > IMSIC_MMIO_PAGE_SHIFT - 1, 0); > - global->base_addr &= ~((BIT(global->group_index_bits) - 1) << > - global->group_index_shift); > + if (global->group_index_bits) > + global->base_addr &= ~((BIT(global->group_index_bits) - 1) << > + global->group_index_shift); There is nothing to be fixed here. For group_index_bits == 0, BIT(group_index_bits) - 1 will be zero and 0 << any_shift_value will be 0. In other words, global->base_addr won't change when group_index_bits == 0. > > /* Find number of MMIO register sets */ > while (!imsic_get_mmio_resource(fwnode, *nr_mmios, &res)) > @@ -854,8 +857,9 @@ int __init imsic_setup_state(struct fwnode_handle *fwnode, void *opaque) > base_addr &= ~GENMASK(global->guest_index_bits + > global->hart_index_bits + > IMSIC_MMIO_PAGE_SHIFT - 1, 0); > - base_addr &= ~((BIT(global->group_index_bits) - 1) << > - global->group_index_shift); > + if (global->group_index_bits) > + base_addr &= ~((BIT(global->group_index_bits) - 1) << > + global->group_index_shift); Same comment as above. > if (base_addr != global->base_addr) { > rc = -EINVAL; > pr_err("%pfwP: address mismatch for regset %d\n", fwnode, i); > > base-commit: 13ca1c0b7d8a3ab1e59cb3e780c1dd7441e22515 > Overall, this patch fixes nothing. Regards, Anup ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] irqchip/riscv-imsic: Reject overlapping group index fields 2026-09-06 8:07 [PATCH 0/2] irqchip/riscv-imsic: Validate group index geometry Pengpeng Hou 2026-09-06 8:08 ` [PATCH 1/2] irqchip/riscv-imsic: Ignore unused group index shifts Pengpeng Hou @ 2026-09-06 8:09 ` Pengpeng Hou 2026-09-06 15:12 ` Anup Patel 1 sibling, 1 reply; 5+ messages in thread From: Pengpeng Hou @ 2026-09-06 8:09 UTC (permalink / raw) To: Anup Patel, Thomas Gleixner Cc: Pengpeng Hou, Radu Rendec, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, linux-riscv, linux-kernel, Andrew Jones, Bjorn Topel The IMSIC parser validates index widths but does not check that a nonzero group field starts above the page, guest and hart fields. A malformed layout can therefore assign one address bit to two fields. Require the group shift to be at least the end of the lower fields. Check the upper bound using subtraction after validating the shift, so the bound check cannot wrap when processing a firmware-supplied value. The issue was found by our static-analysis tool and manually reviewed. Fixes: 21a8f8a0eb35 ("irqchip: Add RISC-V incoming MSI controller early driver") Assisted-by: GPT-5 Signed-off-by: Pengpeng Hou <hppiscas@163.com> --- diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c index 44b9c34c..c6eb16ae 100644 --- a/drivers/irqchip/irq-riscv-imsic-state.c +++ b/drivers/irqchip/irq-riscv-imsic-state.c @@ -742,8 +742,16 @@ static int __init imsic_parse_fwnode(struct fwnode_handle *fwnode, /* Sanity check group index shift */ if (global->group_index_bits) { - i = global->group_index_bits + global->group_index_shift - 1; - if (i >= BITS_PER_LONG) { + i = IMSIC_MMIO_PAGE_SHIFT + global->guest_index_bits + + global->hart_index_bits; + if (global->group_index_shift < i) { + pr_err("%pfwP: group index shift too small\n", fwnode); + return -EINVAL; + } + + if (global->group_index_shift >= BITS_PER_LONG || + global->group_index_bits > + BITS_PER_LONG - global->group_index_shift) { pr_err("%pfwP: group index shift too big\n", fwnode); return -EINVAL; } base-commit: 13ca1c0b7d8a3ab1e59cb3e780c1dd7441e22515 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] irqchip/riscv-imsic: Reject overlapping group index fields 2026-09-06 8:09 ` [PATCH 2/2] irqchip/riscv-imsic: Reject overlapping group index fields Pengpeng Hou @ 2026-09-06 15:12 ` Anup Patel 0 siblings, 0 replies; 5+ messages in thread From: Anup Patel @ 2026-09-06 15:12 UTC (permalink / raw) To: Pengpeng Hou Cc: Thomas Gleixner, Radu Rendec, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, linux-riscv, linux-kernel, Andrew Jones, Bjorn Topel On Sun, Sep 6, 2026 at 1:39 PM Pengpeng Hou <hppiscas@163.com> wrote: > > The IMSIC parser validates index widths but does not check that a > nonzero group field starts above the page, guest and hart fields. > A malformed layout can therefore assign one address bit to two fields. > > Require the group shift to be at least the end of the lower fields. > Check the upper bound using subtraction after validating the shift, so > the bound check cannot wrap when processing a firmware-supplied value. > > The issue was found by our static-analysis tool and manually reviewed. > > Fixes: 21a8f8a0eb35 ("irqchip: Add RISC-V incoming MSI controller early driver") > Assisted-by: GPT-5 > Signed-off-by: Pengpeng Hou <hppiscas@163.com> > --- > > diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c > index 44b9c34c..c6eb16ae 100644 > --- a/drivers/irqchip/irq-riscv-imsic-state.c > +++ b/drivers/irqchip/irq-riscv-imsic-state.c > @@ -742,8 +742,16 @@ static int __init imsic_parse_fwnode(struct fwnode_handle *fwnode, > > /* Sanity check group index shift */ > if (global->group_index_bits) { > - i = global->group_index_bits + global->group_index_shift - 1; > - if (i >= BITS_PER_LONG) { > + i = IMSIC_MMIO_PAGE_SHIFT + global->guest_index_bits + > + global->hart_index_bits; > + if (global->group_index_shift < i) { > + pr_err("%pfwP: group index shift too small\n", fwnode); > + return -EINVAL; > + } > + > + if (global->group_index_shift >= BITS_PER_LONG || > + global->group_index_bits > > + BITS_PER_LONG - global->group_index_shift) { The above check for "too small group index shift" is fine but why is the existing check for "too big group index shift" not sufficient. > pr_err("%pfwP: group index shift too big\n", fwnode); > return -EINVAL; > } > > base-commit: 13ca1c0b7d8a3ab1e59cb3e780c1dd7441e22515 > Regards, Anup ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-09-06 15:12 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-09-06 8:07 [PATCH 0/2] irqchip/riscv-imsic: Validate group index geometry Pengpeng Hou 2026-09-06 8:08 ` [PATCH 1/2] irqchip/riscv-imsic: Ignore unused group index shifts Pengpeng Hou 2026-09-06 9:26 ` Anup Patel 2026-09-06 8:09 ` [PATCH 2/2] irqchip/riscv-imsic: Reject overlapping group index fields Pengpeng Hou 2026-09-06 15:12 ` Anup Patel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®