[PATCH] dmaengine: fsl_raid: byte-swap cdb32 when programming CDBs

[PATCH] dmaengine: fsl_raid: byte-swap cdb32 when programming CDBs

[Rosen Penev]

The three prep functions write the first word of the command descriptor
block (cdb32) with a plain u32 assignment, while every other CDB/CF
descriptor field is written with cpu_to_be32(). The RAID Engine expects
big-endian descriptors, so the plain assignment produces a corrupted CDB
on little-endian hosts. Use cpu_to_be32() for cdb32 to match the
surrounding code.

Fixes: ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/fsl_raid.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index befb4bb69d54..9cc289f7129e 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -361,7 +361,7 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_genq(
 	cdb |= FSL_RE_INTR_ON_ERROR << FSL_RE_CDB_ERROR_SHIFT;
 	cdb |= FSL_RE_DATA_DEP << FSL_RE_CDB_DEPEND_SHIFT;
 	xor = desc->cdb_addr;
-	xor->cdb32 = cdb;
+	xor->cdb32 = cpu_to_be32(cdb);
 
 	if (scf) {
 		/* compute q = src0*coef0^src1*coef1^..., * is GF(8) mult */
@@ -481,7 +481,7 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_pq(
 	cdb |= FSL_RE_DATA_DEP << FSL_RE_CDB_DEPEND_SHIFT;
 
 	pq = desc->cdb_addr;
-	pq->cdb32 = cdb;
+	pq->cdb32 = cpu_to_be32(cdb);
 
 	p = pq->gfm_q1;
 	/* Init gfm_q1[] */
@@ -564,7 +564,7 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_memcpy(
 	cdb |= FSL_RE_DATA_DEP << FSL_RE_CDB_DEPEND_SHIFT;
 
 	move = desc->cdb_addr;
-	move->cdb32 = cdb;
+	move->cdb32 = cpu_to_be32(cdb);
 
 	/* Filling frame 0 of CFD with move CDB */
 	cf = desc->cf_addr;
-- 
2.55.0

Re: [PATCH] dmaengine: fsl_raid: byte-swap cdb32 when programming CDBs

[sashiko-bot]

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

Pre-existing issues:
- [High] Buffer overflow in the compound frame descriptor array overwrites the Command Descriptor Block (CDB) during PQ continuation operations.
- [High] Out-of-bounds array read on the caller-provided `scf` coefficients array in `fsl_re_prep_dma_pq` when `DMA_PREP_CONTINUE` is set.
- [High] Deadlock from calling client DMA callbacks while holding the channel descriptor spinlock.
- [High] List corruption and kernel panic in `fsl_re_free_chan_resources` due to blind extraction from `free_q`.
- [High] Missing IRQ cleanup on probe error, causing IRQ resource leak and potential NULL pointer dereference in the ISR.
--

commit 35b8359a749c05ccda3b77bf570e1ce40a818eb4
Author: Rosen Penev <rosenp@gmail.com>

    dmaengine: fsl_raid: byte-swap cdb32 when programming CDBs
    
    The three prep functions write the first word of the command descriptor
    block (cdb32) with a plain u32 assignment, while every other CDB/CF
    descriptor field is written with cpu_to_be32(). The RAID Engine expects
    big-endian descriptors, so the plain assignment produces a corrupted CDB
    on little-endian hosts. Use cpu_to_be32() for cdb32 to match the
    surrounding code.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 2d86f61105e52..9845b6f1bb2c0 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
[ ... ]
> @@ -470,7 +470,7 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_pq(
>  	cdb |= FSL_RE_DATA_DEP << FSL_RE_CDB_DEPEND_SHIFT;
>  
>  	pq = desc->cdb_addr;
> -	pq->cdb32 = cdb;
> +	pq->cdb32 = cpu_to_be32(cdb);

While reviewing this patch, I noticed a few pre-existing issues in the
surrounding driver code.

>  	p = pq->gfm_q1;
>  	/* Init gfm_q1[] */
>  	for (i = 0; i < src_cnt; i++)
>  		p[i] = 1;
> 
>  	/* Align gfm[] to 32bit */
>  	gfmq_len = ALIGN(src_cnt, 4);
> 
>  	/* Init gfm_q2[] */
>  	p += gfmq_len;
>  	for (i = 0; i < src_cnt; i++)
>  		p[i] = scf[i];

[Severity: High]
This isn't a bug introduced by this patch, but does this loop read out of
bounds of the caller-provided scf array?

When DMA_PREP_CONTINUE is set, src_cnt is artificially inflated by 3 earlier
in the function to account for continuation buffers:

	if (flags & DMA_PREP_CONTINUE)
		src_cnt += 3;

However, this inflated count is then used as the loop boundary when reading
from the caller's strictly-sized scf array. Can this cause a KASAN
out-of-bounds read panic or disclose unrelated adjacent memory?

[ ... ]
> 	/* PQ computation continuation */
> 	if (flags & DMA_PREP_CONTINUE) {
> 		if (src_cnt - save_src_cnt == 3) {
> 			p[save_src_cnt] = 0;
> 			p[save_src_cnt + 1] = 0;
> 			p[save_src_cnt + 2] = 1;
> 			fill_cfd_frame(cf, i++, len, dest[0], 0);
> 			fill_cfd_frame(cf, i++, len, dest[1], 0);
> 			fill_cfd_frame(cf, i++, len, dest[1], 1);

[Severity: High]
This is a pre-existing issue, but does this continuation sequence cause a
buffer overflow that overwrites the Command Descriptor Block (CDB)?

The CDB is allocated exactly at the end boundary of the compound frame array:

static struct fsl_re_desc *fsl_re_init_desc(...)
{
    ...
	desc->cdb_addr = (void *)(cf + FSL_RE_CF_DESC_SIZE);
    ...
}

The compound frame descriptor array holds exactly 20 frames (320 bytes / 16
bytes per frame). If a caller invokes fsl_re_prep_dma_pq() with the maximum
16 sources (FSL_RE_MAX_PQ_SRCS) and the DMA_PREP_CONTINUE flag is set, the
driver writes 1 CDB frame, 2 destination frames, 16 source frames, and 3
continuation frames (22 frames total).

Does this sequence push the frame index i to 20 and 21, overflowing the
bounds at the end of the array and directly corrupting the CDB residing
immediately after it?

[ ... ]
> static void fsl_re_dequeue(struct tasklet_struct *t)
> {
[ ... ]
> 	spin_lock_irqsave(&re_chan->desc_lock, flags);
> 	count =	FSL_RE_SLOT_FULL(in_be32(&re_chan->jrregs->oubring_slot_full));
> 	while (count--) {
[ ... ]
> 		if (found) {
> 			fsl_re_desc_done(desc);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
fsl_re_desc_done() with the desc_lock held cause a deadlock?

Inside fsl_re_desc_done():

static void fsl_re_desc_done(struct fsl_re_desc *desc)
{
	...
	dmaengine_desc_get_callback_invoke(&desc->async_tx, NULL);
}

This invokes the client's callback synchronously. A standard pattern in DMA
clients (like MD RAID) is to immediately chain the next request inside the
completion callback. The fsl_re_tx_submit() function unconditionally tries
to acquire the same desc_lock:

static dma_cookie_t fsl_re_tx_submit(struct dma_async_tx_descriptor *tx)
{
    ...
	spin_lock_irqsave(&re_chan->desc_lock, flags);

Can this trigger an immediate spinlock recursion deadlock?

[ ... ]
> static void fsl_re_free_chan_resources(struct dma_chan *chan)
> {
> 	struct fsl_re_chan *re_chan;
> 	struct fsl_re_desc *desc;
> 
> 	re_chan = container_of(chan, struct fsl_re_chan, chan);
> 	while (re_chan->alloc_count--) {
> 		desc = list_first_entry(&re_chan->free_q,
> 					struct fsl_re_desc,
> 					node);
> 
> 		list_del(&desc->node);

[Severity: High]
This is a pre-existing issue, but can this cause list corruption and a kernel
panic if there are pending transactions?

The driver loops exactly alloc_count times calling list_first_entry()
without checking list_empty(). If any allocated descriptors are absent from
free_q (for example, if they are active, in ack_q, or leaked), the list will
be empty before the loop finishes. This would cast the list head itself to a
descriptor, call list_del() and kfree(), and corrupt kernel memory.

[ ... ]
> static int fsl_re_chan_probe(struct platform_device *ofdev,
> 		      struct device_node *np, u8 q, u32 off)
> {
[ ... ]
> 	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
> 	if (ret) {
[ ... ]
> 	chan->inb_ring_virt_addr = dma_pool_alloc(chan->re_dev->hw_desc_pool,
> 		GFP_KERNEL, &chan->inb_phys_addr);
> 	if (!chan->inb_ring_virt_addr) {
> 		dev_err(dev, "No dma memory for inb_ring_virt_addr\n");
> 		ret = -ENOMEM;
> 		goto err_free;
> 	}
[ ... ]
> err_free:
> 	return ret;
> }

[Severity: High]
This isn't a bug introduced by this patch, but does this leak the active IRQ
handler if memory allocation fails?

If a step like dma_pool_alloc() fails, the code jumps to err_free without
calling free_irq(). Additionally, dev_set_drvdata() is only called at the
end of the probe. If a spurious interrupt fires after the error return,
fsl_re_isr() will fetch NULL from dev_get_drvdata() and immediately
dereference it.

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

Re: [PATCH] dmaengine: fsl_raid: byte-swap cdb32 when programming CDBs

[Frank Li]

On Thu, Sep 10, 2026 at 12:43:24PM -0700, Rosen Penev wrote:
> The three prep functions write the first word of the command descriptor
> block (cdb32) with a plain u32 assignment, while every other CDB/CF
> descriptor field is written with cpu_to_be32(). The RAID Engine expects
> big-endian descriptors, so the plain assignment produces a corrupted CDB
> on little-endian hosts. Use cpu_to_be32() for cdb32 to match the
> surrounding code.
>
> Fixes: ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/fsl_raid.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index befb4bb69d54..9cc289f7129e 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -361,7 +361,7 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_genq(
>  	cdb |= FSL_RE_INTR_ON_ERROR << FSL_RE_CDB_ERROR_SHIFT;
>  	cdb |= FSL_RE_DATA_DEP << FSL_RE_CDB_DEPEND_SHIFT;
>  	xor = desc->cdb_addr;
> -	xor->cdb32 = cdb;
> +	xor->cdb32 = cpu_to_be32(cdb);
>
>  	if (scf) {
>  		/* compute q = src0*coef0^src1*coef1^..., * is GF(8) mult */
> @@ -481,7 +481,7 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_pq(
>  	cdb |= FSL_RE_DATA_DEP << FSL_RE_CDB_DEPEND_SHIFT;
>
>  	pq = desc->cdb_addr;
> -	pq->cdb32 = cdb;
> +	pq->cdb32 = cpu_to_be32(cdb);
>
>  	p = pq->gfm_q1;
>  	/* Init gfm_q1[] */
> @@ -564,7 +564,7 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_memcpy(
>  	cdb |= FSL_RE_DATA_DEP << FSL_RE_CDB_DEPEND_SHIFT;
>
>  	move = desc->cdb_addr;
> -	move->cdb32 = cdb;
> +	move->cdb32 = cpu_to_be32(cdb);
>
>  	/* Filling frame 0 of CFD with move CDB */
>  	cf = desc->cf_addr;
> --
> 2.55.0
>