Re: [PATCH v8 19/19] dmaengine: ti: k3-udma: Validate resource ID and fix logging in reservation

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

Pre-existing issues:
- [High] The K3 UDMA resource allocation APIs perform unprotected, non-atomic test-and-set sequences on resource bitmaps, leading to concurrent allocations of the same hardware DMA channel.
- [High] Missing bounds check on `tflow_id` when allocating a transmit flow in `udma_get_tchan()`, leading to potential out-of-bounds bitmap access and incorrect error return.
--

--- Patch [19]: [PATCH v8 19/19] dmaengine: ti: k3-udma: Validate resource ID and fix logging in reservation ---
commit 491caa42a0fc50724787a5febd8ecf1dd0d46ac0
Author: Sai Sree Kartheek Adivi <[email protected]>
dmaengine: ti: k3-udma: Validate resource ID and fix logging in reservation

The __udma_reserve_##res macro currently lacks a bounds check for
the provided id. If a caller passes an ID exceeding the resource
count (ud->res##_cnt), test_bit() performs an out-of-bounds
memory access on the bitmap.

Additionally, the macro returns -ENOENT when a resource is already
in use, which is semantically incorrect.

Update the macro to validate the id, return -EBUSY instead of -ENOENT
for reserved resources, and properly print the resource name.

> diff --git a/drivers/dma/ti/k3-udma-common.c b/drivers/dma/ti/k3-udma-common.c
> index d5d7158a37f8f..381e1a30c0be7 100644
> --- a/drivers/dma/ti/k3-udma-common.c
> +++ b/drivers/dma/ti/k3-udma-common.c
> @@ -2016,9 +2016,14 @@ struct udma_##res *__udma_reserve_##res(struct udma_dev *ud,	\
>  					       int id)			\
>  {									\
>  	if (id >= 0) {							\
> +		if (id >= ud->res##_cnt) {				\
> +			dev_err(ud->dev,				\
> +				#res " id %d is out of bounds.\n", id);	\
> +			return ERR_PTR(-EINVAL);			\
> +		}							\
>  		if (test_bit(id, ud->res##_map)) {			\
> -			dev_err(ud->dev, "res##%d is in use\n", id);	\
> -			return ERR_PTR(-ENOENT);			\
> +			dev_err(ud->dev, #res "%d is in use\n", id);	\
> +			return ERR_PTR(-EBUSY);			\
>  		}							\

[Severity: High]
This is a pre-existing issue, but does this unprotected non-atomic
test-and-set sequence on the resource bitmap allow concurrent allocations of
the same hardware DMA channel?

If multiple client drivers request resources during parallel asynchronous
probe without external synchronization:

Driver 1 checks test_bit():
	if (test_bit(id, ud->res##_map)) { ... }

Driver 2 checks test_bit() concurrently:
	if (test_bit(id, ud->res##_map)) { ... }

Both drivers then proceed to set the same bit at the end of the macro:
	set_bit(id, ud->res##_map);

Could they both receive the same DMA channel?

This same unprotected pattern appears in __udma_get_rflow() and
__udma_alloc_gp_rflow_range().

[Severity: High]
This isn't a bug introduced by this patch, but while adding bounds checks
here, should we also add them to udma_get_tchan() in the same file?

In udma_get_tchan(), tflow_id is used in test_bit() and set_bit() on
ud->tflow_map without being validated against ud->tflow_cnt:

	if (test_bit(tflow_id, ud->tflow_map)) {
		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;
	}

	uc->tchan->tflow_id = tflow_id;
	set_bit(tflow_id, ud->tflow_map);

Can this write beyond the bounds of the bitmap if an invalid default_flow_id
is supplied from the Device Tree, or if tchan_cnt > tflow_cnt?

Additionally, it looks like this function still returns -ENOENT instead of
-EBUSY when a resource is already in use.

>  	} else {							\
>  		int start;						\

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