Re: [PATCH] dmaengine: fsl_raid: zero CF descriptor pool allocations

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

Pre-existing issues:
- [High] When reusing a descriptor from `free_q`, `fsl_re_chan_alloc_desc()` fails to clear the residual state from the previous transaction (e.g., `async_tx.callback`, `async_tx.callback_param`).
- [High] `fsl_re_free_chan_resources()` blindly loops `alloc_count` times without verifying list emptiness, causing list corruption. Furthermore, a post-decrement on `alloc_count` causes integer underflow, leaking descriptors on subsequent allocations.
- [High] `fsl_re_dequeue()` invokes client callbacks while holding a spinlock with interrupts disabled, causing an AA deadlock if the client submits a new transaction.
- [High] The driver teardown and probe error paths completely fail to free the requested IRQ, and teardown frees DMA memory before unregistering the dmaengine device.
--

--- Patch [1]: [PATCH] dmaengine: fsl_raid: zero CF descriptor pool allocations ---
commit 0eda1d4c582e7fd0cd8ab28917a580d65f3d2988
Author: Rosen Penev <[email protected]>
Subject: dmaengine: fsl_raid: zero CF descriptor pool allocations

This patch replaces dma_pool_alloc() with dma_pool_zalloc() to ensure
compound frame descriptors and embedded CDBs are initialized to zero.
This prevents exposing uninitialized heap contents to the hardware.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..1ed41d7e923bf 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -294,8 +294,8 @@ static struct fsl_re_desc *fsl_re_chan_alloc_desc(struct fsl_re_chan *re_chan,
>  		if (!desc)
>  			return NULL;
>  
> -		cf = dma_pool_alloc(re_chan->re_dev->cf_desc_pool, GFP_NOWAIT,
> -				    &paddr);
> +		cf = dma_pool_zalloc(re_chan->re_dev->cf_desc_pool, GFP_NOWAIT,
> +				     &paddr);

[Severity: High]
This is a pre-existing issue, but when reusing a descriptor from free_q
earlier in fsl_re_chan_alloc_desc(), does it leak residual state?

If a client allocates a descriptor, sets a callback, and executes it, the
descriptor returns to free_q. When a subsequent client allocates from free_q:

drivers/dma/fsl_raid.c:fsl_re_chan_alloc_desc() {
    ...
    if (!list_empty(&re_chan->free_q)) {
        /* take one desc from free_q */
        desc = list_first_entry(&re_chan->free_q,
                    struct fsl_re_desc, node);
        list_del(&desc->node);

        desc->async_tx.flags = flags;
    }
    ...
}

Only async_tx.flags is updated here, leaving async_tx.callback and
callback_param populated. Could this result in spurious callbacks being
invoked with freed structures if the new transaction does not explicitly
clear them?

>  		if (!cf) {
>  			kfree(desc);
>  			return NULL;

[ ... ]

> @@ -583,8 +583,8 @@ static int fsl_re_alloc_chan_resources(struct dma_chan *chan)
>  		if (!desc)
>  			break;
>  
> -		cf = dma_pool_alloc(re_chan->re_dev->cf_desc_pool, GFP_KERNEL,
> -				    &paddr);
> +		cf = dma_pool_zalloc(re_chan->re_dev->cf_desc_pool, GFP_KERNEL,
> +				     &paddr);

[Severity: High]
This isn't a bug introduced by this patch, but does the way alloc_count is
tracked lead to integer underflow and list corruption in
fsl_re_free_chan_resources()?

In fsl_re_free_chan_resources(), there is a blind loop:

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

If the channel is freed while any descriptors are still active (not in
free_q), the loop will run more times than items exist in free_q. Would
this result in list_first_entry() returning an invalid pointer and causing
list corruption?

Additionally, if alloc_count reaches zero and the post-decrement underflows
it to UINT_MAX, wouldn't subsequent calls to fsl_re_alloc_chan_resources()
permanently break descriptor tracking when incrementing alloc_count?

[Severity: High]
This is a pre-existing issue, but could invoking client callbacks from
fsl_re_dequeue() while holding desc_lock cause an AA deadlock?

drivers/dma/fsl_raid.c:fsl_re_dequeue() {
    ...
    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);
    ...
}

The fsl_re_desc_done() function executes the client's completion callback.
If the client submits a new transaction from within this callback,
fsl_re_tx_submit() will attempt to acquire re_chan->desc_lock again. Does
this result in a recursive spinlock deadlock?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver fail to free
its requested IRQ, potentially leading to a use-after-free?

In fsl_re_chan_probe(), an IRQ is requested without devm_:

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

However, free_irq() does not appear to be called in the probe error paths
or during driver removal in fsl_re_remove_chan(). If the device is removed,
re_chan is freed via devm_kzalloc. If a subsequent hardware interrupt
occurs, won't fsl_re_isr() attempt to dereference the freed re_chan
pointer?

Also, in fsl_re_remove(), memory areas and tasklets are destroyed before
the device is unregistered:

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

Does this allow clients to still access DMA resources while they are being
dismantled?

>  		if (!cf) {
>  			kfree(desc);
>  			break;

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