[PATCH] Revert "spi: simplify devm_spi_register_controller"

[PATCH] Revert "spi: simplify devm_spi_register_controller"

[Yang Yingliang]

This reverts commit 59ebbe40fb51e307032ae7f63b2749fad2d4635a.

If devm_add_action() fails in devm_add_action_or_reset(),
devm_spi_unregister() will be called, it decreases the
refcount of 'ctlr->dev' to 0, then it will cause uaf in
the drivers that calling spi_put_controller() in error path.

Fixes: 59ebbe40fb51 ("spi: simplify devm_spi_register_controller")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/spi/spi.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index dc1a324e3271..ef751ccd65be 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3130,9 +3130,9 @@ int spi_register_controller(struct spi_controller *ctlr)
 }
 EXPORT_SYMBOL_GPL(spi_register_controller);
 
-static void devm_spi_unregister(void *ctlr)
+static void devm_spi_unregister(struct device *dev, void *res)
 {
-	spi_unregister_controller(ctlr);
+	spi_unregister_controller(*(struct spi_controller **)res);
 }
 
 /**
@@ -3151,13 +3151,22 @@ static void devm_spi_unregister(void *ctlr)
 int devm_spi_register_controller(struct device *dev,
 				 struct spi_controller *ctlr)
 {
+	struct spi_controller **ptr;
 	int ret;
 
+	ptr = devres_alloc(devm_spi_unregister, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
 	ret = spi_register_controller(ctlr);
-	if (ret)
-		return ret;
+	if (!ret) {
+		*ptr = ctlr;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
 
-	return devm_add_action_or_reset(dev, devm_spi_unregister, ctlr);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(devm_spi_register_controller);
 
-- 
2.25.1

Re: [PATCH] Revert "spi: simplify devm_spi_register_controller"

[Conor.Dooley]

On 12/07/2022 14:55, Yang Yingliang wrote:
> This reverts commit 59ebbe40fb51e307032ae7f63b2749fad2d4635a.
> 
> If devm_add_action() fails in devm_add_action_or_reset(),
> devm_spi_unregister() will be called, it decreases the
> refcount of 'ctlr->dev' to 0, then it will cause uaf in
> the drivers that calling spi_put_controller() in error path.

Whether a revert is the right fix or not, this is the same
conclusion I came to reading your patch for my driver & on
that basis:

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Seems like the master variant of this is used over 40 times:
rg "(?s)devm_spi_register_master.*master_put" drivers/spi --multiline -l

> 
> Fixes: 59ebbe40fb51 ("spi: simplify devm_spi_register_controller")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>   drivers/spi/spi.c | 19 ++++++++++++++-----
>   1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index dc1a324e3271..ef751ccd65be 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -3130,9 +3130,9 @@ int spi_register_controller(struct spi_controller *ctlr)
>   }
>   EXPORT_SYMBOL_GPL(spi_register_controller);
>   
> -static void devm_spi_unregister(void *ctlr)
> +static void devm_spi_unregister(struct device *dev, void *res)
>   {
> -	spi_unregister_controller(ctlr);
> +	spi_unregister_controller(*(struct spi_controller **)res);
>   }
>   
>   /**
> @@ -3151,13 +3151,22 @@ static void devm_spi_unregister(void *ctlr)
>   int devm_spi_register_controller(struct device *dev,
>   				 struct spi_controller *ctlr)
>   {
> +	struct spi_controller **ptr;
>   	int ret;
>   
> +	ptr = devres_alloc(devm_spi_unregister, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
>   	ret = spi_register_controller(ctlr);
> -	if (ret)
> -		return ret;
> +	if (!ret) {
> +		*ptr = ctlr;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
>   
> -	return devm_add_action_or_reset(dev, devm_spi_unregister, ctlr);
> +	return ret;
>   }
>   EXPORT_SYMBOL_GPL(devm_spi_register_controller);
>   

Re: [PATCH] Revert "spi: simplify devm_spi_register_controller"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

On Tue, Jul 12, 2022 at 09:55:04PM +0800, Yang Yingliang wrote:
> This reverts commit 59ebbe40fb51e307032ae7f63b2749fad2d4635a.
> 
> If devm_add_action() fails in devm_add_action_or_reset(),
> devm_spi_unregister() will be called, it decreases the

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Revert "spi: simplify devm_spi_register_controller"

[Yang Yingliang]

On 2022/7/13 20:42, Mark Brown wrote:
> On Tue, Jul 12, 2022 at 09:55:04PM +0800, Yang Yingliang wrote:
>> This reverts commit 59ebbe40fb51e307032ae7f63b2749fad2d4635a.
>>
>> If devm_add_action() fails in devm_add_action_or_reset(),
>> devm_spi_unregister() will be called, it decreases the
> Please submit patches using subject lines reflecting the style for the
> subsystem, this makes it easier for people to identify relevant patches.
> Look at what existing commits in the area you're changing are doing and
> make sure your subject lines visually resemble what they're doing.
> There's no need to resubmit to fix this alone.
>
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.
OK. Thanks for changing the subject to apply it, I will make my patch 
more readable next time.

Thanks,
Yang

Re: [PATCH] Revert "spi: simplify devm_spi_register_controller"

[Mark Brown]

On Tue, 12 Jul 2022 21:55:04 +0800, Yang Yingliang wrote:
> This reverts commit 59ebbe40fb51e307032ae7f63b2749fad2d4635a.
> 
> If devm_add_action() fails in devm_add_action_or_reset(),
> devm_spi_unregister() will be called, it decreases the
> refcount of 'ctlr->dev' to 0, then it will cause uaf in
> the drivers that calling spi_put_controller() in error path.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] Revert "spi: simplify devm_spi_register_controller"
      commit: 43cc5a0afe4184a7fafe1eba32b5a11bb69c9ce0

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