Linux-SPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] spi: stm32: drop devres version of spi_register_master
@ 2021-03-12 10:34 Alain Volmat
  2021-03-12 20:25 ` Mark Brown
  2021-03-16 21:17 ` Lukas Wunner
  0 siblings, 2 replies; 4+ messages in thread
From: Alain Volmat @ 2021-03-12 10:34 UTC (permalink / raw)
  To: broonie, amelie.delaunay
  Cc: mcoquelin.stm32, alexandre.torgue, linux-spi, linux-stm32,
	linux-arm-kernel, linux-kernel, fabrice.gasnier, alain.volmat,
	antonio.borneo

From: Antonio Borneo <antonio.borneo@foss.st.com>

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 <antonio.borneo@foss.st.com>
Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
---
 drivers/spi/spi-stm32.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] spi: stm32: drop devres version of spi_register_master
  2021-03-12 10:34 [PATCH] spi: stm32: drop devres version of spi_register_master Alain Volmat
@ 2021-03-12 20:25 ` Mark Brown
  2021-03-16 21:17 ` Lukas Wunner
  1 sibling, 0 replies; 4+ messages in thread
From: Mark Brown @ 2021-03-12 20:25 UTC (permalink / raw)
  To: amelie.delaunay, Alain Volmat
  Cc: antonio.borneo, linux-spi, alexandre.torgue, linux-arm-kernel,
	fabrice.gasnier, linux-stm32, mcoquelin.stm32, linux-kernel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] spi: stm32: drop devres version of spi_register_master
  2021-03-12 10:34 [PATCH] spi: stm32: drop devres version of spi_register_master Alain Volmat
  2021-03-12 20:25 ` Mark Brown
@ 2021-03-16 21:17 ` Lukas Wunner
  2021-03-18  7:22   ` Alain Volmat
  1 sibling, 1 reply; 4+ messages in thread
From: Lukas Wunner @ 2021-03-16 21:17 UTC (permalink / raw)
  To: Alain Volmat
  Cc: broonie, amelie.delaunay, mcoquelin.stm32, alexandre.torgue,
	linux-spi, linux-stm32, linux-arm-kernel, linux-kernel,
	fabrice.gasnier, antonio.borneo

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.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] spi: stm32: drop devres version of spi_register_master
  2021-03-16 21:17 ` Lukas Wunner
@ 2021-03-18  7:22   ` Alain Volmat
  0 siblings, 0 replies; 4+ messages in thread
From: Alain Volmat @ 2021-03-18  7:22 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: broonie, amelie.delaunay, mcoquelin.stm32, alexandre.torgue,
	linux-spi, linux-stm32, linux-arm-kernel, linux-kernel,
	fabrice.gasnier, antonio.borneo

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-12 10:34 [PATCH] spi: stm32: drop devres version of spi_register_master Alain Volmat
2021-03-12 20:25 ` Mark Brown
2021-03-16 21:17 ` Lukas Wunner
2021-03-18  7:22   ` Alain Volmat

Linux-SPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-spi/0 linux-spi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-spi linux-spi/ https://lore.kernel.org/linux-spi \
		linux-spi@vger.kernel.org
	public-inbox-index linux-spi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-spi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git