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: <20211001175422.GA53652@sirena.org.uk>
Date:   Fri, 1 Oct 2021 18:54:22 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Lino Sanfilippo <LinoSanfilippo@....de>
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

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.

> -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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)