Re: [PATCH v7 09/19] dmaengine: ti: k3-udma: move resource management functions to k3-udma-common.c

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

Pre-existing issues:
- [Critical] Tasklet can dereference a freed cyclic descriptor due to missing `vchan_terminate_vdesc()`.
- [High] Missing bounds checks for resource IDs in `UDMA_RESERVE_RESOURCE` macro and `udma_get_tchan` before performing bitmap accesses.
- [High] Lack of synchronization in UDMA resource allocation functions leading to TOCTOU races and potential bitmap corruption.
- [High] Incorrect boundary check in `udma_get_chan_pair` leads to potential out-of-bounds bitmap writes.
--

--- Patch [9]: [PATCH v7 09/19] dmaengine: ti: k3-udma: move resource management functions to k3-udma-common.c ---
commit e7614ebd200c614b2a17303347bcfc13ecd5262d
Author: Sai Sree Kartheek Adivi <[email protected]>

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

Move functions responsible for allocation and release of udma
resources such as channels, rings and flows from k3-udma.c
to the common k3-udma-common.c file.

> diff --git a/drivers/dma/ti/k3-udma-common.c b/drivers/dma/ti/k3-udma-common.c
> --- a/drivers/dma/ti/k3-udma-common.c
> +++ b/drivers/dma/ti/k3-udma-common.c
> @@ -1891,5 +1891,447 @@ enum dmaengine_alignment udma_get_copy_align(struct udma_dev *ud)

[ ... ]

