| Message ID | 20171029115625.32385-3-johan@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4d5e0689dc9d5640ad46cdfbe1896b74d8df1661 |
| Headers | show |
On 10/29/2017 12:56 PM, Johan Hovold wrote: > Take an extra reference to the controller to avoid use-after-free in > free_irq() which is called only after the controller has been > deregistered and freed. > > Note that this is not an issue for this particular driver which does not > use shared interrupts, but free_irq() could otherwise end up accessing > the freed controller when CONFIG_DEBUG_SHIRQ is set. Strictly speaking there is no guarantee that the IRQ handler does not run until free_irq() has been called. And since the SPI master is referenced in the IRQ handler there could be an use-after-free condition. So there is kind of a real issue here as well. But it should be really really hard to trigger it unless the hardware misbehaves. > > Defer controller release until free_irq() returns to prevent this > from ever becoming an issue should this code be replicated in other > drivers. > > Cc: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: Johan Hovold <johan@kernel.org> Acked-by: Lars-Peter Clausen <lars@metafoo.de> Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 30, 2017 at 10:48:23AM +0100, Lars-Peter Clausen wrote: > On 10/29/2017 12:56 PM, Johan Hovold wrote: > > Take an extra reference to the controller to avoid use-after-free in > > free_irq() which is called only after the controller has been > > deregistered and freed. > > > > Note that this is not an issue for this particular driver which does not > > use shared interrupts, but free_irq() could otherwise end up accessing > > the freed controller when CONFIG_DEBUG_SHIRQ is set. > > Strictly speaking there is no guarantee that the IRQ handler does not run > until free_irq() has been called. And since the SPI master is referenced in > the IRQ handler there could be an use-after-free condition. So there is kind > of a real issue here as well. But it should be really really hard to trigger > it unless the hardware misbehaves. You're right of course. Let me update the commit message in a v2 of the series. > > Defer controller release until free_irq() returns to prevent this > > from ever becoming an issue should this code be replicated in other > > drivers. > > > > Cc: Lars-Peter Clausen <lars@metafoo.de> > > Signed-off-by: Johan Hovold <johan@kernel.org> > > Acked-by: Lars-Peter Clausen <lars@metafoo.de> Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c index 6ab4c7700228..68cfc351b47f 100644 --- a/drivers/spi/spi-axi-spi-engine.c +++ b/drivers/spi/spi-axi-spi-engine.c @@ -553,7 +553,7 @@ static int spi_engine_probe(struct platform_device *pdev) static int spi_engine_remove(struct platform_device *pdev) { - struct spi_master *master = platform_get_drvdata(pdev); + struct spi_master *master = spi_master_get(platform_get_drvdata(pdev)); struct spi_engine *spi_engine = spi_master_get_devdata(master); int irq = platform_get_irq(pdev, 0); @@ -561,6 +561,8 @@ static int spi_engine_remove(struct platform_device *pdev) free_irq(irq, master); + spi_master_put(master); + writel_relaxed(0xff, spi_engine->base + SPI_ENGINE_REG_INT_PENDING); writel_relaxed(0x00, spi_engine->base + SPI_ENGINE_REG_INT_ENABLE); writel_relaxed(0x01, spi_engine->base + SPI_ENGINE_REG_RESET);
Take an extra reference to the controller to avoid use-after-free in free_irq() which is called only after the controller has been deregistered and freed. Note that this is not an issue for this particular driver which does not use shared interrupts, but free_irq() could otherwise end up accessing the freed controller when CONFIG_DEBUG_SHIRQ is set. Defer controller release until free_irq() returns to prevent this from ever becoming an issue should this code be replicated in other drivers. Cc: Lars-Peter Clausen <lars@metafoo.de> Signed-off-by: Johan Hovold <johan@kernel.org> --- drivers/spi/spi-axi-spi-engine.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)