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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2c4d7115-7a02-f79e-c91b-3c2dd54051b2@gmx.de>
Date:   Sun, 3 Oct 2021 17:25:47 +0200
From:   Lino Sanfilippo <LinoSanfilippo@....de>
To:     Mark Brown <broonie@...nel.org>
Cc:     f.fainelli@...il.com, rjui@...adcom.com, sbranden@...adcom.com,
        bcm-kernel-feedback-list@...adcom.com, nsaenz@...nel.org,
        linux-spi@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        jgg@...pe.ca, p.rosenberger@...bus.com,
        linux-integrity@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] spi: bcm2835: do not unregister controller in shutdown
 handler


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