[PATCHv2] dmaengine: bestcomm: make gen_bd init helpers static

[PATCHv2] dmaengine: bestcomm: make gen_bd init helpers static

[Rosen Penev]

bcom_gen_bd_rx_init() and bcom_gen_bd_tx_init() are only called from
the PSC wrapper functions in gen_bd.c, so make them static, drop
their now-unneeded exports, and remove the corresponding header
declarations.

These had use before commit 7d3ee229ea51
("powerpc: Drop MPC5200 LocalPlus bus FIFO driver")

Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: mention driver removal that made them unused.
 drivers/dma/bestcomm/gen_bd.c       | 6 ++----
 include/linux/fsl/bestcomm/gen_bd.h | 8 --------
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/bestcomm/gen_bd.c b/drivers/dma/bestcomm/gen_bd.c
index 61b5746e1a97..b5093f7445cb 100644
--- a/drivers/dma/bestcomm/gen_bd.c
+++ b/drivers/dma/bestcomm/gen_bd.c
@@ -81,7 +81,7 @@ struct bcom_gen_bd_priv {
 /* Task support code                                                        */
 /* ======================================================================== */
 
-struct bcom_task *
+static struct bcom_task *
 bcom_gen_bd_rx_init(int queue_len, phys_addr_t fifo,
 			int initiator, int ipr, int maxbufsize)
 {
@@ -108,7 +108,6 @@ bcom_gen_bd_rx_init(int queue_len, phys_addr_t fifo,
 
 	return tsk;
 }
-EXPORT_SYMBOL_GPL(bcom_gen_bd_rx_init);
 
 int
 bcom_gen_bd_rx_reset(struct bcom_task *tsk)
@@ -166,7 +165,7 @@ bcom_gen_bd_rx_release(struct bcom_task *tsk)
 EXPORT_SYMBOL_GPL(bcom_gen_bd_rx_release);
 
 
-extern struct bcom_task *
+static struct bcom_task *
 bcom_gen_bd_tx_init(int queue_len, phys_addr_t fifo,
 			int initiator, int ipr)
 {
@@ -192,7 +191,6 @@ bcom_gen_bd_tx_init(int queue_len, phys_addr_t fifo,
 
 	return tsk;
 }
-EXPORT_SYMBOL_GPL(bcom_gen_bd_tx_init);
 
 int
 bcom_gen_bd_tx_reset(struct bcom_task *tsk)
diff --git a/include/linux/fsl/bestcomm/gen_bd.h b/include/linux/fsl/bestcomm/gen_bd.h
index aeb312a1cd00..7253d308d77a 100644
--- a/include/linux/fsl/bestcomm/gen_bd.h
+++ b/include/linux/fsl/bestcomm/gen_bd.h
@@ -16,10 +16,6 @@ struct bcom_gen_bd {
 };
 
 
-extern struct bcom_task *
-bcom_gen_bd_rx_init(int queue_len, phys_addr_t fifo,
-			int initiator, int ipr, int maxbufsize);
-
 extern int
 bcom_gen_bd_rx_reset(struct bcom_task *tsk);
 
@@ -27,10 +23,6 @@ extern void
 bcom_gen_bd_rx_release(struct bcom_task *tsk);
 
 
-extern struct bcom_task *
-bcom_gen_bd_tx_init(int queue_len, phys_addr_t fifo,
-			int initiator, int ipr);
-
 extern int
 bcom_gen_bd_tx_reset(struct bcom_task *tsk);
 
-- 
2.55.0

Re: [PATCHv2] dmaengine: bestcomm: make gen_bd init helpers static

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing bounds check for `psc_num` in `bcom_psc_gen_bd_tx_init` can lead to an out-of-bounds read on the `bcom_psc_params` array, resulting in a subsequent out-of-bounds memory write.
--

commit 208dbbe4b8a05c7464d2b55c9abc2d1a2ba7e83a
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: bestcomm: make gen_bd init helpers static

bcom_gen_bd_rx_init() and bcom_gen_bd_tx_init() are only called from
the PSC wrapper functions in gen_bd.c, so make them static, drop
their now-unneeded exports, and remove the corresponding header
declarations.

> diff --git a/drivers/dma/bestcomm/gen_bd.c b/drivers/dma/bestcomm/gen_bd.c
> index 8a24a5cbc2633..abe6c19f7148e 100644
> --- a/drivers/dma/bestcomm/gen_bd.c
> +++ b/drivers/dma/bestcomm/gen_bd.c

[ ... ]

> @@ -166,7 +165,7 @@ bcom_gen_bd_rx_release(struct bcom_task *tsk)
>  EXPORT_SYMBOL_GPL(bcom_gen_bd_rx_release);
>  
>  
> -extern struct bcom_task *
> +static struct bcom_task *
>  bcom_gen_bd_tx_init(int queue_len, phys_addr_t fifo,
>  			int initiator, int ipr)
>  {
> @@ -192,7 +191,6 @@ bcom_gen_bd_tx_init(int queue_len, phys_addr_t fifo,
>  
>  	return tsk;
>  }
> -EXPORT_SYMBOL_GPL(bcom_gen_bd_tx_init);

[Severity: High]
This is a pre-existing issue, but does bcom_psc_gen_bd_tx_init() lack a bounds
check for psc_num before calling this function?

While bcom_psc_gen_bd_rx_init() checks if psc_num is out of bounds:

	if (psc_num >= MPC52xx_PSC_MAXNUM)
		return NULL;

bcom_psc_gen_bd_tx_init() does not, leading to a direct out-of-bounds read on
the statically sized bcom_psc_params array:

	return bcom_gen_bd_tx_init(queue_len, fifo,
				   bcom_psc_params[psc_num].tx_initiator,
				   bcom_psc_params[psc_num].tx_ipr);

The garbage tx_initiator value read out-of-bounds is then passed to
bcom_gen_bd_tx_init() and subsequently used in bcom_gen_bd_tx_reset() as an
array index for an MMIO write:

	out_8(&bcom_eng->regs->ipr[priv->initiator], priv->ipr);

This can be triggered via mpc5200_audio_dma_create() in
sound/soc/fsl/mpc5200_dma.c, where the caller blindly passes the id derived
from the device-tree 'cell-index' property:

	psc_dma->playback.bcom_task =
		bcom_psc_gen_bd_tx_init(psc_dma->id, 10, fifo);

Since it calls tx_init before checking the result of rx_init, the out-of-bounds
read and write will unconditionally execute on a bad ID before any error path
can bail out.

Could a bounds check be added to bcom_psc_gen_bd_tx_init()?

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