[PATCH for-5.10] spi: spi-geni-qcom: Fix use-after-free on unbind

[PATCH for-5.10] spi: spi-geni-qcom: Fix use-after-free on unbind

[Lukas Wunner]

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

[PATCH for-5.10] spi: spi-qcom-qspi: Fix use-after-free on unbind

[Lukas Wunner]

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

[PATCH for-5.10] spi: spi-sh: Fix use-after-free on unbind

[Lukas Wunner]

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

[PATCH for-5.10] spi: pxa2xx: Fix use-after-free on unbind

[Lukas Wunner]

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

[PATCH for-5.10] spi: rpc-if: Fix use-after-free on unbind

[Lukas Wunner]

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

Re: [PATCH for-5.10] spi: rpc-if: Fix use-after-free on unbind

[Sergey Shtylyov]

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

Re: [PATCH for-5.10] spi: rpc-if: Fix use-after-free on unbind

[Lukas Wunner]

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

Re: [PATCH for-5.10] spi: rpc-if: Fix use-after-free on unbind

[Sergey Shtylyov]

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

Re: [PATCH for-5.10] spi: rpc-if: Fix use-after-free on unbind

[Lukas Wunner]

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

[PATCH for-5.10] spi: mxic: Don't leak SPI master in probe error path

[Lukas Wunner]

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

[PATCH for-5.10] spi: mt7621: Don't leak SPI master in probe error path

[Lukas Wunner]

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

Re: [PATCH for-5.10] spi: mt7621: Don't leak SPI master in probe error path

[Stefan Roese]

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

[PATCH for-5.10] spi: spi-mtk-nor: Don't leak SPI master in probe error path

[Lukas Wunner]

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

Re: [PATCH for-5.10] spi: spi-mtk-nor: Don't leak SPI master in probe error path

[Ikjoon Jang]

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
>

Re: [PATCH for-5.10] spi: spi-mtk-nor: Don't leak SPI master in probe error path

[Mark Brown]

[-- 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 --]

[PATCH for-5.10] spi: gpio: Don't leak SPI master in probe error path

[Lukas Wunner]

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

Re: [PATCH for-5.10] spi: gpio: Don't leak SPI master in probe error path

[Andrey Smirnov]

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
>

Re: [PATCH for-5.10] spi: gpio: Don't leak SPI master in probe error path

[Lukas Wunner]

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

Re: [PATCH for-5.10] spi: gpio: Don't leak SPI master in probe error path

[Andrey Smirnov]

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

Re: [PATCH for-5.10] spi: gpio: Don't leak SPI master in probe error path

[Linus Walleij]

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

[PATCH for-5.10] spi: npcm-fiu: Don't leak SPI master in probe error path

[Lukas Wunner]

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

Re: [PATCH for-5.10] spi: npcm-fiu: Don't leak SPI master in probe error path

[Mark Brown]

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

Re: [PATCH for-5.10] spi: npcm-fiu: Don't leak SPI master in probe error path

[Mark Brown]

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

Re: [PATCH for-5.10] spi: npcm-fiu: Don't leak SPI master in probe error path

[Lukas Wunner]

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

Re: [PATCH for-5.10] spi: npcm-fiu: Don't leak SPI master in probe error path

[Mark Brown]

[-- 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 --]

Re: [PATCH for-5.10] spi: npcm-fiu: Don't leak SPI master in probe error path

[Lukas Wunner]

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

Re: [PATCH for-5.10] spi: npcm-fiu: Don't leak SPI master in probe error path

[Mark Brown]

[-- 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 --]

[PATCH for-5.10] spi: rb4xx: Don't leak SPI master in probe error path

[Lukas Wunner]

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

[PATCH for-5.10] spi: sc18is602: Don't leak SPI master in probe error path

[Lukas Wunner]

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

[PATCH for-5.10] media: netup_unidvb: Don't leak SPI master in probe error path

[Lukas Wunner]

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

Re: [PATCH for-5.10] media: netup_unidvb: Don't leak SPI master in probe error path

[Mauro Carvalho Chehab]

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

Re: [PATCH for-5.10] media: netup_unidvb: Don't leak SPI master in probe error path

[Mark Brown]

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