* Re: [PATCH] dmaengine: bestcomm: set bcom_eng to NULL on probe failure
2026-08-17 1:54 [PATCH] dmaengine: bestcomm: set bcom_eng to NULL on probe failure Rosen Penev
@ 2026-08-17 2:07 ` sashiko-bot
2026-08-17 13:55 ` Frank Li
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-08-17 2:07 UTC (permalink / raw)
To: Rosen Penev; +Cc: Frank.Li, dmaengine, vkoul
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [High] Mismatch between request_mem_region() and release_mem_region() sizes causes a permanent resource leak.
- [High] The global pointer `bcom_eng` is published before its fields are initialized, creating a race condition where concurrent API calls can use uninitialized state. The error path also has a Use-After-Free window.
- [High] Missing `IS_ERR` check for `rh_create()` return value causes a kernel panic on failure.
- [High] SRAM memory allocated for microcode descriptors is leaked when a task is freed.
- [Medium] The `stop` field used to track task allocation is cleared without holding the required lock, causing a data race and potential premature reallocation.
--
--- Patch [1]: [PATCH] dmaengine: bestcomm: set bcom_eng to NULL on probe failure ---
commit 92a145656e579b67978a7c55621b1c807ffdc1c7
Author: Rosen Penev <rosenp@gmail.com>
dmaengine: bestcomm: set bcom_eng to NULL on probe failure
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.
> diff --git a/drivers/dma/bestcomm/bestcomm.c b/drivers/dma/bestcomm/bestcomm.c
> --- a/drivers/dma/bestcomm/bestcomm.c
> +++ b/drivers/dma/bestcomm/bestcomm.c
[ ... ]
> @@ -445,6 +445,7 @@ static int mpc52xx_bcom_probe(struct platform_device *op)
> release_mem_region(res_bcom.start, sizeof(struct mpc52xx_sdma));
[Severity: High]
This is a pre-existing issue, but does this mismatch in size during release
cause a permanent resource leak?
The memory region is requested earlier in mpc52xx_bcom_probe() using the
device tree node size:
if (!request_mem_region(res_bcom.start, resource_size(&res_bcom),
DRIVER_NAME)) {
However, the error path above attempts to release the region using a fixed
compile-time size: sizeof(struct mpc52xx_sdma). Since __release_region()
strictly verifies the boundaries of the requested resource, if these sizes
differ, the check fails and the I/O memory region is never freed. Driver
unbinding in mpc52xx_bcom_remove() also uses this same fixed size.
> error_sramclean:
> kfree(bcom_eng);
> + bcom_eng = NULL;
[Severity: High]
This is a pre-existing issue, but by assigning bcom_eng early, do we expose
uninitialized state to concurrent accessors?
In mpc52xx_bcom_probe(), the global pointer is published immediately after
allocation:
bcom_eng = kzalloc_obj(struct bcom_engine);
if (!bcom_eng) {
Concurrent API calls like bcom_task_alloc() rely on the pointer not being
NULL as a safety check:
if (!bcom_eng)
return NULL;
spin_lock(&bcom_eng->lock);
Because the pointer is published before bcom_engine_init() sets up the lock
and structures, a concurrent client could dereference uninitialized fields.
While adding bcom_eng = NULL closes the use-after-free window during
cleanup, the early publication race still exists.
> bcom_sram_cleanup();
> error_ofput:
> of_node_put(op->dev.of_node);
[Severity: High]
This is a pre-existing issue, but is there a missing check for an error
pointer from rh_create() that could cause a kernel panic?
In drivers/dma/bestcomm/sram.c:bcom_sram_init(), the result of rh_create()
is directly used without validation:
/* Create an rheap (defaults to 32 bits word alignment) */
bcom_sram->rh = rh_create(4);
/* Attach the free zones */
regaddr_p = NULL;
psize = 0;
if (!regaddr_p || !psize) {
/* Attach the whole zone */
rh_attach_region(bcom_sram->rh, 0, bcom_sram->size);
If memory allocation fails under pressure, rh_create() returns
ERR_PTR(-ENOMEM). Passing this directly to rh_attach_region() will cause
a crash when it accesses internal fields.
[Severity: Medium]
This is a pre-existing issue, but does locklessly clearing the stop marker
create a data race and risk premature reallocation?
In drivers/dma/bestcomm/bestcomm.c:bcom_task_free(), the task marker state
is cleared without holding the required lock:
/* Clear TDT */
bcom_eng->tdt[tsk->tasknum].start = 0;
bcom_eng->tdt[tsk->tasknum].stop = 0;
The same happens on the allocation error path in bcom_task_alloc():
bcom_eng->tdt[tasknum].stop = 0;
Since bcom_task_alloc() enforces a lock to safely locate free tasks, if
stop is cleared locklessly before task teardown completes, a concurrent
allocator might identify the task as available and reallocate it while
resources are still being freed.
[Severity: High]
This is a pre-existing issue, but does freeing a task leak the dynamically
allocated SRAM used for microcode image descriptors?
When loading an image, drivers/dma/bestcomm/bestcomm.c:bcom_load_image()
dynamically allocates SRAM and tracks it via tdt->start:
desc = bcom_sram_alloc(hdr->desc_size * sizeof(u32), 4, &start_pa);
if (!desc)
return -ENOMEM;
tdt->start = start_pa;
However, when the task is torn down, bcom_task_free() simply discards this
physical address without freeing the block:
/* Clear TDT */
bcom_eng->tdt[tsk->tasknum].start = 0;
bcom_eng->tdt[tsk->tasknum].stop = 0;
Because SRAM is extremely limited on the MPC5200, repeatedly bringing a
network interface up and down could exhaust SRAM and break bestcomm
functionality. Should bcom_sram_free() be called here before zeroing the
marker?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260817015442.136038-1-rosenp@gmail.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] dmaengine: bestcomm: set bcom_eng to NULL on probe failure
2026-08-17 1:54 [PATCH] dmaengine: bestcomm: set bcom_eng to NULL on probe failure Rosen Penev
2026-08-17 2:07 ` sashiko-bot
@ 2026-08-17 13:55 ` Frank Li
1 sibling, 0 replies; 3+ messages in thread
From: Frank Li @ 2026-08-17 13:55 UTC (permalink / raw)
To: Rosen Penev
Cc: dmaengine, Vinod Koul, Frank Li, Paul Mackerras, Grant Likely,
open list
On Sun, Aug 16, 2026 at 06:54:42PM -0700, Rosen Penev wrote:
> 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.
>
> Fixes: 9560aea4e9d1 ("[POWERPC] mpc5200: Fix null dereference if bestcomm fails to initialize")
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
Not sure that still have power pc user.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> drivers/dma/bestcomm/bestcomm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/dma/bestcomm/bestcomm.c b/drivers/dma/bestcomm/bestcomm.c
> index 432b43520ddc..ec58d2c6573f 100644
> --- a/drivers/dma/bestcomm/bestcomm.c
> +++ b/drivers/dma/bestcomm/bestcomm.c
> @@ -445,6 +445,7 @@ static int mpc52xx_bcom_probe(struct platform_device *op)
> release_mem_region(res_bcom.start, sizeof(struct mpc52xx_sdma));
> error_sramclean:
> kfree(bcom_eng);
> + bcom_eng = NULL;
> bcom_sram_cleanup();
> error_ofput:
> of_node_put(op->dev.of_node);
> --
> 2.55.0
>
^ permalink raw reply [flat|nested] 3+ messages in thread