dmaengine: bestcomm: gen_bd: fix out-of-bounds access in PSC parameter lookup

PATCH Superseded Rosen Penev 3 participants 3 messages Sep 10 – Sep 11 dmaengine

Share & tools

Rosen Penev

author

2026-09-10 19:24 UTC #0 lore raw

PATCH v2 1/1 dmaengine: bestcomm: gen_bd: fix out-of-bounds access in PSC parameter lookup

The bcom_psc_params[] array has 6 entries (indices 0-5), but bcom_psc_gen_bd_rx_init() checked against MPC52xx_PSC_MAXNUM which can be 12 when CONFIG_PPC_MPC512x is set, allowing indices 6-11 to pass and read past the array. The tx init function had no bounds check at all.

A malformed device tree with a large cell-index could therefore trigger an out-of-bounds read. The garbage initiator and ipr values would then be used for MMIO writes via out_8(&bcom_eng->regs->ipr[...], ...), potentially causing out-of-bounds MMIO accesses.

Fix both functions to use ARRAY_SIZE(bcom_psc_params) for the bounds check.

Also remove a stray unused 'struct psc;' forward declaration in bcom_psc_gen_bd_tx_init.

Assisted-by Opencode:big-pickle

Signed-off-by Rosen Penev <[email protected]>

---
v2: fix title

 drivers/dma/bestcomm/gen_bd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/dma/bestcomm/gen_bd.c +4 -2

index 61b5746e1a97..00dfdc4b8eba 100644
@@ -321,7 +321,7 @@ static const struct bcom_psc_params 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,

@@ -342,7 +342,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);

Signature · 1 line

2.55.0

[email protected]

AI review

2026-09-10 19:34 UTC #1 lore raw

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 <[email protected]>

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 14 lines

drivers/dma/bestcomm/gen_bd.c +1 -1

index 8a24a5cbc2633..75d1a6f543df9 100644
@@ -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,

drivers/dma/bestcomm/gen_bd.c EXPORT_SYMBOL_GPL(bcom_psc_gen_bd_rx_init); · line 344

Quoted 10 lines

drivers/dma/bestcomm/gen_bd.c +3 -1

@@ -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?

Signature · 1 line

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

Frank Li

2026-09-11 16:08 UTC #2 lore raw

In reply to Rosen Penev

Quoted 11 lines · Rosen Penev

The bcom_psc_params[] array has 6 entries (indices 0-5), but bcom_psc_gen_bd_rx_init() checked against MPC52xx_PSC_MAXNUM which can be 12 when CONFIG_PPC_MPC512x is set, allowing indices 6-11 to pass and read past the array. The tx init function had no bounds check at all.
A malformed device tree with a large cell-index could therefore trigger an out-of-bounds read. The garbage initiator and ipr values would then be used for MMIO writes via out_8(&bcom_eng->regs->ipr[...], ...), potentially causing out-of-bounds MMIO accesses.

Quoted 5 lines

Fix both functions to use ARRAY_SIZE(bcom_psc_params) for the bounds check.
Also remove a stray unused 'struct psc;' forward declaration in bcom_psc_gen_bd_tx_init.

use two patches to fix it.

Frank

Quoted 35 lines

Assisted-by Opencode:big-pickle

Signed-off-by Rosen Penev <[email protected]>

---
v2: fix title

 drivers/dma/bestcomm/gen_bd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/dma/bestcomm/gen_bd.c +4 -3

index 61b5746e1a97..00dfdc4b8eba 100644
@@ -321,7 +321,7 @@ static const struct bcom_psc_params 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,

@@ -342,7 +342,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);
--

2.55.0