Hi, On 01.10.21 at 19:54, Mark Brown wrote: > On Tue, Sep 28, 2021 at 09:56:57PM +0200, Lino Sanfilippo wrote: > >> One example is if the BCM2835 driver is used together with the TPM SPI >> driver: >> At system shutdown first the TPM chip devices (pre) shutdown handler >> (tpm_class_shutdown) is called, stopping the chip and setting an operations >> pointer to NULL. >> Then since the BCM2835 shutdown handler unregisters the SPI controller the >> TPM SPI remove function (tpm_tis_spi_remove) is also called. In case of >> TPM 2 this function accesses the now nullified operations pointer, >> resulting in the following NULL pointer access: > > This is a bug in that driver, it should be able to cope with a race > between a removal (which might be triggered for some other reason) and a > shutdown. Obviously this is actively triggered by this code path but it > could happen via some other mechanism. > >> The first attempt to fix this was with an extra check in the tpm chip >> driver (see https://marc.info/?l=linux-integrity&m=163129718414118&w=2) to >> avoid the NULL pointer access. >> Then Jason Gunthorpe noted that the real issue was the BCM driver >> unregistering the chip in the shutdown handler(see >> https://marc.info/?l=linux-integrity&m=163129718414118&w=2) which led >> me to this solution. > > Whatever happens here you should still fix the driver. Agreed. > >> -static int bcm2835_spi_remove(struct platform_device *pdev) >> +static void bcm2835_spi_shutdown(struct platform_device *pdev) >> { >> struct spi_controller *ctlr = platform_get_drvdata(pdev); >> struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr); >> >> - bcm2835_debugfs_remove(bs); >> - >> - spi_unregister_controller(ctlr); >> - >> bcm2835_dma_release(ctlr, bs); > > It is not at all clear to me that it is safe to deallocate the DMA > resources the controller is using without first releasing the > controller, I don't see what's stopping something coming along and > submitting new transactions which could in turn try to start doing > DMA. > I see your point here. So what about narrowing down the shutdown handler to only disable the hardware: static void bcm2835_spi_shutdown(struct platform_device *pdev) { struct spi_controller *ctlr = platform_get_drvdata(pdev); struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr); if (ctlr->dma_tx) dmaengine_terminate_sync(ctlr->dma_tx); if (ctlr->dma_rx) dmaengine_terminate_sync(ctlr->dma_rx); /* Clear FIFOs, and disable the HW block */ bcm2835_wr(bs, BCM2835_SPI_CS, BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX); clk_disable_unprepare(bs->clk); } Regards, Lino
