Re: [PATCH v4] dma: at_hdmac: Fix use-after-free by proper tasklet cleanup

[Hongling Zeng]

在 2026年07月09日 05:32, Frank Li 写道:
> On Wed, Jul 08, 2026 at 10:59:59AM +0800, Hongling Zeng wrote:
> > Current cleanup paths have a use-after-free vulnerability:
> > - vchan_init() creates tasklets that access at_dma_chan memory
> > - free_irq() only waits for IRQ handler, NOT tasklets
> > - atdma is devm-managed and freed after probe/remove
> > - Running tasklets accessing freed memory → Use-After-Free!
> >
> > The fix requires careful ordering:
> > - free_irq() FIRST to synchronize with running IRQ handlers and prevent
> >    them from scheduling new tasklets
> > - Then kill tasklets to wait for already-scheduled ones to complete
> > - Only then free other resources
> >
> > Fixes: ac803b56860f ("dmaengine: at_hdmac: Convert driver to use virt-dma")
> > Reported-by: sashiko-bot@xxxxxxxxxx
> > Closes: https://lore.kernel.org/all/20260604073945.54B311F00898@xxxxxxxxxxxxxxx/
> > Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> >
> > ---
> > Change in v4:
> >    - Fix error path fallthrough causing double-free_irq()
> >    - Use channel iteration index (chan_id not initialized before registration)
> >    - Remove unnecessary defensive checks
> > ---
> >   drivers/dma/at_hdmac.c | 31 ++++++++++++++++++++++---------
> >   1 file changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> > index e5b30a57c477..044a0fb38b7a 100644
> > --- a/drivers/dma/at_hdmac.c
> > +++ b/drivers/dma/at_hdmac.c
> > @@ -1940,6 +1940,20 @@ static void at_dma_off(struct at_dma *atdma)
> >   		cpu_relax();
> >   }
> >
> > +static void at_dma_cleanup_channels(struct at_dma *atdma)
> > +{
> > +	struct dma_chan *chan, *_chan;
> > +	int i = 0;
> > +
> > +	list_for_each_entry_safe(chan, _chan, &atdma->dma_device.channels,
> > +			device_node) {
> > +		/* Disable interrupts */
> > +		atc_disable_chan_irq(atdma, i++);
> > +		tasklet_kill(&to_at_dma_chan(chan)->vc.task);
> > +		list_del(&chan->device_node);
> > +	}
> > +}
> > +
> >   static int __init at_dma_probe(struct platform_device *pdev)
> >   {
> >   	struct at_dma		*atdma;
> > @@ -2105,12 +2119,17 @@ static int __init at_dma_probe(struct platform_device *pdev)
> >   err_of_dma_controller_register:
> >   	dma_async_device_unregister(&atdma->dma_device);
> >   err_dma_async_device_register:
> > +	free_irq(platform_get_irq(pdev, 0), atdma);
> > +	at_dma_cleanup_channels(atdma);
> >   	dma_pool_destroy(atdma->memset_pool);
> > +	dma_pool_destroy(atdma->lli_pool);
> > +	goto err_clk;
> I forget the reason why need goto here. Can you call disable_irq() or
> disable hardware irq and call synchronize_irq() at free_irq() place. then
> goto can fallback to below clean up code
>
> Frank
  Thank you for the suggestion. I've updated to v6 following your advice:
  - Replaced free_irq() with disable_irq() to allow fallthrough without 
goto
  - free_irq() is now called only once at err_desc_pool_create label
  The new approach ensures IRQ handler cannot schedule tasklets before 
tasklet_kill(), then free_irq() handles the actual release.

   Please help to review again.
Thanks!

> >   err_memset_pool_create:
> >   	dma_pool_destroy(atdma->lli_pool);
> >   err_desc_pool_create:
> >   	free_irq(platform_get_irq(pdev, 0), atdma);
> >   err_irq:
> > +err_clk:
> >   	clk_disable_unprepare(atdma->clk);
> >   	return err;
> >   }
> > @@ -2118,23 +2137,17 @@ static int __init at_dma_probe(struct platform_device *pdev)
> >   static void at_dma_remove(struct platform_device *pdev)
> >   {
> >   	struct at_dma		*atdma = platform_get_drvdata(pdev);
> > -	struct dma_chan		*chan, *_chan;
> >
> >   	at_dma_off(atdma);
> >   	if (pdev->dev.of_node)
> >   		of_dma_controller_free(pdev->dev.of_node);
> >   	dma_async_device_unregister(&atdma->dma_device);
> >
> > -	dma_pool_destroy(atdma->memset_pool);
> > -	dma_pool_destroy(atdma->lli_pool);
> >   	free_irq(platform_get_irq(pdev, 0), atdma);
> >
> > -	list_for_each_entry_safe(chan, _chan, &atdma->dma_device.channels,
> > -			device_node) {
> > -		/* Disable interrupts */
> > -		atc_disable_chan_irq(atdma, chan->chan_id);
> > -		list_del(&chan->device_node);
> > -	}
> > +	at_dma_cleanup_channels(atdma);
> > +	dma_pool_destroy(atdma->memset_pool);
> > +	dma_pool_destroy(atdma->lli_pool);
> >
> >   	clk_disable_unprepare(atdma->clk);
> >   }
> > --
> > 2.25.1