Re: [PATCHv2 2/4] dma: fsl_raid: set final bit via fill_cfd_frame() argument

[sashiko-bot@kernel.org]

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

New issues:
- [High] Integer underflow leading to wild pointer dereference when src_cnt == 0.
- [Medium] The commit message claims to fix an endianness issue by setting a bit before a 'single cpu_to_be32() store', but this byte-swap is completely missing from the modified code.

Pre-existing issues:
- [High] Multiple severe out-of-bounds writes in `fsl_re_prep_dma_pq()` corrupt the Command Descriptor Block (CDB) when `DMA_PREP_CONTINUE` is set.
- [High] Missing IRQ cleanup leaves the interrupt handler active after module unload.
- [High] Incorrect teardown sequence frees DMA channel resources before unregistering the DMA device.
- [High] Ignored probe failures lead to uninitialized or partially initialized channels being torn down, causing NULL pointer dereferences or double frees.
- [High] Recursive locking deadlock when invoking DMA client callback.
- [High] Race condition due to prematurely enabling interrupts before initialization is complete.
- [High] DMA transfers of exactly 1MB will be silently truncated to 0 bytes due to `FSL_RE_CF_LENGTH_MASK` mismatch.
--

--- Patch [1]: [PATCHv2 2/4] dma: fsl_raid: set final bit via fill_cfd_frame() argument ---
commit 62a6bad1aef1e87fcc87f1d390c2ed3be7152732
Author: Rosen Penev <rosenp@gmail.com>

dma: fsl_raid: set final bit via fill_cfd_frame() argument

The final-frame bit is now passed as the "final" argument of
fill_cfd_frame() (as fsl_re_prep_dma_memcpy already did) and set in CPU
order before the single cpu_to_be32() store, replacing the previous
read-modify-write of the __be32 efrl32 field.

[Severity: Medium]
The commit message claims to fix an endianness issue by setting a bit before
a single cpu_to_be32() store, but this byte-swap appears to be missing from
the modified code.  Looking at fill_cfd_frame() in drivers/dma/fsl_raid.c,
the native integer is assigned directly to the __be32 field:

    cf[index].efrl32 = efrl;

Could this leave the type mismatch unresolved and the endianness bug in
place on little-endian hardware?

> ---
>  drivers/dma/fsl_raid.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..a4feaa9a1c628 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -374,11 +374,11 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_genq(
>  	for (i = 2, j = 0; j < save_src_cnt; i++, j++)
>  		fill_cfd_frame(cf, i, len, src[j], 0);
>  
> +	/* Fill the last frame and mark it final */
>  	if (cont_q)
> -		fill_cfd_frame(cf, i++, len, dest, 0);
> -
> -	/* Setting the final bit in the last source buffer frame in CFD */
> -	cf[i - 1].efrl32 |= 1 << FSL_RE_CF_FINAL_SHIFT;
> +		fill_cfd_frame(cf, i, len, dest, 1);
> +	else
> +		fill_cfd_frame(cf, i - 1, len, src[j - 1], 1);

[Severity: High]
Does this code cause an integer underflow when fsl_re_prep_dma_genq() is
called with src_cnt == 0?

If the loop initializing cf frames is completely skipped, j remains 0
(unsigned), causing j - 1 to underflow to UINT_MAX.  This could lead to a
wild pointer dereference when accessing src[j - 1].

>  	return &desc->async_tx;
>  }

[ ... ]

> @@ -504,16 +504,16 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_pq(
>  			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], 0);
> +			fill_cfd_frame(cf, i++, len, dest[1], 1);

[Severity: High]
This is a pre-existing issue, but does this continuation path write out of
bounds?

When dmaengine_prep_dma_pq is called with DMA_PREP_CONTINUE and a
src_cnt of 14 to 16, the driver artificially increments src_cnt by 3 (up to
19).  This causes the loop initializing gfm_q1 (and subsequently scf copying)
to overflow the fixed-size 16-byte arrays.

Additionally, the continuation frames are written to cf[19], cf[20], and
cf[21], which overflows the 20-frame limit and can corrupt the adjacent
hardware Command Descriptor Block (CDB) structure.

