| Message ID | 20220712135504.1055688-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 43cc5a0afe4184a7fafe1eba32b5a11bb69c9ce0 |
| Headers | show |
| Series | Revert "spi: simplify devm_spi_register_controller" | expand |
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); >
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.
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
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
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);
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(-)