Re: [PATCHv2] dmaengine: bestcomm: gen_bd: fix out-of-bounds access in PSC parameter lookup
From: sashiko-bot@kernel.org
Date: 2026-09-10 19:34:51
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: Pre-existing issues: - [High] Pre-existing memory and resource leak of BestComm tasks on error paths during DMA creation. - [High] Pre-existing memory leak of BestComm task microcode (desc) in SRAM on task release. -- commit 151ed4c77a710a40a6d4463f6c561df7caf80bcc Author: Rosen Penev [off-list ref] dmaengine: bestcomm: gen_bd: fix out-of-bounds access in PSC parameter lookup This commit fixes out-of-bounds accesses in bcom_psc_gen_bd_rx_init() and bcom_psc_gen_bd_tx_init() by using ARRAY_SIZE() for bounds checking. It also removes an unused forward declaration.
quoted hunk ↗ jump to hunk
diff --git a/drivers/dma/bestcomm/gen_bd.c b/drivers/dma/bestcomm/gen_bd.c index 8a24a5cbc2633..75d1a6f543df9 100644 --- a/drivers/dma/bestcomm/gen_bd.c +++ b/drivers/dma/bestcomm/gen_bd.c@@ -315,7 +315,7 @@ static struct bcom_psc_params { struct bcom_task * bcom_psc_gen_bd_rx_init(unsigned psc_num, int queue_len, phys_addr_t fifo, int maxbufsize) { - if (psc_num >= MPC52xx_PSC_MAXNUM) + if (psc_num >= ARRAY_SIZE(bcom_psc_params)) return NULL; return bcom_gen_bd_rx_init(queue_len, fifo,@@ -336,7 +336,9 @@ EXPORT_SYMBOL_GPL(bcom_psc_gen_bd_rx_init); struct bcom_task * bcom_psc_gen_bd_tx_init(unsigned psc_num, int queue_len, phys_addr_t fifo) { - struct psc; + if (psc_num >= ARRAY_SIZE(bcom_psc_params)) + return NULL; + return bcom_gen_bd_tx_init(queue_len, fifo, bcom_psc_params[psc_num].tx_initiator, bcom_psc_params[psc_num].tx_ipr);
[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a
pre-existing memory and resource leak of BestComm tasks on error paths
during DMA creation.
If mpc5200_audio_dma_create() fails during devm_request_irq() or
devm_snd_soc_register_component(), it returns an error without freeing the
successfully allocated bcom_task structures:
sound/soc/fsl/mpc5200_dma.c:mpc5200_audio_dma_create() {
if (!psc_dma->capture.bcom_task ||
!psc_dma->playback.bcom_task) {
dev_err(&op->dev, "Could not allocate bestcomm tasks\n");
return -ENODEV;
}
/* ... */
if (rc)
return -ENODEV;
/* ... */
return devm_snd_soc_register_component(&op->dev,
&mpc5200_audio_dma_component, NULL, 0);
}
Additionally, if snd_soc_set_ac97_ops() fails after mpc5200_audio_dma_create()
succeeds, it returns without calling mpc5200_audio_dma_destroy(), leaking the
tasks:
sound/soc/fsl/mpc5200_psc_ac97.c:psc_ac97_of_probe() {
/* ... */
rc = snd_soc_set_ac97_ops(&psc_ac97_ops);
if (rc != 0) {
dev_err(&op->dev, "Failed to set AC'97 ops: %d\n", rc);
return rc;
}
/* ... */
}
Repeated deferred probes or driver bind/unbind cycles could exhaust the 16
BestComm task slots, preventing further DMA operations. Are we missing cleanup
steps on these error paths to ensure the allocated tasks are freed?
[Severity: High]
This is a pre-existing issue, but there seems to be a memory leak of BestComm
task microcode in SRAM on task release.
When bcom_load_image() allocates a chunk of SRAM for task microcode, it stores
its physical address in tdt->start:
drivers/dma/bestcomm/bestcomm.c:bcom_load_image() {
/* ... */
desc = bcom_sram_alloc(hdr->desc_size * sizeof(u32), 4, &start_pa);
/* ... */
}
When the task is freed, the physical address is zeroed out without explicitly
freeing the underlying SRAM chunk:
drivers/dma/bestcomm/bestcomm.c:bcom_task_free() {
/* ... */
/* Clear TDT */
bcom_eng->tdt[tsk->tasknum].start = 0;
bcom_eng->tdt[tsk->tasknum].stop = 0;
/* ... */
}
Since the SRAM chunk is not freed using bcom_sram_free(), repeated unbinding
and binding of BestComm drivers could eventually exhaust all available on-chip
SRAM. Does bcom_task_free() need to free the SRAM before clearing tdt->start?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910192404.21809-1-rosenp@gmail.com?part=1