>  		} else {
>  			dev_err(re_chan->dev, "PQ tx continuation error!\n");
>  			return NULL;
>  		}
> +	} else {
> +		/* Mark the last source buffer frame final */
> +		fill_cfd_frame(cf, i - 1, len, src[j - 1], 1);

[Severity: High]
Similar to the issue in fsl_re_prep_dma_genq(), does this access cause an
integer underflow if fsl_re_prep_dma_pq() is called with src_cnt == 0?

>  	}
>  
> -	/* Setting the final bit in the last source buffer frame in CFD */
> -	cf[i - 1].efrl32 |= 1 << FSL_RE_CF_FINAL_SHIFT;
> -
>  	return &desc->async_tx;
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but does fsl_re_remove_chan() leak the
interrupt handler after module unload?

The function kills the tasklet and frees memory, but fails to call
free_irq(chan->irq, chan->dev).  This could leave the ISR active after the
driver is unloaded, potentially causing a use-after-free on the next
hardware interrupt:

drivers/dma/fsl_raid.c:fsl_re_remove_chan() {
    ...
	tasklet_kill(&chan->irqtask);
    ...
}

[Severity: High]
This is a pre-existing issue, but does fsl_re_remove() free DMA channel
resources before unregistering the DMA device?

The teardown sequence iterates over channels to destroy their rings before
notifying the DMA core.  This creates a race condition where a concurrent
client might submit a new DMA request into already freed memory:

drivers/dma/fsl_raid.c:fsl_re_remove() {
    ...
	/* Cleanup chan related memory areas */
	for (i = 0; i < re_priv->total_chans; i++)
		fsl_re_remove_chan(re_priv->re_jrs[i]);
	
	/* Unregister the driver */
	dma_async_device_unregister(&re_priv->dma_dev);
    ...
}

[Severity: High]
This is a pre-existing issue, but does fsl_re_probe() ignore probe failures
and blindly increment the active channel count?

drivers/dma/fsl_raid.c:fsl_re_probe() {
    ...
		if (rc) {
			fsl_re_chan_probe(ofdev, child, ridx++, off);
			re_priv->total_chans++;
		}
    ...
}

If fsl_re_chan_probe() fails, re_jrs[i] remains NULL.  During unload, the
teardown loop unconditionally dereferences it:

drivers/dma/fsl_raid.c:fsl_re_remove() {
    ...
	for (i = 0; i < re_priv->total_chans; i++)
		fsl_re_remove_chan(re_priv->re_jrs[i]);
    ...
}

Could this lead to a NULL pointer dereference in tasklet_kill() if a channel
failed to probe?

[Severity: High]
This is a pre-existing issue, but can fsl_re_dequeue() cause a recursive
locking deadlock if the client submits a new transaction from the callback?

fsl_re_dequeue() acquires desc_lock and holds it while executing the client
callback:

drivers/dma/fsl_raid.c:fsl_re_dequeue() {
    ...
	spin_lock_irqsave(&re_chan->desc_lock, flags);
	...
		fsl_re_desc_done(desc);
    ...
}

If the callback implements request chaining, fsl_re_tx_submit() will attempt
to synchronously re-acquire the exact same desc_lock:

drivers/dma/fsl_raid.c:fsl_re_tx_submit() {
    ...
	spin_lock_irqsave(&re_chan->desc_lock, flags);
    ...
}

[Severity: High]
This is a pre-existing issue, but does fsl_re_chan_probe() enable interrupts
before initialization is complete?

The IRQ is requested before drvdata is persistently linked to the device:

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
	...
	dev_set_drvdata(chandev, chan);
    ...
}

If a pending or spurious interrupt fires immediately, fsl_re_isr() will call
dev_get_drvdata(), which returns NULL, and then dereference it:

drivers/dma/fsl_raid.c:fsl_re_isr() {
    ...
	re_chan = dev_get_drvdata((struct device *)data);

	irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);
    ...
}

[Severity: High]
This is a pre-existing issue, but does fill_cfd_frame() silently truncate
1MB transfers to 0 bytes?

drivers/dma/fsl_raid.c:fill_cfd_frame() {
    ...
	u32 efrl = length & FSL_RE_CF_LENGTH_MASK;
    ...
}

FSL_RE_MAX_DATA_LEN is defined as 1024*1024 (0x100000), allowing exactly 1MB.
However, if FSL_RE_CF_LENGTH_MASK is 0x000FFFFF (20 bits), the bitwise-AND
operation will silently truncate a 1MB transfer to 0.

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