diff mbox series

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

Message ID 49102f5bbb3f1592d9cfd7b39ac5e131a031f950.1605512876.git.lukas@wunner.de
State Accepted
Commit 7174dc655ef0578877b0b4598e69619d2be28b4d
Headers show
Series [for-5.10] spi: gpio: Don't leak SPI master in probe error path | expand

Commit Message

Lukas Wunner Nov. 16, 2020, 8:23 a.m. UTC
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(-)

Comments

Linus Walleij Nov. 18, 2020, 1:08 a.m. UTC | #1
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
diff mbox series

Patch

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