* Re: [PATCHv3] dmaengine: bestcomm: gen_bd: fix out-of-bounds access in PSC parameter lookup
2026-09-11 21:37 [PATCHv3] dmaengine: bestcomm: gen_bd: fix out-of-bounds access in PSC parameter lookup Rosen Penev
@ 2026-09-11 21:48 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-09-11 21:48 UTC (permalink / raw)
To: Rosen Penev; +Cc: Frank.Li, dmaengine, vkoul
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch fails to completely resolve the vulnerability. The patch correctly addresses an out-of-bounds array access in `bcom_psc_gen_bd_rx_init()` by checking `psc_num >= ARRAY_SIZE(bcom_psc_params)`, but leaves the identical vulnerability unpatched in its sibling function `bcom_psc_gen_bd_tx_init()`. The author explicitly claims in the commit message that the TX bounds check was omitted because the function is 'not used anywhere anyway.' However, `bcom_psc_gen_bd_tx_init()` is exported and actively used in `sound/soc/fsl/mpc5200_dma.c`. In `mpc5200_audio_dma_create()`, `tx_init` is called unconditionally immediately after `rx_init`, reusing the exact same unvalidated device tree `cell-index`. If a malformed `cell-index` is provided, `rx_init` gracefully returns `NULL`, but execution continues to `tx_init`, where the unprotected out-of-bounds array read still occurs. These garbage values are then used as an index for an MMIO write in `bcom_gen_bd_tx_reset()`, resulting in a potentially severe out-of-bounds MMIO access.
--
commit fe70e15aeccf81a4b8cdfa4a3e93e93a4e989ae4
Author: Rosen Penev <rosenp@gmail.com>
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.
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.
> diff --git a/drivers/dma/bestcomm/gen_bd.c b/drivers/dma/bestcomm/gen_bd.c
> index 8a24a5cbc2633..ce59a79325657 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,
[Severity: High]
This is a pre-existing issue, but does this patch fail to completely resolve
the vulnerability by leaving the identical out-of-bounds access unpatched in
the sibling function bcom_psc_gen_bd_tx_init()?
The bcom_psc_params array access in bcom_psc_gen_bd_tx_init() remains
completely unprotected by bounds checks:
drivers/dma/bestcomm/gen_bd.c:bcom_psc_gen_bd_tx_init() {
...
return bcom_gen_bd_tx_init(queue_len, fifo,
bcom_psc_params[psc_num].tx_initiator,
bcom_psc_params[psc_num].tx_ipr);
}
The patch omits the bounds check in the TX path on the assumption that the
function is unused, but the function is exported and actively used here:
sound/soc/fsl/mpc5200_dma.c:mpc5200_audio_dma_create() {
...
psc_dma->playback.bcom_task =
bcom_psc_gen_bd_tx_init(psc_dma->id, 10, fifo);
...
}
In mpc5200_audio_dma_create(), tx_init is called unconditionally immediately
after rx_init, reusing the exact same unvalidated device tree cell-index. If a
malformed cell-index is provided, rx_init gracefully returns NULL, but
execution continues to tx_init, where the unprotected out-of-bounds array read
still occurs.
These garbage values are then used as an index for an MMIO write in
bcom_gen_bd_tx_reset(), resulting in a potentially severe out-of-bounds MMIO
access:
drivers/dma/bestcomm/gen_bd.c:bcom_gen_bd_tx_reset() {
...
out_8(&bcom_eng->regs->ipr[priv->initiator], priv->ipr);
...
}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260911213747.132694-1-rosenp@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread