Re: [PATCH] dmaengine: nbpfaxi: Fix setting channel irqs in probe()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces a heap buffer overflow and uninitialized memory read when assigning IRQs, which triggers if the error IRQ (`eirq`) is the last element in `irqbuf`.

Pre-existing issues:
- [High] The probe function contains an infinite loop if the platform device is configured with fewer than 9 IRQs.
- [High] In the error path of `nbpf_desc_page_alloc()`, a memory page is leaked and the wrong size is passed to `dma_unmap_single()`.
- [High] The device's shared IRQs are requested before its clock is enabled, potentially causing kernel panics (data aborts) if an interrupt triggers early.
--

commit ece6c7ef191ef5f893ded32908010de168eac2be
Author: Christian Taedcke <christian.taedcke@weidmueller.com>

dmaengine: nbpfaxi: Fix setting channel irqs in probe()

This commit updates the loop iterating over channel irqs in the nbpfaxi driver
to iterate over all collected irqs instead of the channel count. This ensures
that skipping the error irq does not lead to the last channel being left
without an assigned irq.

> diff --git a/drivers/dma/nbpfaxi.c b/drivers/dma/nbpfaxi.c
> index 05d7321629cc8..74ff7bd979e26 100644
> --- a/drivers/dma/nbpfaxi.c
> +++ b/drivers/dma/nbpfaxi.c
> @@ -1374,7 +1374,7 @@ static int nbpf_probe(struct platform_device *pdev)
>  		if (irqs == num_channels + 1) {
>  			struct nbpf_channel *chan;
>  
> -			for (i = 0, chan = nbpf->chan; i < num_channels;
> +			for (i = 0, chan = nbpf->chan; i < irqs;
>  			     i++, chan++) {
>  				/* Skip the error IRQ */
>  				if (irqbuf[i] == eirq)

[Severity: High]
Does this loop write past the end of the nbpf->chan array if the error irq
happens to be the last element in irqbuf?

With the new loop condition i < irqs, chan is advanced unconditionally on
every iteration.

If the platform provides the error irq as the final entry, on the last
iteration chan will point past the end of the dynamically allocated
nbpf->chan array.

The code then increments i to skip the error irq and writes from
out-of-bounds stack memory at irqbuf[i] into the out-of-bounds chan->irq
pointer.


While reviewing the above, I also noticed a few pre-existing issues in this
driver.

drivers/dma/nbpfaxi.c:nbpf_probe() {
[ ... ]
	for (i = 0; irqs < ARRAY_SIZE(irqbuf); i++) {
		irq = platform_get_irq_optional(pdev, i);
		if (irq < 0 && irq != -ENXIO)
			return irq;
		if (irq > 0)
			irqbuf[irqs++] = irq;
	}
[ ... ]
}

[Severity: High]
This is a pre-existing issue, but does this loop hang indefinitely if the
device is configured with fewer than 9 irqs?

If platform_get_irq_optional() returns -ENXIO, we ignore the error but do
not increment irqs or break out of the loop.

Since i continues to increment without irqs reaching ARRAY_SIZE(irqbuf), it
appears the driver will endlessly query for non-existent irqs.


drivers/dma/nbpfaxi.c:nbpf_probe() {
[ ... ]
	ret = devm_request_irq(dev, eirq, nbpf_err_irq,
			       IRQF_SHARED, "dma error", nbpf);
[ ... ]
	ret = clk_prepare_enable(nbpf->clk);
[ ... ]
}

[Severity: High]
This isn't a bug introduced by this patch, but could this cause a hard fault
if another device on the same shared irq line triggers an interrupt before
our clock is enabled?

We call devm_request_irq() with IRQF_SHARED here, but clk_prepare_enable()
isn't called until later in nbpf_probe().

If a shared interrupt fires in this window, the interrupt handler might
attempt to read unclocked memory-mapped registers.


drivers/dma/nbpfaxi.c:nbpf_desc_page_alloc() {
	struct nbpf_desc_page *dpage = (void *)get_zeroed_page(GFP_KERNEL | GFP_DMA);
[ ... ]
unmap_error:
	while (i--) {
		ldesc--; hwdesc--;

		dma_unmap_single(dchan->device->dev, ldesc->hwdesc_dma_addr,
				 sizeof(hwdesc), DMA_TO_DEVICE);
	}

	return -ENOMEM;
}

[Severity: High]
This is a pre-existing issue, but does the unmap_error path leak the allocated
dpage?

Additionally, is the wrong size passed to dma_unmap_single()?

It uses sizeof(hwdesc), which is the size of the pointer, instead of
sizeof(*hwdesc).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-upstreaming-nbpfaxi-v1-v1-1-fd8ea8830cea@weidmueller.com?part=1