Re: [PATCH v7 10/19] dmaengine: ti: k3-udma: refactor resource setup functions

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

New issues:
- [Medium] Bitmaps allocated with `devm_kmalloc_array` remain uninitialized if the optional SCI resource setup functions are not provided.
- [Medium] Missing NULL pointer check before invoking the `ud->udma_setup_resources` callback.
- [Low] Exporting an overly generic symbol name `setup_resources` pollutes the global kernel namespace, and internal functions lack static scope.

Pre-existing issues:
- [High] Missing bounds check on hardware/firmware-provided resource ranges leads to a potential out-of-bounds write in `udma_mark_resource_ranges`.
--

--- Patch [10]: [PATCH v7 10/19] dmaengine: ti: k3-udma: refactor resource setup functions ---
commit 3e6d342dd828c46abfda9610c9254ebd7a374ce5
Author: Sai Sree Kartheek Adivi <[email protected]>

dmaengine: ti: k3-udma: refactor resource setup functions

This commit refactors the resource setup functions for K3 UDMA variants by
moving shared logic to a common file. It splits SCI resource setup into
variant-specific functions and uses function pointers to retain existing
functionality.

> diff --git a/drivers/dma/ti/k3-udma-common.c b/drivers/dma/ti/k3-udma-common.c
> index 472903e7d1a9f..8b2a3be3fa932 100644
> --- a/drivers/dma/ti/k3-udma-common.c
> +++ b/drivers/dma/ti/k3-udma-common.c

[ ... ]

> +int bcdma_setup_resources(struct udma_dev *ud)
> +{
> +	int ret;
> +	struct device *dev = ud->dev;
> +	u32 cap;

[ ... ]

> +	ud->bchan_map = devm_kmalloc_array(dev, BITS_TO_LONGS(ud->bchan_cnt),
> +					   sizeof(unsigned long), GFP_KERNEL);

[Severity: Medium]
Because devm_kmalloc_array() does not zero the allocated memory, will
ud->bchan_map remain uninitialized if the optional
bcdma_setup_sci_resources() callback isn't set or fails to populate it? 

Later in setup_resources():

    if (ud->bchan_cnt)
        ch_count -= bitmap_weight(ud->bchan_map, ud->bchan_cnt);
        
Could this call bitmap_weight() on uninitialized memory, leading to
corrupted channel counts? Using devm_bitmap_zalloc() might ensure the
bitmaps are safely initialized.

> +	ud->bchans = devm_kcalloc(dev, ud->bchan_cnt, sizeof(*ud->bchans),
> +				  GFP_KERNEL);

[ ... ]

> +int setup_resources(struct udma_dev *ud)
> +{
> +	struct device *dev = ud->dev;
> +	int ch_count, ret;
> +
> +	switch (ud->match_data->type) {
> +	case DMA_TYPE_UDMA:
> +		ret = ud->udma_setup_resources(ud);

[Severity: Medium]
Should there be a NULL pointer check here before invoking 
ud->udma_setup_resources(ud)? While the current code might populate it 
unconditionally, other variant callbacks in this file are guarded with
NULL checks before invocation.

> +		break;

[ ... ]

> +EXPORT_SYMBOL_GPL(setup_resources);

[Severity: Low]
Does exporting setup_resources with such a generic name risk polluting the 
global kernel namespace? Renaming it to something like 
k3_udma_setup_resources might avoid future linking collisions.

> diff --git a/drivers/dma/ti/k3-udma.h b/drivers/dma/ti/k3-udma.h
> index 329df7eb156b2..5e222ce366faf 100644
> --- a/drivers/dma/ti/k3-udma.h
> +++ b/drivers/dma/ti/k3-udma.h

[ ... ]

> @@ -679,6 +682,11 @@ struct udma_bchan *__udma_reserve_bchan(struct udma_dev *ud, enum udma_tp_level
>  struct udma_tchan *__udma_reserve_tchan(struct udma_dev *ud, enum udma_tp_level tpl, int id);
>  struct udma_rchan *__udma_reserve_rchan(struct udma_dev *ud, enum udma_tp_level tpl, int id);
>  
> +int udma_setup_resources(struct udma_dev *ud);
> +int bcdma_setup_resources(struct udma_dev *ud);
> +int pktdma_setup_resources(struct udma_dev *ud);
> +int setup_resources(struct udma_dev *ud);

[Severity: Low]
Since bcdma_setup_resources() and pktdma_setup_resources() are only called 
internally within k3-udma-common.c, could they be declared static in the C 
file instead of being exposed globally in this header?

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds write 
in udma_mark_resource_ranges() in k3-udma-common.c:

void udma_mark_resource_ranges(struct udma_dev *ud, unsigned long *map,
                               struct ti_sci_resource_desc *rm_desc,
                               char *name)
{
    bitmap_clear(map, rm_desc->start, rm_desc->num);
    bitmap_clear(map, rm_desc->start_sec, rm_desc->num_sec);
    ...
}

Is it possible for firmware or device tree to provide a resource range where 
rm_desc->start + rm_desc->num exceeds the allocated bitmap size? If this 
happens, it seems it would silently corrupt adjacent memory on the heap. 
Would it make sense to add bounds checking against the channel count before 
clearing the bits?

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