Re: [PATCH v7 05/19] 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 3 potential issue(s) to consider:

New issues:
- [Low] Global namespace pollution due to missing `static` keyword on `metadata_ops`.

Pre-existing issues:
- [Critical] Signed 32-bit integer overflow during multi-TR DMA address calculation causes massive negative offset.
- [High] Buffer underallocation and out-of-bounds write due to swapped arguments in `cppi5_trdesc_calc_size`.
--

--- Patch [5]: [PATCH v7 05/19] dmaengine: ti: k3-udma: move descriptor management to k3-udma-common.c ---
commit bdc5f8902724dd8a4b2dc695e0b04524fbd51779
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
(used in AM62L) that can reuse the same descriptor handling logic.

> diff --git a/drivers/dma/ti/k3-udma-common.c b/drivers/dma/ti/k3-udma-common.c
> new file mode 100644
> index 0000000000000..daf5f19dfb3ab
> --- /dev/null
> +++ b/drivers/dma/ti/k3-udma-common.c
> @@ -0,0 +1,1243 @@
[ ... ]
> +
> +const struct dma_descriptor_metadata_ops metadata_ops = {
> +	.attach = udma_attach_metadata,
> +	.get_ptr = udma_get_metadata_ptr,
> +	.set_len = udma_set_metadata_len,
> +};

[Severity: Low]
Does this struct need to be exported to the global namespace? The static
keyword seems to have been dropped during the code move. Leaving such a
generic name in the global namespace might cause linker collisions with other
subsystems.

> +
> +struct udma_desc *udma_udma_desc_from_paddr(struct udma_chan *uc,
[ ... ]
> +struct udma_desc *udma_alloc_tr_desc(struct udma_chan *uc,
> +				     size_t tr_size, int tr_count,
> +				     enum dma_transfer_direction dir)
> +{
[ ... ]
> +	/* Allocate memory for DMA ring descriptor */
> +	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]
This is a pre-existing issue, but does this call to cppi5_trdesc_calc_size()
pass the arguments in the correct order? The function expects tr_count first
and tr_size second.

Passing them in reverse order calculates a smaller size than needed when
the scatterlist has a large number of entries (e.g., tr_count > 64). As a
result, when the hardware writes the 4-byte responses, does it write past the
allocated buffer size and corrupt adjacent coherent DMA memory?

> +		hwdesc->cppi5_desc_size = ALIGN(hwdesc->cppi5_desc_size,
> +						uc->ud->desc_align);
> +		hwdesc->cppi5_desc_vaddr = dma_alloc_coherent(uc->ud->dev,
[ ... ]
> +struct udma_desc *
> +udma_prep_slave_sg_tr(struct udma_chan *uc, struct scatterlist *sgl,
> +		      unsigned int sglen, enum dma_transfer_direction dir,
> +		      unsigned long tx_flags, void *context)
> +{
[ ... ]
> +		if (num_tr == 2) {
> +			cppi5_tr_init(&tr_req[tr_idx].flags, CPPI5_TR_TYPE1,
> +				      false, false,
> +				      CPPI5_TR_EVENT_SIZE_COMPLETION, 0);
> +			cppi5_tr_csf_set(&tr_req[tr_idx].flags,
> +					 CPPI5_TR_CSF_SUPR_EVT);
> +
> +			tr_req[tr_idx].addr = sg_addr + tr0_cnt1 * tr0_cnt0;

[Severity: Critical]
This is a pre-existing issue, but can this address calculation overflow and
result in a massive negative offset?

When processing a DMA buffer larger than 2GB, the variables tr0_cnt0 and
tr0_cnt1 (both u16) can have a product that exceeds 2^31. Multiplying two u16
values in C promotes them to a signed 32-bit int, so the product can become
negative.

When this negative 32-bit int is added to sg_addr (a 64-bit dma_addr_t), it
is sign-extended to a massive 64-bit negative offset, calculating a backward
DMA address. Could this bypass memory isolation and corrupt arbitrary kernel
memory?

> +			tr_req[tr_idx].icnt0 = tr1_cnt0;
> +			tr_req[tr_idx].icnt1 = 1;
> +			tr_req[tr_idx].dim1 = tr1_cnt0;
> +			tr_idx++;
> +		}
> +
> +		d->residue += sg_dma_len(sgent);
> +	}
[ ... ]
> +struct udma_desc *
> +udma_prep_dma_cyclic_tr(struct udma_chan *uc, dma_addr_t buf_addr,
> +			size_t buf_len, size_t period_len,
> +			enum dma_transfer_direction dir, unsigned long flags)
> +{
[ ... ]
> +		if (num_tr == 2) {
> +			cppi5_tr_csf_set(&tr_req[tr_idx].flags,
> +					 CPPI5_TR_CSF_SUPR_EVT);
> +			tr_idx++;
> +
> +			cppi5_tr_init(&tr_req[tr_idx].flags, CPPI5_TR_TYPE1,
> +				      false, false,
> +				      CPPI5_TR_EVENT_SIZE_COMPLETION, 0);
> +
> +			tr_req[tr_idx].addr = period_addr + tr0_cnt1 * tr0_cnt0;

[Severity: Critical]
This is a pre-existing issue, but can this run into the same signed 32-bit
integer overflow regression during the cyclic descriptor preparation path?

> +			tr_req[tr_idx].icnt0 = tr1_cnt0;
> +			tr_req[tr_idx].icnt1 = 1;
> +			tr_req[tr_idx].dim1 = tr1_cnt0;
> +		}
[ ... ]
> +struct dma_async_tx_descriptor *
> +udma_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
> +		     size_t len, unsigned long tx_flags)
> +{
[ ... ]
> +	if (num_tr == 2) {
> +		cppi5_tr_init(&tr_req[1].flags, CPPI5_TR_TYPE15, false, true,
> +			      CPPI5_TR_EVENT_SIZE_COMPLETION, 0);
> +		cppi5_tr_csf_set(&tr_req[1].flags, csf);
> +
> +		tr_req[1].addr = src + tr0_cnt1 * tr0_cnt0;
> +		tr_req[1].icnt0 = tr1_cnt0;
> +		tr_req[1].icnt1 = 1;
> +		tr_req[1].icnt2 = 1;
> +		tr_req[1].icnt3 = 1;
> +
> +		tr_req[1].daddr = dest + tr0_cnt1 * tr0_cnt0;

[Severity: Critical]
This is a pre-existing issue, but does the same vulnerable 32-bit integer
promotion and overflow occur here when calculating both the source and
destination addresses for the second TR?

> +		tr_req[1].dicnt0 = tr1_cnt0;
> +		tr_req[1].dicnt1 = 1;
> +		tr_req[1].dicnt2 = 1;
> +		tr_req[1].dicnt3 = 1;
> +	}
> +
> +	cppi5_tr_csf_set(&tr_req[num_tr - 1].flags, csf | CPPI5_TR_CSF_EOP);

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