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

[sashiko-bot@kernel.org]

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