Re: [PATCH v12 05/20] dmaengine: ti: k3-udma: move descriptor management to k3-udma-common.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] dmaengine: ti: k3-udma: mismatched devices in dma_alloc_coherent and dma_free_coherent
- [High] dmaengine: ti: buffer overflow due to swapped tr_size and tr_count in cppi5_trdesc_calc_size
- [High] dmaengine: ti: UAF and double free in udma_desc_free() during teardown
- [Low] dma: ti: k3-udma-common: missing <linux/module.h> inclusion
--

commit 7926401443e76f276939e13c216c32bd4d7e7504
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>

dmaengine: ti: k3-udma: move descriptor management to k3-udma-common.c

Refactor the K3 UDMA driver by moving all DMA descriptor handling
functions from k3-udma.c to a new common library, k3-udma-common.c.
This prepares the driver for supporting new K3 UDMA v2 variant.

> diff --git a/drivers/dma/ti/k3-udma-common.c b/drivers/dma/ti/k3-udma-common.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/dma/ti/k3-udma-common.c
> @@ -0,0 +1,1244 @@
[ ... ]
> +struct udma_desc *udma_udma_desc_from_paddr(struct udma_chan *uc,
> +					    dma_addr_t paddr)
> +{
> +	struct udma_desc *d = uc->terminated_desc;
> +
> +	if (d) {
> +		dma_addr_t desc_paddr = udma_curr_cppi5_desc_paddr(d,
> +								   d->desc_idx);

[Severity: High]
Could this result in a use-after-free? During channel teardown,
udma_reset_rings() is called locklessly from udma_synchronize() or
udma_free_chan_resources(), freeing uc->terminated_desc. If the DMA ring IRQ
fires concurrently, udma_ring_irq_handler() could access d->desc_idx here
after the descriptor has been freed.

> +
> +		if (desc_paddr != paddr)
> +			d = NULL;
> +	}
[ ... ]
> +static void udma_free_hwdesc(struct udma_chan *uc, struct udma_desc *d)
> +{
> +	if (uc->use_dma_pool) {
[ ... ]
> +	} else if (d->hwdesc[0].cppi5_desc_vaddr) {
> +		dma_free_coherent(uc->dma_dev, d->hwdesc[0].cppi5_desc_size,
> +				  d->hwdesc[0].cppi5_desc_vaddr,
> +				  d->hwdesc[0].cppi5_desc_paddr);

[Severity: High]
Is it possible to pass mismatched device pointers to the DMA mapping API here?
In udma_alloc_tr_desc(), the coherent memory is allocated using the parent
controller device uc->ud->dev. However, here it is freed using uc->dma_dev.
If the channel is configured for specific coherency, uc->dma_dev differs from
uc->ud->dev, which could cause issues during unmapping.

> +
> +		d->hwdesc[0].cppi5_desc_vaddr = NULL;
> +	}
> +}
[ ... ]
> +void udma_desc_free(struct virt_dma_desc *vd)
> +{
> +	struct udma_dev *ud = to_udma_dev(vd->tx.chan->device);
> +	struct udma_chan *uc = to_udma_chan(vd->tx.chan);
> +	struct udma_desc *d = to_udma_desc(&vd->tx);
> +	unsigned long flags;
> +
> +	if (uc->terminated_desc == d)
> +		uc->terminated_desc = NULL;

[Severity: High]
Might this lead to a double-free or list corruption? Since this check is
performed locklessly, both udma_reset_rings() and udma_ring_irq_handler()
could concurrently evaluate this to true and attempt to free the same
descriptor.

> +
> +	if (uc->use_dma_pool) {
> +		udma_free_hwdesc(uc, d);
[ ... ]
> +static struct udma_desc *udma_alloc_tr_desc(struct udma_chan *uc,
> +				     size_t tr_size, int tr_count,
> +				     enum dma_transfer_direction dir)
> +{
[ ... ]
> +	if (uc->use_dma_pool) {
> +		hwdesc->cppi5_desc_size = uc->config.hdesc_size;
> +		hwdesc->cppi5_desc_vaddr = dma_pool_zalloc(uc->hdesc_pool,
> +							   GFP_NOWAIT,
> +							   &hwdesc->cppi5_desc_paddr);
> +	} else {
> +		hwdesc->cppi5_desc_size = cppi5_trdesc_calc_size(tr_size,
> +								 tr_count);

[Severity: High]
Can this cause an out-of-bounds DMA access? The helper
cppi5_trdesc_calc_size() expects arguments in the order of tr_count, tr_size,
but they are swapped here. Because the sizing formula scales differently, this
results in a smaller allocated buffer whenever tr_count is greater than
tr_size.

> +		hwdesc->cppi5_desc_size = ALIGN(hwdesc->cppi5_desc_size,
> +						uc->ud->desc_align);
> +		hwdesc->cppi5_desc_vaddr = dma_alloc_coherent(uc->ud->dev,
> +							      hwdesc->cppi5_desc_size,
> +							      &hwdesc->cppi5_desc_paddr,
> +							      GFP_NOWAIT);
> +	}
[ ... ]
> +MODULE_DESCRIPTION("Texas Instruments K3 UDMA Common Library");
> +MODULE_LICENSE("GPL v2");

[Severity: Low]
This isn't a bug, but there seems to be a missing include for linux/module.h
which is required for module macros like MODULE_DESCRIPTION and
MODULE_LICENSE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007102936.2402427-1-s-adivi@ti.com?part=5