| Message ID | 73adc6ba84a4f968f2e1499a776e5c928fbdde56.1605512876.git.lukas@wunner.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-5.10] spi: spi-geni-qcom: Fix use-after-free on unbind | expand |
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.
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
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
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
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
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
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.
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
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.
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);
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(-)