Re: [PATCH 2/3] [v5 net-next] p54spi: convert to devicetree

[Simon Horman <horms@kernel.org>]

On Mon, May 11, 2026 at 09:45:18PM +0200, Arnd Bergmann wrote:
> On Mon, May 11, 2026, at 18:12, Simon Horman wrote:
> >
> > Simon says: As a pre-existing issue I don't think we need to be concerned
> >             about this in the scope of this patch-set. But it may
> > 	    be a potential for follow-up.
> >
> > This problem wasn't introduced by this patch, but I noticed a pre-existing
> > issue in the context here. Does the p54spi_spi_read() function perform an SPI
> > transfer using a stack-allocated buffer?
> >
> > Looking at p54spi_spi_read(), the addr variable is allocated on the stack and
> > passed as a transfer buffer to spi_sync(). The SPI framework maps transfer
> > buffers for DMA. Passing a stack-allocated buffer is illegal, particularly with
> > CONFIG_VMAP_STACK enabled, because stack memory resides in vmalloc space and
> > cannot be safely DMA-mapped.
> >
> > Could this cause memory corruption or a kernel panic during the transfer?
> 
> I already commented on this earlier: This is indeed a theoretical
> bug, but it works in practice because DMA-capable SPI controllers
> tend to all use MMIO for short transfers instead of DMA.
> 
> SPI is very fragile this way, and I do have some patches to improve
> that overall, but it seems out of scope for this one driver.

Thanks, I agree this is out of scope.

> >> -
> >> -	gpio_direction_output(p54spi_gpio_power, 0);
> >> -	gpio_direction_input(p54spi_gpio_irq);
> >> -
> >> -	ret = request_irq(gpio_to_irq(p54spi_gpio_irq),
> >> -			  p54spi_interrupt, IRQF_NO_AUTOEN, "p54spi",
> >> -			  priv->spi);
> >> +	ret = request_irq(spi->irq, p54spi_interrupt, IRQF_NO_AUTOEN, "p54spi", priv->spi);
> >
> > Simon says: This one seems more concerning in the context of this patch-set.
> >
> > Is there missing validation for spi->irq before calling request_irq()?
> >
> > If the device tree node lacks an interrupts property, the SPI core may set
> > spi->irq to 0 or a negative error code.
> >
> > Passing this directly to request_irq() without validation can successfully
> > hijack a core system interrupt like the system timer on some architectures,
> > leading to system instability or deadlocks.
> >
> > Should there be a check like if (spi->irq <= 0) to fail the probe gracefully?
> 
> I also commented on this: request_irq() already fails gracefully
> with -EINVAL when presented with an invalid IRQ. IRQ 0 is guaranteed
> to be invalid on any target that uses devicetree.

Thanks, and sorry for not seeing your earlier comment.
Or realising it is a false positive.