spi: imx: fix use-after-free on unbind
Subsystems (6)
v2 cover spi: imx: fix use-after-free on unbind
The SPI subsystem frees the controller and any subsystem allocated driver data as part of deregistration (unless the allocation is device managed).
This series fixes the IMX driver that got this wrong and then converts it to use device managed allocation.
Included are also a (preparatory) deregistration fix for the rockchip driver and related cleanups for the tegre20-slink and rockchip drivers that both take a controller reference during unbind.
Johan
Changes in v2
- add stable tag to rockchip fix
- move rockchip fix before the cleanups
Johan Hovold (5):
spi: imx: fix use-after-free on unbind
spi: rockchip: fix controller deregistration
spi: imx: switch to managed controller allocation
spi: tegra20-slink: switch to managed controller allocation
spi: rockchip: switch to managed controller allocation
drivers/spi/spi-imx.c | 41 +++++++++++---------------------- drivers/spi/spi-rockchip.c | 40 +++++++++++++------------------- drivers/spi/spi-tegra20-slink.c | 26 ++++++++------------- 3 files changed, 40 insertions(+), 67 deletions(-)
Signature · 1 line
2.52.0
PATCH v2 1/5 spi: imx: fix use-after-free on unbind
The SPI subsystem frees the controller and any subsystem allocated driver data as part of deregistration (unless the allocation is device managed).
Take another reference before deregistering the controller so that the driver data is not freed until the driver is done with it.
--- drivers/spi/spi-imx.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/spi/spi-imx.c +4 -0
@@ -2401,6 +2401,8 @@ static void spi_imx_remove(struct platform_device *pdev)struct spi_imx_data *spi_imx =spi_controller_get_devdata (controller);int ret; +spi_controller_get (controller); +spi_unregister_controller (controller); ret =pm_runtime_get_sync (spi_imx->dev);
@@ -2414,6 +2416,8 @@ static void spi_imx_remove(struct platform_device *pdev)pm_runtime_disable (spi_imx->dev);spi_imx_sdma_exit (spi_imx); + +spi_controller_put (controller); }static int spi_imx_runtime_resume (struct device *dev)
Signature · 1 line
2.52.0
PATCH v2 2/5 spi: rockchip: fix controller deregistration
In reply to Johan HovoldMake sure to deregister the controller before freeing underlying resources like DMA channels during driver unbind.
--- drivers/spi/spi-rockchip.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/spi/spi-rockchip.c +3 -1
@@ -908,7 +908,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)break ; } - ret =devm_spi_register_controller (&pdev->dev, ctlr); + ret =spi_register_controller (ctlr);if (ret <0 ) {dev_err (&pdev->dev,"Failed to register controller\n" ); goto err_free_dma_rx;
@@ -936,6 +936,8 @@ static void rockchip_spi_remove(struct platform_device *pdev)pm_runtime_get_sync (&pdev->dev); +spi_unregister_controller (ctlr); +pm_runtime_put_noidle (&pdev->dev);pm_runtime_disable (&pdev->dev);pm_runtime_set_suspended (&pdev->dev);
Signature · 1 line
2.52.0
PATCH v2 3/5 spi: imx: switch to managed controller allocation
In reply to Johan HovoldSwitch to device managed controller allocation to simplify error handling and to avoid having to take another reference during deregistration.
--- drivers/spi/spi-imx.c | 45 ++++++++++++++----------------------------- 1 file changed, 14 insertions(+), 31 deletions(-)
drivers/spi/spi-imx.c +14 -31
@@ -2231,11 +2231,9 @@ static int spi_imx_probe(struct platform_device *pdev) target_mode = devtype_data->has_targetmode &&of_property_read_bool (np,"spi-slave" );if (target_mode) - controller =spi_alloc_target (&pdev->dev, -sizeof (struct spi_imx_data )); + controller =devm_spi_alloc_target (&pdev->dev,sizeof (*spi_imx));else - controller =spi_alloc_host (&pdev->dev, -sizeof (struct spi_imx_data )); + controller =devm_spi_alloc_host (&pdev->dev,sizeof (*spi_imx));if (!controller)return -ENOMEM ;
@@ -2304,40 +2302,31 @@ static int spi_imx_probe(struct platform_device *pdev)init_completion (&spi_imx->xfer_done); spi_imx->base =devm_platform_get_and_ioremap_resource (pdev,0 , &res); -if (IS_ERR (spi_imx->base)) { - ret =PTR_ERR (spi_imx->base); - goto out_controller_put; - } +if (IS_ERR (spi_imx->base)) +return PTR_ERR (spi_imx->base); + spi_imx->base_phys = res->start; irq =platform_get_irq (pdev,0 ); -if (irq <0 ) { - ret = irq; - goto out_controller_put; - } +if (irq <0 ) +return irq; ret =devm_request_irq (&pdev->dev, irq, spi_imx_isr,0 ,dev_name (&pdev->dev), spi_imx); -if (ret) { -dev_err (&pdev->dev,"can't get irq%d: %d\n" , irq, ret); - goto out_controller_put; - } +if (ret) +return dev_err_probe (&pdev->dev, ret,"can't get irq%d\n" , irq); spi_imx->clk_ipg =devm_clk_get (&pdev->dev,"ipg" ); -if (IS_ERR (spi_imx->clk_ipg)) { - ret =PTR_ERR (spi_imx->clk_ipg); - goto out_controller_put; - } +if (IS_ERR (spi_imx->clk_ipg)) +return PTR_ERR (spi_imx->clk_ipg); spi_imx->clk_per =devm_clk_get (&pdev->dev,"per" ); -if (IS_ERR (spi_imx->clk_per)) { - ret =PTR_ERR (spi_imx->clk_per); - goto out_controller_put; - } +if (IS_ERR (spi_imx->clk_per)) +return PTR_ERR (spi_imx->clk_per); ret =clk_prepare_enable (spi_imx->clk_per);if (ret) - goto out_controller_put; +return ret; ret =clk_prepare_enable (spi_imx->clk_ipg);if (ret)
@@ -2389,8 +2378,6 @@ static int spi_imx_probe(struct platform_device *pdev)clk_disable_unprepare (spi_imx->clk_ipg); out_put_per:clk_disable_unprepare (spi_imx->clk_per); -out_controller_put: -spi_controller_put (controller);return ret; }
@@ -2401,8 +2388,6 @@ static void spi_imx_remove(struct platform_device *pdev)struct spi_imx_data *spi_imx =spi_controller_get_devdata (controller);int ret; -spi_controller_get (controller); -spi_unregister_controller (controller); ret =pm_runtime_get_sync (spi_imx->dev);
@@ -2416,8 +2401,6 @@ static void spi_imx_remove(struct platform_device *pdev)pm_runtime_disable (spi_imx->dev);spi_imx_sdma_exit (spi_imx); - -spi_controller_put (controller); }static int spi_imx_runtime_resume (struct device *dev)
Signature · 1 line
2.52.0
PATCH v2 4/5 spi: tegra20-slink: switch to managed controller allocation
In reply to Johan HovoldSwitch to device managed controller allocation to simplify error handling and to avoid having to take another reference during deregistration.
--- drivers/spi/spi-tegra20-slink.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)
drivers/spi/spi-tegra20-slink.c +10 -16
@@ -1007,7 +1007,7 @@ static int tegra_slink_probe(struct platform_device *pdev) cdata =of_device_get_match_data (&pdev->dev); - host =spi_alloc_host (&pdev->dev,sizeof (*tspi)); + host =devm_spi_alloc_host (&pdev->dev,sizeof (*tspi));if (!host) {dev_err (&pdev->dev,"host allocation failed\n" );return -ENOMEM ;
@@ -1034,37 +1034,34 @@ static int tegra_slink_probe(struct platform_device *pdev) host->max_speed_hz =25000000 ;/* 25MHz */ tspi->base =devm_platform_get_and_ioremap_resource (pdev,0 , &r); -if (IS_ERR (tspi->base)) { - ret =PTR_ERR (tspi->base); - goto exit_free_host; - } +if (IS_ERR (tspi->base)) +return PTR_ERR (tspi->base); + tspi->phys = r->start;/* disabled clock may cause interrupt storm upon request */ tspi->clk =devm_clk_get (&pdev->dev,NULL );if (IS_ERR (tspi->clk)) { ret =PTR_ERR (tspi->clk); -dev_err (&pdev->dev,"Can not get clock %d\n" , ret); - goto exit_free_host; +return dev_err_probe (&pdev->dev, ret,"Can not get clock\n" ); } tspi->rst =devm_reset_control_get_exclusive (&pdev->dev,"spi" );if (IS_ERR (tspi->rst)) { -dev_err (&pdev->dev,"can not get reset\n" ); ret =PTR_ERR (tspi->rst); - goto exit_free_host; +return dev_err_probe (&pdev->dev, ret,"can not get reset\n" ); } ret =devm_tegra_core_dev_init_opp_table_common (&pdev->dev);if (ret) - goto exit_free_host; +return ret; tspi->max_buf_size =SLINK_FIFO_DEPTH <<2 ; tspi->dma_buf_size =DEFAULT_SPI_DMA_BUF_LEN ; ret =tegra_slink_init_dma_param (tspi, true);if (ret <0 ) - goto exit_free_host; +return ret; ret =tegra_slink_init_dma_param (tspi, false);if (ret <0 ) goto exit_rx_dma_free;
@@ -1125,14 +1122,13 @@ static int tegra_slink_probe(struct platform_device *pdev)tegra_slink_deinit_dma_param (tspi, false); exit_rx_dma_free:tegra_slink_deinit_dma_param (tspi, true); -exit_free_host: -spi_controller_put (host); +return ret; }static void tegra_slink_remove (struct platform_device *pdev) { -struct spi_controller *host =spi_controller_get (platform_get_drvdata (pdev)); +struct spi_controller *host =platform_get_drvdata (pdev);struct tegra_slink_data *tspi =spi_controller_get_devdata (host);spi_unregister_controller (host);
@@ -1146,8 +1142,6 @@ static void tegra_slink_remove(struct platform_device *pdev)if (tspi->rx_dma_chan)tegra_slink_deinit_dma_param (tspi, true); - -spi_controller_put (host); }#ifdef CONFIG_PM_SLEEP
Signature · 1 line
2.52.0
PATCH v2 5/5 spi: rockchip: switch to managed controller allocation
In reply to Johan HovoldSwitch to device managed controller allocation to simplify error handling and to avoid having to take another reference during deregistration.
--- drivers/spi/spi-rockchip.c | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-)
drivers/spi/spi-rockchip.c +13 -23
@@ -767,9 +767,9 @@ static int rockchip_spi_probe(struct platform_device *pdev) target_mode =of_property_read_bool (np,"spi-slave" );if (target_mode) - ctlr =spi_alloc_target (&pdev->dev,sizeof (struct rockchip_spi )); + ctlr =devm_spi_alloc_target (&pdev->dev,sizeof (*rs));else - ctlr =spi_alloc_host (&pdev->dev,sizeof (struct rockchip_spi )); + ctlr =devm_spi_alloc_host (&pdev->dev,sizeof (*rs));if (!ctlr)return -ENOMEM ;
@@ -780,35 +780,31 @@ static int rockchip_spi_probe(struct platform_device *pdev)/* Get basic io resource and map it */ rs->regs =devm_platform_get_and_ioremap_resource (pdev,0 , &mem); -if (IS_ERR (rs->regs)) { - ret =PTR_ERR (rs->regs); - goto err_put_ctlr; - } +if (IS_ERR (rs->regs)) +return PTR_ERR (rs->regs); rs->apb_pclk =devm_clk_get_enabled (&pdev->dev,"apb_pclk" );if (IS_ERR (rs->apb_pclk)) { - ret =dev_err_probe (&pdev->dev,PTR_ERR (rs->apb_pclk), -"Failed to get apb_pclk\n" ); - goto err_put_ctlr; +return dev_err_probe (&pdev->dev,PTR_ERR (rs->apb_pclk), +"Failed to get apb_pclk\n" ); } rs->spiclk =devm_clk_get_enabled (&pdev->dev,"spiclk" );if (IS_ERR (rs->spiclk)) { - ret =dev_err_probe (&pdev->dev,PTR_ERR (rs->spiclk), -"Failed to get spi_pclk\n" ); - goto err_put_ctlr; +return dev_err_probe (&pdev->dev,PTR_ERR (rs->spiclk), +"Failed to get spi_pclk\n" ); }spi_enable_chip (rs, false); ret =platform_get_irq (pdev,0 );if (ret <0 ) - goto err_put_ctlr; +return ret; ret =devm_request_irq (&pdev->dev, ret, rockchip_spi_isr,0 ,dev_name (&pdev->dev), ctlr);if (ret) - goto err_put_ctlr; +return ret; rs->dev = &pdev->dev; rs->freq =clk_get_rate (rs->spiclk);
@@ -830,10 +826,8 @@ static int rockchip_spi_probe(struct platform_device *pdev) } rs->fifo_len =get_fifo_len (rs); -if (!rs->fifo_len) { - ret =dev_err_probe (&pdev->dev, -EINVAL ,"Failed to get fifo length\n" ); - goto err_put_ctlr; - } +if (!rs->fifo_len) +return dev_err_probe (&pdev->dev, -EINVAL ,"Failed to get fifo length\n" );pm_runtime_set_autosuspend_delay (&pdev->dev,ROCKCHIP_AUTOSUSPEND_TIMEOUT );pm_runtime_use_autosuspend (&pdev->dev);
@@ -924,15 +918,13 @@ static int rockchip_spi_probe(struct platform_device *pdev)dma_release_channel (ctlr->dma_tx); err_disable_pm_runtime:pm_runtime_disable (&pdev->dev); -err_put_ctlr: -spi_controller_put (ctlr);return ret; }static void rockchip_spi_remove (struct platform_device *pdev) { -struct spi_controller *ctlr =spi_controller_get (platform_get_drvdata (pdev)); +struct spi_controller *ctlr =platform_get_drvdata (pdev);pm_runtime_get_sync (&pdev->dev);
@@ -946,8 +938,6 @@ static void rockchip_spi_remove(struct platform_device *pdev)dma_release_channel (ctlr->dma_tx);if (ctlr->dma_rx)dma_release_channel (ctlr->dma_rx); - -spi_controller_put (ctlr); }#ifdef CONFIG_PM_SLEEP
Signature · 1 line
2.52.0
Mark Brown
maintainerQuoted 10 lines · Johan Hovold
spi: imx: fix use-after-free on unbind
The SPI subsystem frees the controller and any subsystem allocated driver data as part of deregistration (unless the allocation is device managed).
This series fixes the IMX driver that got this wrong and then converts it to use device managed allocation.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-7.1
Thanks!
[1/5] spi: imx: fix use-after-free on unbind
https://git.kernel.org/broonie/spi/c/1c78c2002380 [2/5] spi: rockchip: fix controller deregistration https://git.kernel.org/broonie/spi/c/53e7a16070fe [3/5] spi: imx: switch to managed controller allocation https://git.kernel.org/broonie/spi/c/9c0180650654 [4/5] spi: tegra20-slink: switch to managed controller allocation https://git.kernel.org/broonie/spi/c/4ba75403fb4d [5/5] spi: rockchip: switch to managed controller allocation https://git.kernel.org/broonie/spi/c/7f3eb70516c7
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