| Message ID | 49102f5bbb3f1592d9cfd7b39ac5e131a031f950.1605512876.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7174dc655ef0578877b0b4598e69619d2be28b4d |
| Headers | show |
| Series | [for-5.10] spi: gpio: Don't leak SPI master in probe error path | expand |
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 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 --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);
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(-)