spi: stm32: drop devres version of spi_register_master

Commit Message

Alain Volmat March 12, 2021, 10:34 a.m. UTC

From: Antonio Borneo <[e-mail]>

A call to spi_unregister_master() triggers calling remove()
for all the spi devices binded to the spi master.

Some spi device driver requires to "talk" with the spi device
during the remove(), e.g.:
- a LCD panel like drivers/gpu/drm/panel/panel-lg-lg4573.c
  will turn off the backlighting sending a command over spi.
This implies that the spi master must be fully functional when
spi_unregister_master() is called, either if it is called
explicitly in the master's remove() code or implicitly by the
devres framework.

Devres calls devres_release_all() to release all the resources
"after" the remove() of the spi master driver (check code of
__device_release_driver() in drivers/base/dd.c).
If the spi master driver has an empty remove() then there would
be no issue; the devres_release_all() will release everything
in reverse order w.r.t. probe().
But if code in spi master driver remove() disables the spi or
makes it not functional (like in this spi-stm32), then devres
cannot be used safely for unregistering the spi master and the
binded spi devices.

Replace devm_spi_register_master() with spi_register_master()
and add spi_unregister_master() as first action in remove().

Fixes: dcbe0d84dfa5 ("spi: add driver for STM32 SPI controller")

Signed-off-by: Antonio Borneo <[e-mail]>
Signed-off-by: Alain Volmat <[e-mail]>
---
 drivers/spi/spi-stm32.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Mark Brown March 12, 2021, 8:25 p.m. UTC | #1

On Fri, 12 Mar 2021 11:34:46 +0100, Alain Volmat wrote:
> A call to spi_unregister_master() triggers calling remove()
> for all the spi devices binded to the spi master.
> 
> Some spi device driver requires to "talk" with the spi device
> during the remove(), e.g.:
> - a LCD panel like drivers/gpu/drm/panel/panel-lg-lg4573.c
>   will turn off the backlighting sending a command over spi.
> This implies that the spi master must be fully functional when
> spi_unregister_master() is called, either if it is called
> explicitly in the master's remove() code or implicitly by the
> devres framework.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: stm32: drop devres version of spi_register_master
      commit: 8d559a64f00b59af9cc02b803ff52f6e6880a651

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Alain Volmat March 18, 2021, 7:22 a.m. UTC | #2

Hi Lukas,

On Tue, Mar 16, 2021 at 10:17:44PM +0100, Lukas Wunner wrote:
> On Fri, Mar 12, 2021 at 11:34:46AM +0100, Alain Volmat wrote:
> > --- a/drivers/spi/spi-stm32.c
> > +++ b/drivers/spi/spi-stm32.c
> > @@ -1960,6 +1960,7 @@ static int stm32_spi_remove(struct platform_device *pdev)
> >  	struct spi_master *master = platform_get_drvdata(pdev);
> >  	struct stm32_spi *spi = spi_master_get_devdata(master);
> >  
> > +	spi_unregister_master(master);
> >  	spi->cfg->disable(spi);
> >  
> >  	if (master->dma_tx)
> 
> This introduces a use-after-free because spi_unregister_master()
> drops the last reference on the spi_master allocation (which includes
> the struct stm32_spi), causing it to be freed, yet the stm32_spi
> struct is accessed afterwards.

Indeed. Thanks. I've fixed that and will post it.

> You need to convert the driver to devm_spi_alloc_master() to
> fix the use-after-free.  See commit 6cfd39e212de for an example.
> 
> Thanks,
> 
> Lukas

Patch

diff --git a/drivers/spi/spi-stm32.c b/drivers/spi/spi-stm32.c
index 25c076461011..97cf3a2d4180 100644
--- a/drivers/spi/spi-stm32.c
+++ b/drivers/spi/spi-stm32.c
@@ -1929,7 +1929,7 @@  static int stm32_spi_probe(struct platform_device *pdev)
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
-	ret = devm_spi_register_master(&pdev->dev, master);
+	ret = spi_register_master(master);
 	if (ret) {
 		dev_err(&pdev->dev, "spi master registration failed: %d\n",
 			ret);
@@ -1960,6 +1960,7 @@  static int stm32_spi_remove(struct platform_device *pdev)
 	struct spi_master *master = platform_get_drvdata(pdev);
 	struct stm32_spi *spi = spi_master_get_devdata(master);
 
+	spi_unregister_master(master);
 	spi->cfg->disable(spi);
 
 	if (master->dma_tx)