Re: [PATCH] dmaengine: fsl_raid: fix sparse warnings

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete Endianness Fix
- [High] Use-After-Free / Resource Leak
- [High] Incorrect teardown ordering in `fsl_re_remove()`
- [High] Memory Corruption due to unconditional pop in `fsl_re_free_chan_resources()`
- [High] Deadlock during client callback invocation
- [High] NULL Pointer Dereference on probe failure
- [Medium] Uninitialized DMA Memory Leak
--

--- Patch [1]: [PATCH] dmaengine: fsl_raid: fix sparse warnings ---
commit 736f7849012b086f7c1f995cc45ee838fbb53650
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsl_raid: fix sparse warnings

The register maps in fsl_raid.h annotate every field as __be32, but
they are accessed only through ioread32be()/iowrite32be(), which
already perform the byte swap and take plain u32 __iomem pointers.
The __be32 annotation therefore makes sparse reject each access.

Closes: https://lore.kernel.org/oe-kbuild-all/202608200637.ro1CqMxi-lkp@intel.com/

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..97103c84dd70b 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -334,7 +334,7 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_genq(
>  	}
>  
>  	desc = fsl_re_chan_alloc_desc(re_chan, flags);
> -	if (desc <= 0)
> +	if (!desc)

[Severity: Medium]
This is a pre-existing issue, but does this allocation leak uninitialized
kernel memory to the device?

Looking at fsl_re_chan_alloc_desc():

drivers/dma/fsl_raid.c:fsl_re_chan_alloc_desc() {
    ...
    cf = dma_pool_alloc(re_chan->re_dev->cf_desc_pool, GFP_NOWAIT, &paddr);
    ...
}

Since the compound frame descriptors are dynamically allocated without zeroing
(__GFP_ZERO), could the uninitialized dpi and trailing gfm array fields expose
kernel heap state to the device?

>  		return NULL;
>  
>  	if (scf && (flags & DMA_PREP_CONTINUE)) {
> @@ -349,7 +349,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);

[Severity: High]
This is a pre-existing issue, but does swapping only cdb32 here leave the
driver broken on little-endian platforms?

Looking at fill_cfd_frame() which is called shortly after:

drivers/dma/fsl_raid.c:fill_cfd_frame() {
    cf[index].efrl32 = efrl;
    cf[index].addr_high = upper_32_bits(addr);
    cf[index].addr_low = lower_32_bits(addr);
    ...
}

It appears native u32 values are still assigned to __be32 fields without
swapping. Will this incomplete endianness conversion cause issues on
little-endian hosts?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the driver leak the IRQ resource or
cause a use-after-free on unbind?

Looking at fsl_re_chan_probe():

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

The IRQ is requested here but appears to lack cleanup in error paths and the
remove function. Upon device unbind, could the IRQ remain active while the
channel structure is freed, leading to a use-after-free on the next interrupt?

[Severity: High]
This is a pre-existing issue, but does the teardown order in fsl_re_remove()
violate DMA engine lifecycle constraints?

Looking at fsl_re_remove():

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]);

    dma_async_device_unregister(&re_priv->dma_dev);
    ...
}

Since it destroys channel resources before unregistering the DMA device, does
this race condition permit concurrent client requests to operate on freed
memory?

[Severity: High]
This is a pre-existing issue, but can the loop in fsl_re_free_chan_resources()
cause memory corruption?

Looking at fsl_re_free_chan_resources():

drivers/dma/fsl_raid.c:fsl_re_free_chan_resources() {
    ...
    while (re_chan->alloc_count--) {
        desc = list_first_entry(&re_chan->free_q, struct fsl_re_desc, node);
        list_del(&desc->node);
        ...
    }
    ...
}

The cleanup routine loops blindly based on alloc_count. If descriptors are
currently active or submitted, could free_q run out early and cause
list_first_entry() to pop invalid pointers?

[Severity: High]
This is a pre-existing issue, but could fsl_re_dequeue() deadlock during client
callback invocation?

Looking at fsl_re_dequeue():

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

The client callback is triggered while a non-recursive spinlock is held. Will
this self-deadlock if the callback immediately submits a new transaction?

[Severity: High]
This is a pre-existing issue, but does fsl_re_probe() risk a NULL pointer
dereference on probe failure?

Looking at fsl_re_probe():

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

Since the return code of fsl_re_chan_probe() is ignored and total_chans is
incremented unconditionally, will failed channel allocations leave re_jrs[q]
as NULL, causing them to be blindly dereferenced on module removal?

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