Re: [PATCH] spi: bcm2835: do not unregister controller in shutdown handler

[Lino Sanfilippo]

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