Re: [PATCH] dmaengine: bestcomm: ioremap and get irq early

[sashiko-bot@kernel.org]

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

New issues:
- [High] bcom_task_alloc() returns an integer error code implicitly cast to a pointer, which bypasses caller NULL checks and causes a kernel panic.
- [High] bcom_task_alloc() permanently leaks a reserved task index if platform_get_irq() fails.

Pre-existing issues:
- [High] Probe error path frees the global `bcom_eng` pointer without setting it to NULL, leaving a dangling pointer.
--

--- Patch [1]: [PATCH] dmaengine: bestcomm: ioremap and get irq early ---
commit f02c723e90824bdf0feee229bfc42b04e0be6dc0
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: bestcomm: ioremap and get irq early

This patch refactors the MPC52xx BestComm driver to use managed resources for
memory mapping and to acquire the IRQ earlier. By doing so, it allows handling
potential probe deferrals before allocating resources, simplifying the cleanup
paths.

> diff --git a/drivers/dma/bestcomm/bestcomm.c b/drivers/dma/bestcomm/bestcomm.c
> index 432b43520ddc9..55ad7bc34df78 100644
> --- a/drivers/dma/bestcomm/bestcomm.c
> +++ b/drivers/dma/bestcomm/bestcomm.c
[ ... ]
> @@ -68,6 +69,10 @@ bcom_task_alloc(int bd_count, int bd_size, int priv_size)
>  	if (tasknum < 0)
>  		return NULL;
>  
> +	irq = platform_get_irq(bcom_eng->pdev, tasknum);
> +	if (irq < 0)
> +		return irq;

[Severity: High]
Does returning the integer irq here implicitly cast the negative error code
into a pointer? If platform_get_irq() returns -EPROBE_DEFER, callers such
as bcom_fec_rx_init() that check the return value with if (!tsk) will
assume success and dereference the invalid pointer.

[Severity: High]
Does this early return also permanently leak the reserved task index?
The task index was reserved earlier in bcom_task_alloc() by setting
bcom_eng->tdt[i].stop = 0xfffffffful;.

By returning directly instead of using the error: label, the cleanup code
that clears the reservation (bcom_eng->tdt[tasknum].stop = 0;) is bypassed.
Since the total pool size is strictly limited to 16 tasks, repeated probe
deferrals will rapidly exhaust the pool.

> +
>  	/* Allocate our structure */
>  	tsk = kzalloc(sizeof(struct bcom_task) + priv_size, GFP_KERNEL);
[ ... ]
> @@ -439,15 +416,9 @@ static int mpc52xx_bcom_probe(struct platform_device *op)
>  	return 0;
>  
>  	/* Error path */
> -error_unmap:
> -	iounmap(bcom_eng->regs);
> -error_release:
> -	release_mem_region(res_bcom.start, sizeof(struct mpc52xx_sdma));
>  error_sramclean:
>  	kfree(bcom_eng);

[Severity: High]
This is a pre-existing issue, but does freeing the globally accessible
bcom_eng pointer here without setting it to NULL leave a dangling pointer?

Functions like bcom_task_alloc() use if (!bcom_eng) as a safety readiness
check. If the probe fails and hits this error path, subsequent or concurrent
API calls will bypass the check and dereference the freed memory.

>  	bcom_sram_cleanup();
> -error_ofput:
> -	of_node_put(op->dev.of_node);
>  
>  	printk(KERN_ERR "DMA: MPC52xx BestComm init failed !\n");
>

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