Re: [PATCH] dmaengine: loongson2-apb-cmc: fix descriptor leak in prep_slave_sg()

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] DMA engine prep functions return ERR_PTR instead of NULL on failure, violating API contract.
- [High] Potential division by zero in `loongson2_cmc_dma_prep_dma_cyclic`.
- [High] Unaligned transfer lengths rounded up using `DIV_ROUND_UP`, causing memory corruption.
- [High] NULL pointer dereference when calculating residue for pending descriptors.
- [High] Multi-period cyclic transfers hang because CNDTR is not reloaded.
- [High] Interrupt handler returns IRQ_HANDLED unconditionally on shared IRQ line.
--

commit e41b57bccdb990f1d1319016c8e4a1cb0f109aaf
Author: Diego Fernando Mancera Gomez [off-list ref]

    dmaengine: loongson2-apb-cmc: fix descriptor leak in prep_slave_sg()
    
    loongson2_cmc_dma_prep_slave_sg() allocates the descriptor with
    kzalloc_flex() before iterating over the scatterlist. If
    loongson2_cmc_dma_set_xfer_param() fails inside the loop, the function
    returns ERR_PTR(ret) without freeing the descriptor. At this point the
    descriptor has not been handed to vchan_tx_prep() yet, so nothing else
    will ever free it, leaking it on every failed ->device_prep_slave_sg()
    call (e.g. an unsupported transfer direction or bus width).
    
    The neighbouring error path for the num_items check already frees the
    descriptor before returning; do the same on the set_xfer_param() failure
    path.

    Link: https://lore.kernel.org/all/20260818094957.7D2221F000E9@smtp.kernel.org/ (local)

[Severity: High]
This isn't a bug introduced by this patch, but does returning ERR_PTR()
instead of NULL violate the DMA engine API contract?

If memory allocation fails or invalid parameters are provided,
device_prep_slave_sg() returns ERR_PTR(...). Client drivers expect NULL on
failure and use if (!desc) to check for errors. Since ERR_PTR evaluates to
true, could clients pass the invalid pointer to dmaengine_submit() and cause a
kernel panic?

+		}
 
 		num_items = DIV_ROUND_UP(sg_dma_len(sg), buswidth);

[Severity: High]
This is a pre-existing issue, but can this unaligned transfer length calculation
corrupt memory?

The driver calculates hardware transfer count using
DIV_ROUND_UP(length, buswidth). If a client requests a transfer length not
aligned to the bus width (e.g., 3 bytes on a 4-byte bus), DIV_ROUND_UP rounds
the count up to 1 item. The hardware will then transfer 4 bytes. Could this
cause the DMA controller to write past the end of the client's allocated
buffer?

 		if (num_items >= LOONSON2_CMCDMA_MAX_DATA_ITEMS) {
 			dev_err(chan2dev(lchan), "Number of items not supported\n");
 			kfree(desc);
 			return ERR_PTR(-EINVAL);
 		}

While reviewing this code, I noticed a few other pre-existing issues in this
file:

In loongson2_cmc_dma_prep_dma_cyclic():

	if (unlikely(buf_len % period_len))

[Severity: High]
This is a pre-existing issue, but can this modulo operation cause a
divide-by-zero crash?

The function accepts period_len from external callers and performs
buf_len % period_len without validating that period_len is non-zero. Could
this be reached via ALSA or other cyclic DMA clients if misconfigured to pass
a 0 period length?

[ ... ]

	ret = loongson2_cmc_dma_set_xfer_param(lchan, direction, &buswidth, period_len);
	if (ret)
		return ERR_PTR(ret);

[Severity: High]
This is a pre-existing issue, but does returning ERR_PTR() here also violate
the API contract?

Similar to loongson2_cmc_dma_prep_slave_sg(), this returns ERR_PTR instead of
NULL on failure, which could cause a client panic.

[ ... ]

	num_items = DIV_ROUND_UP(period_len, buswidth);

[Severity: High]
This is a pre-existing issue, but can this also cause an out-of-bounds DMA
transfer for cyclic transfers?

Like with loongson2_cmc_dma_prep_slave_sg(), rounding up unaligned transfer
lengths could write past the end of the destination buffer.


In loongson2_cmc_dma_desc_residue():

	if (lchan->desc->cyclic && next_sg == 0)

[Severity: High]
This is a pre-existing issue, but can this cause a NULL pointer dereference?

If a client driver queries the transfer status using dmaengine_tx_status()
after submitting a descriptor but before it is issued (or while the channel is
idle), lchan->desc will be NULL. The code unconditionally dereferences
lchan->desc->cyclic.


In loongson2_cmc_dma_configure_next_sg():

	/* Start transition */
	ccr |= LOONGSON2_CMCDMA_CCR_EN;
	loongson2_cmc_dma_write(lddev, LOONGSON2_CMCDMA_CCR, id, ccr);

[Severity: High]
This is a pre-existing issue, but will multi-period cyclic transfers hang
because CNDTR is not reloaded?

For multi-period cyclic transfers, hardware circular mode is not enabled. At
the end of each period, the driver reconfigures the next scatterlist element,
but fails to write the transfer count to the CNDTR register. Since CNDTR is 0
from the completed period, will re-enabling the channel result in an immediate
stall?


In loongson2_cmc_dma_chan_irq():

	return IRQ_HANDLED;
}

[Severity: High]
This is a pre-existing issue, but does unconditionally claiming the interrupt
break shared IRQ semantics?

The interrupt is requested with IRQF_SHARED. If another device on the same
line triggers an interrupt, status will be 0. The handler skips processing but
still returns IRQ_HANDLED instead of IRQ_NONE. Could this defeat the kernel's
spurious interrupt detection layer and prevent it from disabling a faulty IRQ
line?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905062833.272484-1-diegomancera.dev@gmail.com?part=1