spi_geni_remove() accesses the driver's private data after calling spi_unregister_master() even though that function releases the last reference on the spi_master and thereby frees the private data. Moreover, since commit 1a9e489e6128 ("spi: spi-geni-qcom: Use OPP API to set clk/perf state"), spi_geni_probe() leaks the spi_master allocation if the calls to dev_pm_opp_set_clkname() or dev_pm_opp_of_add_table() fail. Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound and also avoids the spi_master leak on probe. Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v4.20+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v4.20+ Cc: Rajendra Nayak <rnayak@codeaurora.org> Cc: Girish Mahadevan <girishm@codeaurora.org> --- drivers/spi/spi-geni-qcom.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index 25810a7eef10..0e3d8e6c08f4 100644 --- a/drivers/spi/spi-geni-qcom.c +++ b/drivers/spi/spi-geni-qcom.c @@ -603,7 +603,7 @@ static int spi_geni_probe(struct platform_device *pdev) if (IS_ERR(clk)) return PTR_ERR(clk); - spi = spi_alloc_master(dev, sizeof(*mas)); + spi = devm_spi_alloc_master(dev, sizeof(*mas)); if (!spi) return -ENOMEM; @@ -673,7 +673,6 @@ static int spi_geni_probe(struct platform_device *pdev) free_irq(mas->irq, spi); spi_geni_probe_runtime_disable: pm_runtime_disable(dev); - spi_master_put(spi); dev_pm_opp_of_remove_table(&pdev->dev); put_clkname: dev_pm_opp_put_clkname(mas->se.opp_table); -- 2.28.0
qcom_qspi_remove() accesses the driver's private data after calling spi_unregister_master() even though that function releases the last reference on the spi_master and thereby frees the private data. Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound. Fixes: f79a158d37c2 ("spi: spi-qcom-qspi: Use OPP API to set clk/perf state") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v5.9+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v5.9+ Cc: Rajendra Nayak <rnayak@codeaurora.org> --- drivers/spi/spi-qcom-qspi.c | 42 ++++++++++++++----------------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c index 5eed88af6899..8863be370884 100644 --- a/drivers/spi/spi-qcom-qspi.c +++ b/drivers/spi/spi-qcom-qspi.c @@ -462,7 +462,7 @@ static int qcom_qspi_probe(struct platform_device *pdev) dev = &pdev->dev; - master = spi_alloc_master(dev, sizeof(*ctrl)); + master = devm_spi_alloc_master(dev, sizeof(*ctrl)); if (!master) return -ENOMEM; @@ -473,54 +473,49 @@ static int qcom_qspi_probe(struct platform_device *pdev) spin_lock_init(&ctrl->lock); ctrl->dev = dev; ctrl->base = devm_platform_ioremap_resource(pdev, 0); - if (IS_ERR(ctrl->base)) { - ret = PTR_ERR(ctrl->base); - goto exit_probe_master_put; - } + if (IS_ERR(ctrl->base)) + return PTR_ERR(ctrl->base); ctrl->clks = devm_kcalloc(dev, QSPI_NUM_CLKS, sizeof(*ctrl->clks), GFP_KERNEL); - if (!ctrl->clks) { - ret = -ENOMEM; - goto exit_probe_master_put; - } + if (!ctrl->clks) + return -ENOMEM; ctrl->clks[QSPI_CLK_CORE].id = "core"; ctrl->clks[QSPI_CLK_IFACE].id = "iface"; ret = devm_clk_bulk_get(dev, QSPI_NUM_CLKS, ctrl->clks); if (ret) - goto exit_probe_master_put; + return ret; ctrl->icc_path_cpu_to_qspi = devm_of_icc_get(dev, "qspi-config"); - if (IS_ERR(ctrl->icc_path_cpu_to_qspi)) { - ret = dev_err_probe(dev, PTR_ERR(ctrl->icc_path_cpu_to_qspi), - "Failed to get cpu path\n"); - goto exit_probe_master_put; - } + if (IS_ERR(ctrl->icc_path_cpu_to_qspi)) + return dev_err_probe(dev, PTR_ERR(ctrl->icc_path_cpu_to_qspi), + "Failed to get cpu path\n"); + /* Set BW vote for register access */ ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, Bps_to_icc(1000), Bps_to_icc(1000)); if (ret) { dev_err(ctrl->dev, "%s: ICC BW voting failed for cpu: %d\n", __func__, ret); - goto exit_probe_master_put; + return ret; } ret = icc_disable(ctrl->icc_path_cpu_to_qspi); if (ret) { dev_err(ctrl->dev, "%s: ICC disable failed for cpu: %d\n", __func__, ret); - goto exit_probe_master_put; + return ret; } ret = platform_get_irq(pdev, 0); if (ret < 0) - goto exit_probe_master_put; + return ret; ret = devm_request_irq(dev, ret, qcom_qspi_irq, IRQF_TRIGGER_HIGH, dev_name(dev), ctrl); if (ret) { dev_err(dev, "Failed to request irq %d\n", ret); - goto exit_probe_master_put; + return ret; } master->max_speed_hz = 300000000; @@ -537,10 +532,8 @@ static int qcom_qspi_probe(struct platform_device *pdev) master->auto_runtime_pm = true; ctrl->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core"); - if (IS_ERR(ctrl->opp_table)) { - ret = PTR_ERR(ctrl->opp_table); - goto exit_probe_master_put; - } + if (IS_ERR(ctrl->opp_table)) + return PTR_ERR(ctrl->opp_table); /* OPP table is optional */ ret = dev_pm_opp_of_add_table(&pdev->dev); if (ret && ret != -ENODEV) { @@ -562,9 +555,6 @@ static int qcom_qspi_probe(struct platform_device *pdev) exit_probe_put_clkname: dev_pm_opp_put_clkname(ctrl->opp_table); -exit_probe_master_put: - spi_master_put(master); - return ret; } -- 2.28.0
spi_sh_remove() accesses the driver's private data after calling spi_unregister_master() even though that function releases the last reference on the spi_master and thereby frees the private data. Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound. Fixes: 680c1305e259 ("spi/spi_sh: use spi_unregister_master instead of spi_master_put in remove path") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v3.0+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v3.0+ Cc: Axel Lin <axel.lin@ingics.com> --- drivers/spi/spi-sh.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/spi/spi-sh.c b/drivers/spi/spi-sh.c index 20bdae5fdf3b..15123a8f41e1 100644 --- a/drivers/spi/spi-sh.c +++ b/drivers/spi/spi-sh.c @@ -440,7 +440,7 @@ static int spi_sh_probe(struct platform_device *pdev) if (irq < 0) return irq; - master = spi_alloc_master(&pdev->dev, sizeof(struct spi_sh_data)); + master = devm_spi_alloc_master(&pdev->dev, sizeof(struct spi_sh_data)); if (master == NULL) { dev_err(&pdev->dev, "spi_alloc_master error.\n"); return -ENOMEM; @@ -458,16 +458,14 @@ static int spi_sh_probe(struct platform_device *pdev) break; default: dev_err(&pdev->dev, "No support width\n"); - ret = -ENODEV; - goto error1; + return -ENODEV; } ss->irq = irq; ss->master = master; ss->addr = devm_ioremap(&pdev->dev, res->start, resource_size(res)); if (ss->addr == NULL) { dev_err(&pdev->dev, "ioremap error.\n"); - ret = -ENOMEM; - goto error1; + return -ENOMEM; } INIT_LIST_HEAD(&ss->queue); spin_lock_init(&ss->lock); @@ -477,7 +475,7 @@ static int spi_sh_probe(struct platform_device *pdev) ret = request_irq(irq, spi_sh_irq, 0, "spi_sh", ss); if (ret < 0) { dev_err(&pdev->dev, "request_irq error\n"); - goto error1; + return ret; } master->num_chipselect = 2; @@ -496,9 +494,6 @@ static int spi_sh_probe(struct platform_device *pdev) error3: free_irq(irq, ss); - error1: - spi_master_put(master); - return ret; } -- 2.28.0
pxa2xx_spi_remove() accesses the driver's private data after calling spi_unregister_controller() even though that function releases the last reference on the spi_controller and thereby frees the private data. Fix by switching over to the new devm_spi_alloc_master/slave() helper which keeps the private data accessible until the driver has unbound. Fixes: 32e5b57232c0 ("spi: pxa2xx: Fix controller unregister order") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v2.6.17+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v2.6.17+: 32e5b57232c0: spi: pxa2xx: Fix controller unregister order Cc: <stable@vger.kernel.org> # v2.6.17+ --- drivers/spi/spi-pxa2xx.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index 814268405ab0..d6b534d38e5d 100644 --- a/drivers/spi/spi-pxa2xx.c +++ b/drivers/spi/spi-pxa2xx.c @@ -1686,9 +1686,9 @@ static int pxa2xx_spi_probe(struct platform_device *pdev) } if (platform_info->is_slave) - controller = spi_alloc_slave(dev, sizeof(struct driver_data)); + controller = devm_spi_alloc_slave(dev, sizeof(*drv_data)); else - controller = spi_alloc_master(dev, sizeof(struct driver_data)); + controller = devm_spi_alloc_master(dev, sizeof(*drv_data)); if (!controller) { dev_err(&pdev->dev, "cannot alloc spi_controller\n"); @@ -1911,7 +1911,6 @@ static int pxa2xx_spi_probe(struct platform_device *pdev) free_irq(ssp->irq, drv_data); out_error_controller_alloc: - spi_controller_put(controller); pxa_ssp_free(ssp); return status; } -- 2.28.0
rpcif_spi_remove() accesses the driver's private data after calling spi_unregister_controller() even though that function releases the last reference on the spi_controller and thereby frees the private data. Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound. Fixes: eb8d6d464a27 ("spi: add Renesas RPC-IF driver") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v5.9+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v5.9+ Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- drivers/spi/spi-rpc-if.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/spi/spi-rpc-if.c b/drivers/spi/spi-rpc-if.c index ed3e548227f4..3579675485a5 100644 --- a/drivers/spi/spi-rpc-if.c +++ b/drivers/spi/spi-rpc-if.c @@ -134,7 +134,7 @@ static int rpcif_spi_probe(struct platform_device *pdev) struct rpcif *rpc; int error; - ctlr = spi_alloc_master(&pdev->dev, sizeof(*rpc)); + ctlr = devm_spi_alloc_master(&pdev->dev, sizeof(*rpc)); if (!ctlr) return -ENOMEM; @@ -159,13 +159,8 @@ static int rpcif_spi_probe(struct platform_device *pdev) error = spi_register_controller(ctlr); if (error) { dev_err(&pdev->dev, "spi_register_controller failed\n"); - goto err_put_ctlr; + rpcif_disable_rpm(rpc); } - return 0; - -err_put_ctlr: - rpcif_disable_rpm(rpc); - spi_controller_put(ctlr); return error; } -- 2.28.0
If the calls to devm_clk_get() or devm_ioremap_resource() fail on probe of the Macronix SPI driver, the spi_master struct is erroneously not freed. Fix by switching over to the new devm_spi_alloc_master() helper. Fixes: b942d80b0a39 ("spi: Add MXIC controller driver") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v5.0+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v5.0+ Cc: Mason Yang <masonccyang@mxic.com.tw> --- drivers/spi/spi-mxic.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c index 8c630acb0110..96b418293bf2 100644 --- a/drivers/spi/spi-mxic.c +++ b/drivers/spi/spi-mxic.c @@ -529,7 +529,7 @@ static int mxic_spi_probe(struct platform_device *pdev) struct mxic_spi *mxic; int ret; - master = spi_alloc_master(&pdev->dev, sizeof(struct mxic_spi)); + master = devm_spi_alloc_master(&pdev->dev, sizeof(struct mxic_spi)); if (!master) return -ENOMEM; @@ -574,15 +574,9 @@ static int mxic_spi_probe(struct platform_device *pdev) ret = spi_register_master(master); if (ret) { dev_err(&pdev->dev, "spi_register_master failed\n"); - goto err_put_master; + pm_runtime_disable(&pdev->dev); } - return 0; - -err_put_master: - spi_master_put(master); - pm_runtime_disable(&pdev->dev); - return ret; } -- 2.28.0
If the calls to device_reset() or devm_spi_register_controller() fail on probe of the MediaTek MT7621 SPI driver, the spi_controller struct is erroneously not freed. Fix by switching over to the new devm_spi_alloc_master() helper. Moreover, the SYS clock is enabled on probe but not disabled if any of the subsequent probe steps fails. Finally, there's an ordering issue in mt7621_spi_remove() wherein the spi_controller is unregistered after disabling the SYS clock. The correct order is to call spi_unregister_controller() *before* this teardown step because bus accesses may still be ongoing until that function returns. All of these bugs have existed since the driver was first introduced, so it seems fair to fix them together in a single commit. Fixes: 1ab7f2a43558 ("staging: mt7621-spi: add mt7621 support") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v4.17+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v4.17+ --- drivers/spi/spi-mt7621.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c index 2c3b7a2a1ec7..d7cda66c4b26 100644 --- a/drivers/spi/spi-mt7621.c +++ b/drivers/spi/spi-mt7621.c @@ -350,10 +350,11 @@ static int mt7621_spi_probe(struct platform_device *pdev) if (status) return status; - master = spi_alloc_master(&pdev->dev, sizeof(*rs)); + master = devm_spi_alloc_master(&pdev->dev, sizeof(*rs)); if (!master) { dev_info(&pdev->dev, "master allocation failed\n"); - return -ENOMEM; + ret = -ENOMEM; + goto err_clk_disable; } master->mode_bits = SPI_LSB_FIRST; @@ -377,10 +378,18 @@ static int mt7621_spi_probe(struct platform_device *pdev) ret = device_reset(&pdev->dev); if (ret) { dev_err(&pdev->dev, "SPI reset failed!\n"); - return ret; + goto err_clk_disable; } - return devm_spi_register_controller(&pdev->dev, master); + ret = spi_register_controller(master); + if (ret) + goto err_clk_disable; + + return 0; + +err_clk_disable: + clk_disable_unprepare(clk); + return ret; } static int mt7621_spi_remove(struct platform_device *pdev) @@ -391,6 +400,7 @@ static int mt7621_spi_remove(struct platform_device *pdev) master = dev_get_drvdata(&pdev->dev); rs = spi_controller_get_devdata(master); + spi_unregister_controller(master); clk_disable_unprepare(rs->clk); return 0; -- 2.28.0
If the call to devm_spi_register_controller() fails on probe of the MediaTek SPI NOR driver, the spi_controller struct is erroneously not freed. Since commit a1daaa991ed1 ("spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer"), the same happens if the call to dmam_alloc_coherent() fails. Since commit 3bfd9103c7af ("spi: spi-mtk-nor: Add power management support"), the same happens if the call to mtk_nor_enable_clk() fails. Fix by switching over to the new devm_spi_alloc_master() helper. Fixes: 881d1ee9fe81 ("spi: add support for mediatek spi-nor controller") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v5.7+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v5.7+ Cc: Chuanhong Guo <gch981213@gmail.com> Cc: Ikjoon Jang <ikjn@chromium.org> --- drivers/spi/spi-mtk-nor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c index b97f26a60cbe..288f6c2bbd57 100644 --- a/drivers/spi/spi-mtk-nor.c +++ b/drivers/spi/spi-mtk-nor.c @@ -768,7 +768,7 @@ static int mtk_nor_probe(struct platform_device *pdev) return -EINVAL; } - ctlr = spi_alloc_master(&pdev->dev, sizeof(*sp)); + ctlr = devm_spi_alloc_master(&pdev->dev, sizeof(*sp)); if (!ctlr) { dev_err(&pdev->dev, "failed to allocate spi controller\n"); return -ENOMEM; -- 2.28.0
If the call to devm_spi_register_master() fails on probe of the GPIO SPI driver, the spi_master struct is erroneously not freed: After allocating the spi_master, its reference count is 1. The driver unconditionally decrements the reference count on unbind using a devm action. Before calling devm_spi_register_master(), the driver unconditionally increments the reference count because on success, that function will decrement the reference count on unbind. However on failure, devm_spi_register_master() does *not* decrement the reference count, so the spi_master is leaked. The issue was introduced by commits 8b797490b4db ("spi: gpio: Make sure spi_master_put() is called in every error path") and 79567c1a321e ("spi: gpio: Use devm_spi_register_master()"), which sought to plug leaks introduced by 9b00bc7b901f ("spi: spi-gpio: Rewrite to use GPIO descriptors") but missed this remaining leak. The situation was later aggravated by commit d3b0ffa1d75d ("spi: gpio: prevent memory leak in spi_gpio_probe"), which introduced a use-after-free because it releases a reference on the spi_master if devm_add_action_or_reset() fails even though the function already does that. Fix by switching over to the new devm_spi_alloc_master() helper. Fixes: 9b00bc7b901f ("spi: spi-gpio: Rewrite to use GPIO descriptors") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v4.17+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v5.1-: 8b797490b4db: spi: gpio: Make sure spi_master_put() is called in every error path Cc: <stable@vger.kernel.org> # v5.1-: 45beec351998: spi: bitbang: Introduce spi_bitbang_init() Cc: <stable@vger.kernel.org> # v5.1-: 79567c1a321e: spi: gpio: Use devm_spi_register_master() Cc: <stable@vger.kernel.org> # v5.4-: d3b0ffa1d75d: spi: gpio: prevent memory leak in spi_gpio_probe Cc: <stable@vger.kernel.org> # v4.17+ Cc: Navid Emamdoost <navid.emamdoost@gmail.com> Cc: Andrey Smirnov <andrew.smirnov@gmail.com> Cc: Linus Walleij <linus.walleij@linaro.org> --- drivers/spi/spi-gpio.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c index 7ceb0ba27b75..0584f4d2fde2 100644 --- a/drivers/spi/spi-gpio.c +++ b/drivers/spi/spi-gpio.c @@ -350,11 +350,6 @@ static int spi_gpio_probe_pdata(struct platform_device *pdev, return 0; } -static void spi_gpio_put(void *data) -{ - spi_master_put(data); -} - static int spi_gpio_probe(struct platform_device *pdev) { int status; @@ -363,16 +358,10 @@ static int spi_gpio_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct spi_bitbang *bb; - master = spi_alloc_master(dev, sizeof(*spi_gpio)); + master = devm_spi_alloc_master(dev, sizeof(*spi_gpio)); if (!master) return -ENOMEM; - status = devm_add_action_or_reset(&pdev->dev, spi_gpio_put, master); - if (status) { - spi_master_put(master); - return status; - } - if (pdev->dev.of_node) status = spi_gpio_probe_dt(pdev, master); else @@ -432,7 +421,7 @@ static int spi_gpio_probe(struct platform_device *pdev) if (status) return status; - return devm_spi_register_master(&pdev->dev, spi_master_get(master)); + return devm_spi_register_master(&pdev->dev, master); } MODULE_ALIAS("platform:" DRIVER_NAME); -- 2.28.0
If the calls to of_match_device(), of_alias_get_id(), devm_ioremap_resource(), devm_regmap_init_mmio() or devm_clk_get() fail on probe of the NPCM FIU SPI driver, the spi_controller struct is erroneously not freed. Fix by switching over to the new devm_spi_alloc_master() helper. Fixes: ace55c411b11 ("spi: npcm-fiu: add NPCM FIU controller driver") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v5.4+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v5.4+ Cc: Tomer Maimon <tmaimon77@gmail.com> --- drivers/spi/spi-npcm-fiu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/spi/spi-npcm-fiu.c b/drivers/spi/spi-npcm-fiu.c index 341f7cffeaac..1cb9329de945 100644 --- a/drivers/spi/spi-npcm-fiu.c +++ b/drivers/spi/spi-npcm-fiu.c @@ -679,7 +679,7 @@ static int npcm_fiu_probe(struct platform_device *pdev) struct resource *res; int id; - ctrl = spi_alloc_master(dev, sizeof(*fiu)); + ctrl = devm_spi_alloc_master(dev, sizeof(*fiu)); if (!ctrl) return -ENOMEM; -- 2.28.0
If the calls to devm_clk_get(), devm_spi_register_master() or clk_prepare_enable() fail on probe of the Mikrotik RB4xx SPI driver, the spi_master struct is erroneously not freed. Fix by switching over to the new devm_spi_alloc_master() helper. Fixes: 05aec357871f ("spi: Add SPI driver for Mikrotik RB4xx series boards") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v4.2+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v4.2+ Cc: Bert Vermeulen <bert@biot.com> --- drivers/spi/spi-rb4xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/spi/spi-rb4xx.c b/drivers/spi/spi-rb4xx.c index 8aa51beb4ff3..9f97d18a05c1 100644 --- a/drivers/spi/spi-rb4xx.c +++ b/drivers/spi/spi-rb4xx.c @@ -143,7 +143,7 @@ static int rb4xx_spi_probe(struct platform_device *pdev) if (IS_ERR(spi_base)) return PTR_ERR(spi_base); - master = spi_alloc_master(&pdev->dev, sizeof(*rbspi)); + master = devm_spi_alloc_master(&pdev->dev, sizeof(*rbspi)); if (!master) return -ENOMEM; -- 2.28.0
If the call to devm_gpiod_get_optional() fails on probe of the NXP SC18IS602/603 SPI driver, the spi_master struct is erroneously not freed. Fix by switching over to the new devm_spi_alloc_master() helper. Fixes: f99008013e19 ("spi: sc18is602: Add reset control via gpio pin.") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v4.9+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v4.9+ Cc: Phil Reid <preid@electromag.com.au> --- drivers/spi/spi-sc18is602.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/drivers/spi/spi-sc18is602.c b/drivers/spi/spi-sc18is602.c index ee0f3edf49cd..297c512069a5 100644 --- a/drivers/spi/spi-sc18is602.c +++ b/drivers/spi/spi-sc18is602.c @@ -238,13 +238,12 @@ static int sc18is602_probe(struct i2c_client *client, struct sc18is602_platform_data *pdata = dev_get_platdata(dev); struct sc18is602 *hw; struct spi_master *master; - int error; if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) return -EINVAL; - master = spi_alloc_master(dev, sizeof(struct sc18is602)); + master = devm_spi_alloc_master(dev, sizeof(struct sc18is602)); if (!master) return -ENOMEM; @@ -298,15 +297,7 @@ static int sc18is602_probe(struct i2c_client *client, master->min_speed_hz = hw->freq / 128; master->max_speed_hz = hw->freq / 4; - error = devm_spi_register_master(dev, master); - if (error) - goto error_reg; - - return 0; - -error_reg: - spi_master_put(master); - return error; + return devm_spi_register_master(dev, master); } static const struct i2c_device_id sc18is602_id[] = { -- 2.28.0
If the call to spi_register_master() fails on probe of the NetUP Universal DVB driver, the spi_master struct is erroneously not freed. Likewise, if spi_new_device() fails, the spi_controller struct is not unregistered. Plug the leaks. While at it, fix an ordering issue in netup_spi_release() wherein spi_unregister_master() is called after fiddling with the IRQ control register. The correct order is to call spi_unregister_master() *before* this teardown step because bus accesses may still be ongoing until that function returns. Fixes: 52b1eaf4c59a ("[media] netup_unidvb: NetUP Universal DVB-S/S2/T/T2/C PCI-E card driver") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v4.3+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v4.3+ Cc: Kozlov Sergey <serjk@netup.ru> --- @Mauro Carvalho Chehab: This patch needs to go in through the spi tree because it depends on commit 5e844cc37a5c, which is on the spi/for-5.10 branch. Please ack (barring any objections). Thanks! drivers/media/pci/netup_unidvb/netup_unidvb_spi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/media/pci/netup_unidvb/netup_unidvb_spi.c b/drivers/media/pci/netup_unidvb/netup_unidvb_spi.c index d4f12c250f91..526042d8afae 100644 --- a/drivers/media/pci/netup_unidvb/netup_unidvb_spi.c +++ b/drivers/media/pci/netup_unidvb/netup_unidvb_spi.c @@ -175,7 +175,7 @@ int netup_spi_init(struct netup_unidvb_dev *ndev) struct spi_master *master; struct netup_spi *nspi; - master = spi_alloc_master(&ndev->pci_dev->dev, + master = devm_spi_alloc_master(&ndev->pci_dev->dev, sizeof(struct netup_spi)); if (!master) { dev_err(&ndev->pci_dev->dev, @@ -208,6 +208,7 @@ int netup_spi_init(struct netup_unidvb_dev *ndev) ndev->pci_slot, ndev->pci_func); if (!spi_new_device(master, &netup_spi_board)) { + spi_unregister_master(master); ndev->spi = NULL; dev_err(&ndev->pci_dev->dev, "%s(): unable to create SPI device\n", __func__); @@ -226,13 +227,13 @@ void netup_spi_release(struct netup_unidvb_dev *ndev) if (!spi) return; + spi_unregister_master(spi->master); spin_lock_irqsave(&spi->lock, flags); reg = readw(&spi->regs->control_stat); writew(reg | NETUP_SPI_CTRL_IRQ, &spi->regs->control_stat); reg = readw(&spi->regs->control_stat); writew(reg & ~NETUP_SPI_CTRL_IMASK, &spi->regs->control_stat); spin_unlock_irqrestore(&spi->lock, flags); - spi_unregister_master(spi->master); ndev->spi = NULL; } -- 2.28.0
On 16.11.20 09:23, Lukas Wunner wrote: > If the calls to device_reset() or devm_spi_register_controller() fail on > probe of the MediaTek MT7621 SPI driver, the spi_controller struct is > erroneously not freed. Fix by switching over to the new > devm_spi_alloc_master() helper. > > Moreover, the SYS clock is enabled on probe but not disabled if any of > the subsequent probe steps fails. > > Finally, there's an ordering issue in mt7621_spi_remove() wherein > the spi_controller is unregistered after disabling the SYS clock. > The correct order is to call spi_unregister_controller() *before* this > teardown step because bus accesses may still be ongoing until that > function returns. > > All of these bugs have existed since the driver was first introduced, > so it seems fair to fix them together in a single commit. > > Fixes: 1ab7f2a43558 ("staging: mt7621-spi: add mt7621 support") > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: <stable@vger.kernel.org> # v4.17+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation > Cc: <stable@vger.kernel.org> # v4.17+ Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > --- > drivers/spi/spi-mt7621.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c > index 2c3b7a2a1ec7..d7cda66c4b26 100644 > --- a/drivers/spi/spi-mt7621.c > +++ b/drivers/spi/spi-mt7621.c > @@ -350,10 +350,11 @@ static int mt7621_spi_probe(struct platform_device *pdev) > if (status) > return status; > > - master = spi_alloc_master(&pdev->dev, sizeof(*rs)); > + master = devm_spi_alloc_master(&pdev->dev, sizeof(*rs)); > if (!master) { > dev_info(&pdev->dev, "master allocation failed\n"); > - return -ENOMEM; > + ret = -ENOMEM; > + goto err_clk_disable; > } > > master->mode_bits = SPI_LSB_FIRST; > @@ -377,10 +378,18 @@ static int mt7621_spi_probe(struct platform_device *pdev) > ret = device_reset(&pdev->dev); > if (ret) { > dev_err(&pdev->dev, "SPI reset failed!\n"); > - return ret; > + goto err_clk_disable; > } > > - return devm_spi_register_controller(&pdev->dev, master); > + ret = spi_register_controller(master); > + if (ret) > + goto err_clk_disable; > + > + return 0; > + > +err_clk_disable: > + clk_disable_unprepare(clk); > + return ret; > } > > static int mt7621_spi_remove(struct platform_device *pdev) > @@ -391,6 +400,7 @@ static int mt7621_spi_remove(struct platform_device *pdev) > master = dev_get_drvdata(&pdev->dev); > rs = spi_controller_get_devdata(master); > > + spi_unregister_controller(master); > clk_disable_unprepare(rs->clk); > > return 0; >
On Mon, Nov 16, 2020 at 12:44 AM Lukas Wunner <lukas@wunner.de> wrote: > > If the call to devm_spi_register_master() fails on probe of the GPIO SPI > driver, the spi_master struct is erroneously not freed: > > After allocating the spi_master, its reference count is 1. The driver > unconditionally decrements the reference count on unbind using a devm > action. Before calling devm_spi_register_master(), the driver > unconditionally increments the reference count because on success, > that function will decrement the reference count on unbind. However on > failure, devm_spi_register_master() does *not* decrement the reference > count, so the spi_master is leaked. Not sure I fully understand this. On failure devm_spi_register_master() will return a negative error code which should result in probe failure and release of devres resource, right? > > The issue was introduced by commits 8b797490b4db ("spi: gpio: Make sure > spi_master_put() is called in every error path") and 79567c1a321e ("spi: > gpio: Use devm_spi_register_master()"), which sought to plug leaks > introduced by 9b00bc7b901f ("spi: spi-gpio: Rewrite to use GPIO > descriptors") but missed this remaining leak. > That extra spi_master_get() that might be problematic was present in the code before 8b797490b4db ("spi: gpio: Make sure spi_master_put() is called in every error path") and I think was first introduced in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers?h=v5.9-rc4&id=702a4879ec337463f858c8ab467482cce260bf18 Or am I missing something? Not really questioning the validity of this fix, just trying to understand what I missed in 8b797490b4db ("spi: gpio: Make sure spi_master_put() is called in every error path") > The situation was later aggravated by commit d3b0ffa1d75d ("spi: gpio: > prevent memory leak in spi_gpio_probe"), which introduced a > use-after-free because it releases a reference on the spi_master if > devm_add_action_or_reset() fails even though the function already > does that. > > Fix by switching over to the new devm_spi_alloc_master() helper. > > Fixes: 9b00bc7b901f ("spi: spi-gpio: Rewrite to use GPIO descriptors") > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: <stable@vger.kernel.org> # v4.17+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation > Cc: <stable@vger.kernel.org> # v5.1-: 8b797490b4db: spi: gpio: Make sure spi_master_put() is called in every error path > Cc: <stable@vger.kernel.org> # v5.1-: 45beec351998: spi: bitbang: Introduce spi_bitbang_init() > Cc: <stable@vger.kernel.org> # v5.1-: 79567c1a321e: spi: gpio: Use devm_spi_register_master() > Cc: <stable@vger.kernel.org> # v5.4-: d3b0ffa1d75d: spi: gpio: prevent memory leak in spi_gpio_probe > Cc: <stable@vger.kernel.org> # v4.17+ > Cc: Navid Emamdoost <navid.emamdoost@gmail.com> > Cc: Andrey Smirnov <andrew.smirnov@gmail.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/spi/spi-gpio.c | 15 ++------------- > 1 file changed, 2 insertions(+), 13 deletions(-) > > diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c > index 7ceb0ba27b75..0584f4d2fde2 100644 > --- a/drivers/spi/spi-gpio.c > +++ b/drivers/spi/spi-gpio.c > @@ -350,11 +350,6 @@ static int spi_gpio_probe_pdata(struct platform_device *pdev, > return 0; > } > > -static void spi_gpio_put(void *data) > -{ > - spi_master_put(data); > -} > - > static int spi_gpio_probe(struct platform_device *pdev) > { > int status; > @@ -363,16 +358,10 @@ static int spi_gpio_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct spi_bitbang *bb; > > - master = spi_alloc_master(dev, sizeof(*spi_gpio)); > + master = devm_spi_alloc_master(dev, sizeof(*spi_gpio)); > if (!master) > return -ENOMEM; > > - status = devm_add_action_or_reset(&pdev->dev, spi_gpio_put, master); > - if (status) { > - spi_master_put(master); > - return status; > - } > - > if (pdev->dev.of_node) > status = spi_gpio_probe_dt(pdev, master); > else > @@ -432,7 +421,7 @@ static int spi_gpio_probe(struct platform_device *pdev) > if (status) > return status; > > - return devm_spi_register_master(&pdev->dev, spi_master_get(master)); > + return devm_spi_register_master(&pdev->dev, master); > } > > MODULE_ALIAS("platform:" DRIVER_NAME); > -- > 2.28.0 >
On Mon, Nov 16, 2020 at 11:23:43AM -0800, Andrey Smirnov wrote: > On Mon, Nov 16, 2020 at 12:44 AM Lukas Wunner <lukas@wunner.de> wrote: > > If the call to devm_spi_register_master() fails on probe of the GPIO SPI > > driver, the spi_master struct is erroneously not freed: > > > > After allocating the spi_master, its reference count is 1. The driver > > unconditionally decrements the reference count on unbind using a devm > > action. Before calling devm_spi_register_master(), the driver > > unconditionally increments the reference count because on success, > > that function will decrement the reference count on unbind. However on > > failure, devm_spi_register_master() does *not* decrement the reference > > count, so the spi_master is leaked. > > Not sure I fully understand this. On failure > devm_spi_register_master() will return a negative error code which > should result in probe failure and release of devres resource, right? Yes, but that just decrements the refcount from 2 to 1: /* refcount initialized to 1 */ master = spi_alloc_master(dev, sizeof(*spi_gpio)); ... /* refcount incremented to 2 */ return devm_spi_register_master(&pdev->dev, spi_master_get(master)); ... /* on failure of devm_spi_register_master(), refcount decremented to 1 by devres action */ spi_gpio_put() > > The issue was introduced by commits 8b797490b4db ("spi: gpio: Make sure > > spi_master_put() is called in every error path") and 79567c1a321e ("spi: > > gpio: Use devm_spi_register_master()"), which sought to plug leaks > > introduced by 9b00bc7b901f ("spi: spi-gpio: Rewrite to use GPIO > > descriptors") but missed this remaining leak. > > That extra spi_master_get() that might be problematic was present in > the code before 8b797490b4db ("spi: gpio: Make sure spi_master_put() > is called in every error path") and I think was first introduced in > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers?h=v5.9-rc4&id=702a4879ec337463f858c8ab467482cce260bf18 > > Or am I missing something? The extra spi_master_get() was introduced by 79567c1a321e. I don't see it in spi-gpio.c before that commit. Its quite possible that I missed something myself, nobody's perfect. But just from code inspection it seems wrong the way it is right now. Shout if I failed to explain it properly and I'll try again. :) Thanks, Lukas
On Mon, Nov 16, 2020 at 3:03 PM Lukas Wunner <lukas@wunner.de> wrote: > > On Mon, Nov 16, 2020 at 11:23:43AM -0800, Andrey Smirnov wrote: > > On Mon, Nov 16, 2020 at 12:44 AM Lukas Wunner <lukas@wunner.de> wrote: > > > If the call to devm_spi_register_master() fails on probe of the GPIO SPI > > > driver, the spi_master struct is erroneously not freed: > > > > > > After allocating the spi_master, its reference count is 1. The driver > > > unconditionally decrements the reference count on unbind using a devm > > > action. Before calling devm_spi_register_master(), the driver > > > unconditionally increments the reference count because on success, > > > that function will decrement the reference count on unbind. However on > > > failure, devm_spi_register_master() does *not* decrement the reference > > > count, so the spi_master is leaked. > > > > Not sure I fully understand this. On failure > > devm_spi_register_master() will return a negative error code which > > should result in probe failure and release of devres resource, right? > > Yes, but that just decrements the refcount from 2 to 1: > > /* refcount initialized to 1 */ > master = spi_alloc_master(dev, sizeof(*spi_gpio)); > > ... > > /* refcount incremented to 2 */ > return devm_spi_register_master(&pdev->dev, spi_master_get(master)); > > ... > > /* on failure of devm_spi_register_master(), refcount decremented to 1 > by devres action */ > spi_gpio_put() > > > > > The issue was introduced by commits 8b797490b4db ("spi: gpio: Make sure > > > spi_master_put() is called in every error path") and 79567c1a321e ("spi: > > > gpio: Use devm_spi_register_master()"), which sought to plug leaks > > > introduced by 9b00bc7b901f ("spi: spi-gpio: Rewrite to use GPIO > > > descriptors") but missed this remaining leak. > > > > That extra spi_master_get() that might be problematic was present in > > the code before 8b797490b4db ("spi: gpio: Make sure spi_master_put() > > is called in every error path") and I think was first introduced in > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers?h=v5.9-rc4&id=702a4879ec337463f858c8ab467482cce260bf18 > > > > Or am I missing something? > > The extra spi_master_get() was introduced by 79567c1a321e. > I don't see it in spi-gpio.c before that commit. > > Its quite possible that I missed something myself, nobody's perfect. > But just from code inspection it seems wrong the way it is right now. > > Shout if I failed to explain it properly and I'll try again. :) > Before 79567c1a321e extra spi_master_get() was a part of spi_bitbang_start(). Ah, OK, it had it's own local spi_master_put() on failure, which I lost when inlinging that function. My bad. Good to see that the with devm_spi_alloc_master() cleanup path is sane and easy to read once again. > Thanks, > > Lukas
On Mon, Nov 16, 2020 at 4:43 PM Lukas Wunner <lukas@wunner.de> wrote: > > If the call to devm_spi_register_controller() fails on probe of the > MediaTek SPI NOR driver, the spi_controller struct is erroneously not > freed. > > Since commit a1daaa991ed1 ("spi: spi-mtk-nor: use dma_alloc_coherent() > for bounce buffer"), the same happens if the call to > dmam_alloc_coherent() fails. > > Since commit 3bfd9103c7af ("spi: spi-mtk-nor: Add power management > support"), the same happens if the call to mtk_nor_enable_clk() fails. > > Fix by switching over to the new devm_spi_alloc_master() helper. > > Fixes: 881d1ee9fe81 ("spi: add support for mediatek spi-nor controller") > Signed-off-by: Lukas Wunner <lukas@wunner.de> Reviewed-by: Ikjoon Jang <ikjn@chromium.org> > Cc: <stable@vger.kernel.org> # v5.7+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation > Cc: <stable@vger.kernel.org> # v5.7+ > Cc: Chuanhong Guo <gch981213@gmail.com> > Cc: Ikjoon Jang <ikjn@chromium.org> > --- > drivers/spi/spi-mtk-nor.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c > index b97f26a60cbe..288f6c2bbd57 100644 > --- a/drivers/spi/spi-mtk-nor.c > +++ b/drivers/spi/spi-mtk-nor.c > @@ -768,7 +768,7 @@ static int mtk_nor_probe(struct platform_device *pdev) > return -EINVAL; > } > > - ctlr = spi_alloc_master(&pdev->dev, sizeof(*sp)); > + ctlr = devm_spi_alloc_master(&pdev->dev, sizeof(*sp)); > if (!ctlr) { > dev_err(&pdev->dev, "failed to allocate spi controller\n"); > return -ENOMEM; > -- > 2.28.0 >
[-- Attachment #1: Type: text/plain, Size: 566 bytes --] On Mon, Nov 16, 2020 at 09:23:08AM +0100, Lukas Wunner wrote: > If the call to devm_spi_register_controller() fails on probe of the > MediaTek SPI NOR driver, the spi_controller struct is erroneously not > freed. Please don't thread things that aren't threads, this breaks tooling that attempts to understand what you're doing - for example b4 thinks every patch in this series is a new revision of a single patch. Just send separate patches with no interdependencies seperately. Please also try to avoid noise like the for-5.10 in the subject line. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
On Mon, 16 Nov 2020 09:23:10 +0100, Lukas Wunner wrote: > If the calls to of_match_device(), of_alias_get_id(), > devm_ioremap_resource(), devm_regmap_init_mmio() or devm_clk_get() > fail on probe of the NPCM FIU SPI driver, the spi_controller struct is > erroneously not freed. > > Fix by switching over to the new devm_spi_alloc_master() helper. Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: npcm-fiu: Don't leak SPI master in probe error path commit: 04a9cd51d3f3308a98cbc6adc07acb12fbade011 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
On Mon, Nov 16, 2020 at 9:44 AM Lukas Wunner <lukas@wunner.de> wrote:
> If the call to devm_spi_register_master() fails on probe of the GPIO SPI
> driver, the spi_master struct is erroneously not freed:
>
> After allocating the spi_master, its reference count is 1. The driver
> unconditionally decrements the reference count on unbind using a devm
> action. Before calling devm_spi_register_master(), the driver
> unconditionally increments the reference count because on success,
> that function will decrement the reference count on unbind. However on
> failure, devm_spi_register_master() does *not* decrement the reference
> count, so the spi_master is leaked.
>
> The issue was introduced by commits 8b797490b4db ("spi: gpio: Make sure
> spi_master_put() is called in every error path") and 79567c1a321e ("spi:
> gpio: Use devm_spi_register_master()"), which sought to plug leaks
> introduced by 9b00bc7b901f ("spi: spi-gpio: Rewrite to use GPIO
> descriptors") but missed this remaining leak.
>
> The situation was later aggravated by commit d3b0ffa1d75d ("spi: gpio:
> prevent memory leak in spi_gpio_probe"), which introduced a
> use-after-free because it releases a reference on the spi_master if
> devm_add_action_or_reset() fails even though the function already
> does that.
>
> Fix by switching over to the new devm_spi_alloc_master() helper.
>
> Fixes: 9b00bc7b901f ("spi: spi-gpio: Rewrite to use GPIO descriptors")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: <stable@vger.kernel.org> # v4.17+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation
> Cc: <stable@vger.kernel.org> # v5.1-: 8b797490b4db: spi: gpio: Make sure spi_master_put() is called in every error path
> Cc: <stable@vger.kernel.org> # v5.1-: 45beec351998: spi: bitbang: Introduce spi_bitbang_init()
> Cc: <stable@vger.kernel.org> # v5.1-: 79567c1a321e: spi: gpio: Use devm_spi_register_master()
> Cc: <stable@vger.kernel.org> # v5.4-: d3b0ffa1d75d: spi: gpio: prevent memory leak in spi_gpio_probe
> Cc: <stable@vger.kernel.org> # v4.17+
> Cc: Navid Emamdoost <navid.emamdoost@gmail.com>
> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
That's a really good fix. Thanks to looking into this!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
Em Mon, 16 Nov 2020 09:23:13 +0100 Lukas Wunner <lukas@wunner.de> escreveu: > If the call to spi_register_master() fails on probe of the NetUP > Universal DVB driver, the spi_master struct is erroneously not freed. > > Likewise, if spi_new_device() fails, the spi_controller struct is > not unregistered. Plug the leaks. > > While at it, fix an ordering issue in netup_spi_release() wherein > spi_unregister_master() is called after fiddling with the IRQ control > register. The correct order is to call spi_unregister_master() *before* > this teardown step because bus accesses may still be ongoing until that > function returns. > > Fixes: 52b1eaf4c59a ("[media] netup_unidvb: NetUP Universal DVB-S/S2/T/T2/C PCI-E card driver") > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: <stable@vger.kernel.org> # v4.3+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation > Cc: <stable@vger.kernel.org> # v4.3+ > Cc: Kozlov Sergey <serjk@netup.ru> > --- > @Mauro Carvalho Chehab: > This patch needs to go in through the spi tree because it depends on > commit 5e844cc37a5c, which is on the spi/for-5.10 branch. > Please ack (barring any objections). Thanks! Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> I'm OK on having this merged via SPI mailing list. > > drivers/media/pci/netup_unidvb/netup_unidvb_spi.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/pci/netup_unidvb/netup_unidvb_spi.c b/drivers/media/pci/netup_unidvb/netup_unidvb_spi.c > index d4f12c250f91..526042d8afae 100644 > --- a/drivers/media/pci/netup_unidvb/netup_unidvb_spi.c > +++ b/drivers/media/pci/netup_unidvb/netup_unidvb_spi.c > @@ -175,7 +175,7 @@ int netup_spi_init(struct netup_unidvb_dev *ndev) > struct spi_master *master; > struct netup_spi *nspi; > > - master = spi_alloc_master(&ndev->pci_dev->dev, > + master = devm_spi_alloc_master(&ndev->pci_dev->dev, > sizeof(struct netup_spi)); > if (!master) { > dev_err(&ndev->pci_dev->dev, > @@ -208,6 +208,7 @@ int netup_spi_init(struct netup_unidvb_dev *ndev) > ndev->pci_slot, > ndev->pci_func); > if (!spi_new_device(master, &netup_spi_board)) { > + spi_unregister_master(master); > ndev->spi = NULL; > dev_err(&ndev->pci_dev->dev, > "%s(): unable to create SPI device\n", __func__); > @@ -226,13 +227,13 @@ void netup_spi_release(struct netup_unidvb_dev *ndev) > if (!spi) > return; > > + spi_unregister_master(spi->master); > spin_lock_irqsave(&spi->lock, flags); > reg = readw(&spi->regs->control_stat); > writew(reg | NETUP_SPI_CTRL_IRQ, &spi->regs->control_stat); > reg = readw(&spi->regs->control_stat); > writew(reg & ~NETUP_SPI_CTRL_IMASK, &spi->regs->control_stat); > spin_unlock_irqrestore(&spi->lock, flags); > - spi_unregister_master(spi->master); > ndev->spi = NULL; > } > Thanks, Mauro
Hello! On 11/16/20 11:23 AM, Lukas Wunner wrote: > rpcif_spi_remove() accesses the driver's private data after calling > spi_unregister_controller() even though that function releases the last > reference on the spi_controller and thereby frees the private data. OK, your analysis seems correct (sorry for the delay admitting this :-). Not sure why spi_unregister_controller() drops the device reference while spi_register_controller() itself doesn't allocate the memory... > Fix by switching over to the new devm_spi_alloc_master() helper which > keeps the private data accessible until the driver has unbound. Perhaps the order of the calls in the remove() method could be reversed? > Fixes: eb8d6d464a27 ("spi: add Renesas RPC-IF driver") > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: <stable@vger.kernel.org> # v5.9+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation > Cc: <stable@vger.kernel.org> # v5.9+ > Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [...] MBR, Sergei
On Sat, Nov 28, 2020 at 11:20:50PM +0300, Sergey Shtylyov wrote: > On 11/16/20 11:23 AM, Lukas Wunner wrote: > > rpcif_spi_remove() accesses the driver's private data after calling > > spi_unregister_controller() even though that function releases the last > > reference on the spi_controller and thereby frees the private data. > > OK, your analysis seems correct (sorry for the delay admitting this :-). Thanks! Is it okay to take this for an Acked-by? > Not sure why spi_unregister_controller() drops the device reference > while spi_register_controller() itself doesn't allocate the memory... Yes, that's exactly what I'm trying to move away from with devm_spi_alloc_master() (introduced in v5.10-rc5 by 5e844cc37a5c). The API as it has been so far has made it really easy to shoot oneself in the foot. > > Fix by switching over to the new devm_spi_alloc_master() helper which > > keeps the private data accessible until the driver has unbound. > > Perhaps the order of the calls in the remove() method could be reversed? I'm not familiar with power management on these Renesas controllers but rpcif_disable_rpm() calls pm_runtime_put_sync(), which I assume may put the controller to sleep. SPI transfers may still be ongoing until spi_unregister_controller() returns. Specifically, this function unbinds and unregisters all SPI slaves attached to the controller and the slaves' drivers may need to perform SPI transfers to quiesce interrupts on the slaves etc. Thus, the correct order is to call spi_unregister_controller() first and only then perform further teardown steps. So the order in rpcif_spi_remove() seems correct to me. The only thing that looks confusing is that rpcif_enable_rpm() calls pm_runtime_enable(), whereas rpcif_disable_rpm() calls pm_runtime_put_sync(). That looks incongruent. Thanks, Lukas
On 11/29/20 2:35 PM, Lukas Wunner wrote: >>> rpcif_spi_remove() accesses the driver's private data after calling >>> spi_unregister_controller() even though that function releases the last >>> reference on the spi_controller and thereby frees the private data. >> >> OK, your analysis seems correct (sorry for the delay admitting this :-). > > Thanks! Is it okay to take this for an Acked-by? Not yet. :-) >> Not sure why spi_unregister_controller() drops the device reference >> while spi_register_controller() itself doesn't allocate the memory... > > Yes, that's exactly what I'm trying to move away from with > devm_spi_alloc_master() (introduced in v5.10-rc5 by 5e844cc37a5c). > The API as it has been so far has made it really easy to shoot oneself > in the foot. Maybe it needs to be fixed, rather than using the managed device API? >>> Fix by switching over to the new devm_spi_alloc_master() helper which >>> keeps the private data accessible until the driver has unbound. >> >> Perhaps the order of the calls in the remove() method could be reversed? > > I'm not familiar with power management on these Renesas controllers > but rpcif_disable_rpm() calls pm_runtime_put_sync(), which I assume > may put the controller to sleep. Sigh, that's a stupid typo on my part, being fixed now to pm_runtim_disable()... > SPI transfers may still be ongoing until spi_unregister_controller() > returns. Specifically, this function unbinds and unregisters all > SPI slaves attached to the controller and the slaves' drivers may > need to perform SPI transfers to quiesce interrupts on the slaves etc. > > Thus, the correct order is to call spi_unregister_controller() first > and only then perform further teardown steps. So the order in > rpcif_spi_remove() seems correct to me. OK. :-) > The only thing that looks confusing is that rpcif_enable_rpm() calls > pm_runtime_enable(), whereas rpcif_disable_rpm() calls > pm_runtime_put_sync(). That looks incongruent. Do you need a link to the fix (it a whole patchset of minor fixes)? > Thanks, > > Lukas MBR, Sergei
On Mon, 16 Nov 2020 09:23:13 +0100, Lukas Wunner wrote: > If the call to spi_register_master() fails on probe of the NetUP > Universal DVB driver, the spi_master struct is erroneously not freed. > > Likewise, if spi_new_device() fails, the spi_controller struct is > not unregistered. Plug the leaks. > > While at it, fix an ordering issue in netup_spi_release() wherein > spi_unregister_master() is called after fiddling with the IRQ control > register. The correct order is to call spi_unregister_master() *before* > this teardown step because bus accesses may still be ongoing until that > function returns. Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] media: netup_unidvb: Don't leak SPI master in probe error path (no commit info) 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
On Mon, 16 Nov 2020 09:23:10 +0100, Lukas Wunner wrote: > If the calls to of_match_device(), of_alias_get_id(), > devm_ioremap_resource(), devm_regmap_init_mmio() or devm_clk_get() > fail on probe of the NPCM FIU SPI driver, the spi_controller struct is > erroneously not freed. > > Fix by switching over to the new devm_spi_alloc_master() helper. Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] spi: npcm-fiu: Don't leak SPI master in probe error path (no commit info) 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
On Tue, Dec 01, 2020 at 01:57:56PM +0000, Mark Brown wrote:
> On Mon, 16 Nov 2020 09:23:10 +0100, Lukas Wunner wrote:
> > If the calls to of_match_device(), of_alias_get_id(),
> > devm_ioremap_resource(), devm_regmap_init_mmio() or devm_clk_get()
> > fail on probe of the NPCM FIU SPI driver, the spi_controller struct is
> > erroneously not freed.
> >
> > Fix by switching over to the new devm_spi_alloc_master() helper.
>
> Applied to
>
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
>
> Thanks!
>
> [1/1] spi: npcm-fiu: Don't leak SPI master in probe error path
> (no commit info)
This patch is already in Linus' tree.
(You applied it to spi.git on Nov 17.)
Thanks,
Lukas
[-- Attachment #1: Type: text/plain, Size: 623 bytes --] On Tue, Dec 01, 2020 at 03:30:27PM +0100, Lukas Wunner wrote: > On Tue, Dec 01, 2020 at 01:57:56PM +0000, Mark Brown wrote: > > [1/1] spi: npcm-fiu: Don't leak SPI master in probe error path > > (no commit info) > This patch is already in Linus' tree. > (You applied it to spi.git on Nov 17.) Yes, b4 had a bug which caused it to generate notification e-mails for things it didn't find in git (as you can see from the "no commit info" bit). BTW it would be really helpful if you could resend this stuff in some more normal fashion (either independently or as a numbered thread), it's really breaking my workflow. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
On Tue, Dec 01, 2020 at 05:17:26PM +0000, Mark Brown wrote:
> BTW it would be really helpful if you could resend this
> stuff in some more normal fashion (either independently or as a numbered
> thread), it's really breaking my workflow.
Will do, sorry for the inconvenience.
I think I'll base the resent patches on for-5.11, Linus would probably
not be happy to receive such a large quantity of commits this late in the
cycle. (Shout if you disagree.)
Thanks,
Lukas
On Mon, Nov 30, 2020 at 10:18:12PM +0300, Sergey Shtylyov wrote: > On 11/29/20 2:35 PM, Lukas Wunner wrote: > > > Not sure why spi_unregister_controller() drops the device reference > > > while spi_register_controller() itself doesn't allocate the memory... > > > > Yes, that's exactly what I'm trying to move away from with > > devm_spi_alloc_master() (introduced in v5.10-rc5 by 5e844cc37a5c). > > The API as it has been so far has made it really easy to shoot oneself > > in the foot. > > Maybe it needs to be fixed, rather than using the managed device API? devm_spi_alloc_master() *is* the fix, or at least a means to get there: No longer dropping the reference in spi_unregister_controller() requires that the drivers drop the reference. So every single SPI driver needs to be touched. However, upon closer examination I've found tons of bugs in the ->probe and ->remove hooks of SPI drivers, some of them related to reference counting (leaks or use-after-free), others related to not disabling clocks properly etc. Ideally, the fixes for those bugs should be backported to stable. devm_spi_alloc_master() allows me to do that and at the same time it allows stretching the migration across multiple releases. That's because spi_unregister_controller() auto-senses if devm_spi_alloc_master() was used, and if so, it no longer drops a reference. devm_spi_alloc_master() has the additional advantage of simplifying probe error paths, as is apparent from the diffstat of the $subject patch: drivers/spi/spi-rpc-if.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) I think the vast majority of SPI drivers can be converted to devm_spi_alloc_master() and the few that can't will be amended to explicitly drop a reference. > > > Perhaps the order of the calls in the remove() method could be reversed? > > > > I'm not familiar with power management on these Renesas controllers > > but rpcif_disable_rpm() calls pm_runtime_put_sync(), which I assume > > may put the controller to sleep. > > Sigh, that's a stupid typo on my part, being fixed now to > pm_runtim_disable()... Okay in that case the order of the two calls in rpcif_spi_remove() won't matter, i.e. it would actually be possible to fix the UAF by calling rpcif_disable_rpm() before spi_unregister_controller(). However, I still recommend fixing the UAF in the way proposed by the $subject patch because of the simplified probe error path and reduced LoC. > > The only thing that looks confusing is that rpcif_enable_rpm() calls > > pm_runtime_enable(), whereas rpcif_disable_rpm() calls > > pm_runtime_put_sync(). That looks incongruent. > > Do you need a link to the fix (it a whole patchset of minor fixes)? I don't *need* it, but am happy to take a look. Glad that I was able to point out another bug. :) Thanks, Lukas
[-- Attachment #1: Type: text/plain, Size: 278 bytes --] On Tue, Dec 01, 2020 at 06:49:08PM +0100, Lukas Wunner wrote: > I think I'll base the resent patches on for-5.11, Linus would probably > not be happy to receive such a large quantity of commits this late in the > cycle. (Shout if you disagree.) That's probably sensible yes. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]