dmaengine: fsl_raid: byte-swap cdb32 when programming CDBs

PATCH Applied Rosen Penev 4 participants 4 messages Sep 10 – Sep 15 dmaengine

Share & tools

Rosen Penev

author

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

PATCH 1/1 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.

Fixes ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")

Assisted-by opencode:big-pickle

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

---
 drivers/dma/fsl_raid.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/dma/fsl_raid.c +3 -3

index befb4bb69d54..9cc289f7129e 100644
@@ -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;

Signature · 1 line

2.55.0

[email protected]

AI review

2026-09-10 20:01 UTC #1 lore raw

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

dmaengine: fsl_raid: byte-swap cdb32 when programming CDBs

Quoted 4 lines

drivers/dma/fsl_raid.c +0 -0
index 2d86f61105e52..9845b6f1bb2c0 100644

[ ... ]

drivers/dma/fsl_raid.c static struct dma_async_tx_descriptor *fsl_re_prep_dma_pq( · line 473

Quoted 6 lines

drivers/dma/fsl_raid.c +1 -1

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

drivers/dma/fsl_raid.c

Quoted 12 lines

drivers/dma/fsl_raid.c +0 -0

 	p = pq->gfm_q1;
 	/* Init gfm_q1[] */
 	for (i = 0; i < src_cnt; i++)

drivers/dma/fsl_raid.c +0 -0

 		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?

[ ... ]

Quoted 9 lines

	/* 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?

[ ... ]

Quoted 2 lines

static void fsl_re_dequeue(struct tasklet_struct *t) {

[ ... ]

Quoted 3 lines

	spin_lock_irqsave(&re_chan->desc_lock, flags);
	count =	FSL_RE_SLOT_FULL(in_be32(&re_chan->jrregs->oubring_slot_full));
	while (count--) {

[ ... ]

Quoted 2 lines

		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?

[ ... ]

Quoted 12 lines

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.

[ ... ]

Quoted 3 lines

static int fsl_re_chan_probe(struct platform_device *ofdev,
struct device_node *np, u8 q, u32 off)
{

[ ... ]

Quoted 2 lines

	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
	if (ret) {

[ ... ]

Quoted 7 lines

	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;
	}

[ ... ]

Quoted 3 lines

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.

Signature · 1 line

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

Frank Li

Reviewed-by on v1 1/1

2026-09-11 15:17 UTC #2 lore raw

In reply to Rosen Penev

Quoted 11 lines · 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 <[email protected]>
---

Reviewed-by Frank Li <[email protected]>

Quoted 37 lines

 drivers/dma/fsl_raid.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/dma/fsl_raid.c +3 -4

index befb4bb69d54..9cc289f7129e 100644
@@ -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

Vinod Koul

maintainer Applied

2026-09-15 17:36 UTC #3 lore raw

In reply to Rosen Penev

Quoted 8 lines · 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.
[...]

Applied, thanks!

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

      commit: 8b641810905cba4eb200d72adde386bca380c0df

Best regards,

Signature · 1 line

~Vinod