Skip to contentSkip to conversation

dmaengine: fsl_raid: zero CF descriptor pool allocations

PATCH Applied Rosen Penev 4 participants 4 messages Aug 20 – Sep 10 dmaengine

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 dmaengine: fsl_raid: zero CF descriptor pool allocations

The compound frame descriptors and embedded CDBs are allocated from the cf_desc_pool with dma_pool_alloc(), which does not zero the memory. Only cdb32, the gfm coefficients and the frame address/length fields are programmed; the dpi_dest_spec/dpi_src_spec fields in the CDBs and the rbro32 field of each frame are left uninitialized. The hardware reads the whole block (frame 0 length is sizeof(struct fsl_re_xor_cdb) or sizeof(struct fsl_re_pq_cdb)), so uninitialized heap contents are exposed to the device on each new descriptor.

Use dma_pool_zalloc() in both fsl_re_chan_alloc_desc() and fsl_re_alloc_chan_resources() so the descriptor and CDB memory is zeroed before it is handed to the RAID engine.

Fixes ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by opencode:deepseek-v4-flash-free
---
 drivers/dma/fsl_raid.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/dma/fsl_raid.c +4 -4
index 2d86f61105e5..1c316d85cfec 100644
@@ -295,8 +295,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);
 		if (!cf) {
 			kfree(desc);
 			return NULL;
@@ -584,8 +584,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);
 		if (!cf) {
 			kfree(desc);
 			break;
Signature · 1 line
2.55.0
#1 lore raw
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

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.

drivers/dma/fsl_raid.c static struct fsl_re_desc *fsl_re_chan_alloc_desc(struct fsl_re_chan *re_chan, · line 298

Quoted 12 lines
drivers/dma/fsl_raid.c +2 -2
index 99945845d8b5e..1ed41d7e923bf 100644
@@ -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?

Quoted 3 lines
drivers/dma/fsl_raid.c +0 -0
 		if (!cf) {
 			kfree(desc);
 			return NULL;

[ ... ]

drivers/dma/fsl_raid.c static int fsl_re_alloc_chan_resources(struct dma_chan *chan) · line 587

Quoted 8 lines
drivers/dma/fsl_raid.c +2 -2
@@ -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?

Quoted 3 lines
drivers/dma/fsl_raid.c +0 -0
 		if (!cf) {
 			kfree(desc);
 			break;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Reviewed-by on v1 1/1
#2 lore raw
In reply to Rosen Penev
Quoted 9 lines · Rosen Penev

The compound frame descriptors and embedded CDBs are allocated from the cf_desc_pool with dma_pool_alloc(), which does not zero the memory. Only cdb32, the gfm coefficients and the frame address/length fields are programmed; the dpi_dest_spec/dpi_src_spec fields in the CDBs and the rbro32 field of each frame are left uninitialized. The hardware reads the whole block (frame 0 length is sizeof(struct fsl_re_xor_cdb) or sizeof(struct fsl_re_pq_cdb)), so uninitialized heap contents are exposed to the device on each new descriptor.

Quoted 8 lines

Use dma_pool_zalloc() in both fsl_re_chan_alloc_desc() and fsl_re_alloc_chan_resources() so the descriptor and CDB memory is zeroed before it is handed to the RAID engine.

Fixes ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by opencode:deepseek-v4-flash-free

---

Quoted 32 lines
 drivers/dma/fsl_raid.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/dma/fsl_raid.c +4 -5
index 2d86f61105e5..1c316d85cfec 100644
@@ -295,8 +295,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);
 		if (!cf) {
 			kfree(desc);
 			return NULL;
@@ -584,8 +584,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);
 		if (!cf) {
 			kfree(desc);
 			break;
--

2.55.0

maintainer Applied
#3 lore raw
In reply to Rosen Penev
Quoted 10 lines · Rosen Penev

The compound frame descriptors and embedded CDBs are allocated from the cf_desc_pool with dma_pool_alloc(), which does not zero the memory. Only cdb32, the gfm coefficients and the frame address/length fields are programmed; the dpi_dest_spec/dpi_src_spec fields in the CDBs and the rbro32 field of each frame are left uninitialized. The hardware reads the whole block (frame 0 length is sizeof(struct fsl_re_xor_cdb) or sizeof(struct fsl_re_pq_cdb)), so uninitialized heap contents are exposed to the device on each new descriptor.

[...]

Applied, thanks!

[1/1] dmaengine: fsl_raid: zero CF descriptor pool allocations

      commit: 26d48e9fb6997f4026dd86daecf8d2c12258f698

Best regards,

Signature · 1 line
~Vinod