> +int __udma_alloc_gp_rflow_range(struct udma_dev *ud, int from, int cnt)
> +{

[ ... ]

> +	start = bitmap_find_next_zero_area(tmp,
> +					   ud->rflow_cnt,
> +					   tmp_from, cnt, 0);
> +	if (start >= ud->rflow_cnt)
> +		return -ENOMEM;
> +
> +	if (from >= 0 && start != from)
> +		return -EEXIST;
> +
> +	bitmap_set(ud->rflow_gp_map_allocated, start, cnt);

[Severity: High]
This is a pre-existing issue, but since these functions are exported for use
by other drivers (like k3-udma-glue.c), can they be called concurrently?

There is no locking (e.g., ud->lock) around the bitmap_find_next_zero_area()
search and the subsequent bitmap_set() operation. Does this expose a
time-of-check to time-of-use (TOCTOU) race condition that could result in
double allocations or bitmap corruption?

> +	return start;
> +}
> +EXPORT_SYMBOL_GPL(__udma_alloc_gp_rflow_range);
> +
> +int __udma_free_gp_rflow_range(struct udma_dev *ud, int from, int cnt)
> +{
> +	if (from < ud->rchan_cnt)
> +		return -EINVAL;
> +	if (from + cnt > ud->rflow_cnt)
> +		return -EINVAL;
> +
> +	bitmap_clear(ud->rflow_gp_map_allocated, from, cnt);

[Severity: High]
This is a pre-existing issue, but similarly to the allocation function, this
modifies the shared bitmap without synchronization. Could this cause data races
if multiple flows are freed concurrently?

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(__udma_free_gp_rflow_range);

[ ... ]

> +#define UDMA_RESERVE_RESOURCE(res)					\
> +struct udma_##res *__udma_reserve_##res(struct udma_dev *ud,	\
> +					       enum udma_tp_level tpl,	\
> +					       int id)			\
> +{									\
> +	if (id >= 0) {							\
> +		if (test_bit(id, ud->res##_map)) {			\

[Severity: High]
This is a pre-existing issue, but is it possible for a client driver or device
tree to provide an out-of-bounds id here?

The macro calls test_bit(id, ud->res##_map) and set_bit(id, ud->res##_map)
without verifying if id is strictly less than ud->res##_cnt. Could this lead
to out-of-bounds memory accesses?

> +			dev_err(ud->dev, "res##%d is in use\n", id);	\
> +			return ERR_PTR(-ENOENT);			\
> +		}							\

[ ... ]

> +int udma_get_tchan(struct udma_chan *uc)
> +{
> +	struct udma_dev *ud = uc->ud;
> +	int ret;

[ ... ]

> +	if (ud->tflow_cnt) {
> +		int tflow_id;
> +
> +		/* Only PKTDMA have support for tx flows */
> +		if (uc->config.default_flow_id >= 0)
> +			tflow_id = uc->config.default_flow_id;
> +		else
> +			tflow_id = uc->tchan->id;
> +
> +		if (test_bit(tflow_id, ud->tflow_map)) {

[Severity: High]
This is a pre-existing issue, but tflow_id can be controlled by
uc->config.default_flow_id and is used in bitmap operations without being
checked against ud->tflow_cnt.

Can this cause out-of-bounds reads/writes if a malformed default flow ID is
provided?

> +			dev_err(ud->dev, "tflow%d is in use\n", tflow_id);
> +			clear_bit(uc->tchan->id, ud->tchan_map);
> +			uc->tchan = NULL;
> +			return -ENOENT;
> +		}

[ ... ]

> +int udma_get_chan_pair(struct udma_chan *uc)
> +{
> +	struct udma_dev *ud = uc->ud;
> +	int chan_id, end;

[ ... ]

> +	/* Can be optimized, but let's have it like this for now */
> +	end = min(ud->tchan_cnt, ud->rchan_cnt);
> +	/*
> +	 * Try to use the highest TPL channel pair for MEM_TO_MEM channels
> +	 * Note: in UDMAP the channel TPL is symmetric between tchan and rchan
> +	 */
> +	chan_id = ud->tchan_tpl.start_idx[ud->tchan_tpl.levels - 1];
> +	for (; chan_id < end; chan_id++) {
> +		if (!test_bit(chan_id, ud->tchan_map) &&
> +		    !test_bit(chan_id, ud->rchan_map))
> +			break;
> +	}
> +
> +	if (chan_id == end)
> +		return -ENOENT;
> +
> +	set_bit(chan_id, ud->tchan_map);
> +	set_bit(chan_id, ud->rchan_map);

[Severity: High]
This is a pre-existing issue, but is it possible for the loop's initial
chan_id (from start_idx) to be strictly greater than end?

If ud->tchan_cnt > ud->rchan_cnt and start_idx > rchan_cnt, the loop condition
chan_id < end is immediately false. The exit check chan_id == end will also
fail, bypassing the -ENOENT return. Would this cause the set_bit() calls to
write out of bounds?

> +	uc->tchan = &ud->tchans[chan_id];
> +	uc->rchan = &ud->rchans[chan_id];

[ ... ]

> +void udma_free_chan_resources(struct dma_chan *chan)
> +{
> +	struct udma_chan *uc = to_udma_chan(chan);
> +	struct udma_dev *ud = to_udma_dev(chan->device);
> +
> +	udma_terminate_all(chan);
> +	if (uc->terminated_desc) {
> +		ud->reset_chan(uc, false);
> +		udma_reset_rings(uc);
> +	}
> +
> +	cancel_delayed_work_sync(&uc->tx_drain.work);
> +
> +	if (uc->irq_num_ring > 0) {
> +		free_irq(uc->irq_num_ring, uc);
> +
> +		uc->irq_num_ring = 0;
> +	}
> +	if (uc->irq_num_udma > 0) {
> +		free_irq(uc->irq_num_udma, uc);
> +
> +		uc->irq_num_udma = 0;
> +	}
> +
> +	/* Release PSI-L pairing */
> +	if (uc->psil_paired && ud->psil_unpair) {
> +		ud->psil_unpair(ud, uc->config.src_thread,
> +				  uc->config.dst_thread);
> +		uc->psil_paired = false;
> +	}
> +
> +	vchan_free_chan_resources(&uc->vc);
> +	tasklet_kill(&uc->vc.task);

[Severity: Critical]
This is a pre-existing issue, but does this sequence leave a window for a
use-after-free on cyclic descriptors?

udma_terminate_all() sets uc->terminated_desc = uc->desc without calling
vchan_terminate_vdesc(), which leaves vc->cyclic pointing to the descriptor.
udma_reset_rings() then frees uc->terminated_desc.

If a custom completion tasklet is scheduled and executes concurrently before
tasklet_kill() runs, won't it try to dereference the already freed vc->cyclic
memory?

> +
> +	bcdma_free_bchan_resources(uc);
> +	udma_free_tx_resources(uc);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9