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 <[email protected]>
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/[email protected]?part=5
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.