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

[sashiko-bot@kernel.org]

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