| Message ID | 20260902062153.31048-9-logang@deltatee.com |
|---|---|
| State | New |
| Headers | show |
| Series | dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [High] Use-After-Free of `ioat_chan` and `ioatdma_device` structures on `ioat3_dma_probe` error path. -- commit 5181bf41c6d9f7fa2de9a1b9a5e45637220e6c4f Author: Logan Gunthorpe <logang@deltatee.com> dmaengine: ioat: disable relaxed ordering before registering the device This patch reorders the initialization sequence in ioat3_dma_probe(). It moves the PCIe relaxed ordering configuration before dma_async_device_register() to prevent leaving a dangling DMA device registration on the error path. Link: https://lore.kernel.org/dmaengine/20260707165906.249F41F000E9@smtp.kernel.org > diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c > index 737496391109e..a57024c4b066e 100644 > --- a/drivers/dma/ioat/init.c > +++ b/drivers/dma/ioat/init.c [ ... ] > @@ -1194,6 +1185,15 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca) > goto err_disable_interrupts; > } > > + err = dma_async_device_register(&ioat_dma->dma_dev); > + if (err) > + goto err_disable_interrupts; [Severity: High] This isn't a bug introduced by this patch, but could this error path lead to a use-after-free if a shared interrupt fires? If ioat3_dma_probe() fails here, it returns the error back to ioat_pci_probe(), which immediately frees the device and its channels manually: drivers/dma/ioat/init.c:ioat_pci_probe() { ... for (i = 0; i < IOAT_MAX_CHANS; i++) kfree(device->idx[i]); kfree(device); ... } However, the interrupts were previously registered using devm_request_irq() in ioat_dma_setup_interrupts(): drivers/dma/ioat/init.c:ioat_dma_setup_interrupts() { ... err = devm_request_irq(dev, pdev->irq, ioat_dma_do_interrupt, IRQF_SHARED, "ioat-intx", ioat_dma); ... } Because devm managed resources are not cleaned up until the callbacks run after the probe function returns, the IRQ handler remains active during this window. If a shared interrupt or pending MSI/MSI-X fires before devres cleans up the IRQ, will the handler dereference the already-freed ioatdma_device and ioat_chan structures?
diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c index 737496391109..a57024c4b066 100644 --- a/drivers/dma/ioat/init.c +++ b/drivers/dma/ioat/init.c @@ -1170,15 +1170,6 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca) ioat_chan->reg_base + IOAT_DCACTRL_OFFSET); } - err = dma_async_device_register(&ioat_dma->dma_dev); - if (err) - goto err_disable_interrupts; - - ioat_kobject_add(ioat_dma, &ioat_ktype); - - if (dca) - ioat_dma->dca = ioat_dca_init(pdev, ioat_dma->reg_base); - /* disable relaxed ordering */ err = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &val16); if (err) { @@ -1194,6 +1185,15 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca) goto err_disable_interrupts; } + err = dma_async_device_register(&ioat_dma->dma_dev); + if (err) + goto err_disable_interrupts; + + ioat_kobject_add(ioat_dma, &ioat_ktype); + + if (dca) + ioat_dma->dca = ioat_dca_init(pdev, ioat_dma->reg_base); + if (ioat_dma->cap & IOAT_CAP_DPS) writeb(ioat_pending_level + 1, ioat_dma->reg_base + IOAT_PREFETCH_LIMIT_OFFSET);
ioat3_dma_probe() disabled PCIe relaxed ordering after calling dma_async_device_register(), so if an error occurs and the code jumps to err_disable_interrupts, the function returns with the device still registered in the core's dma_device_list while the caller frees the ioatdma_device struct, leaving a dangling registration that anything walking the device list can dereference after it's been freed. Move the capability read/write ahead of dma_async_device_register() instead. Nothing after registration depends on relaxed ordering already being disabled, and nothing before it depends on the device being registered, so this is a plain reordering. It also means every remaining step after registration can't fail, so there's no need to ever have to unregister the device once registered. Fixes: 511deae0261c ("dmaengine: ioatdma: disable relaxed ordering for ioatdma") Reported-by: Sashiko <sashiko-bot@kernel.org> Link: https://lore.kernel.org/dmaengine/20260707165906.249F41F000E9@smtp.kernel.org Acked-by: Dave Jiang <dave.jiang@intel.com> Reviewed-by: Frank Li <Frank.Li@nxp.com> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- drivers/dma/ioat/init.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)