dmaengine: idma64: descriptor allocation and length limit fixes

PATCH In review Rosen Penev 3 participants 7 messages Jul 12 – Jul 13

Subsystems (2)

dmaengine Intel LPSS (Low Power SubSystem) drivers

lore

Share & tools

lore

Rosen Penev

author

2026-07-12 22:00 UTC #0 lore raw

v2 cover dmaengine: idma64: descriptor allocation and length limit fixes

This small series cleans up the idma64 descriptor allocation and fixes a long-standing truncation bug in idma64_prep_slave_sg().

Patch 1 replaces the open-coded two-stage allocation in idma64_alloc_desc() with kzalloc_flex() using a flexible array member for the hardware descriptor list, annotated with __counted_by for extra runtime bounds checking. The now-redundant helper is removed.

Patch 2 addresses the hardware limit. The iDMA 64-bit CTL_HI BLOCK_TS field is only 17 bits (IDMA64C_CTLH_BLOCK_TS_MASK = 0x1ffff), so when a scatterlist entry exceeds that size the driver would silently truncate the transfer length. Use sg_nents_for_dma() to size the descriptor ring after splitting oversized entries, and iterate the new per-chunk loop so each hardware descriptor stays within the field.

Rosen Penev (2):

Rosen Penev (2):
dmaengine: idma64: use kzalloc_flex
dmaengine: idma64: use sg_nents_for_dma to respect hardware descriptor
length limit

v2: add second patch

 drivers/dma/idma64.c | 70 ++++++++++++++++++++------------------------
 drivers/dma/idma64.h |  7 +++--
 2 files changed, 36 insertions(+), 41 deletions(-)

Changes since v1:
- (fill in changes here)

Signature · 1 line

2.55.0

Rosen Penev

author

2026-07-12 22:00 UTC #1 lore raw

PATCH v2 1/2 dmaengine: idma64: use kzalloc_flex

Simplifies allocations by using a flexible array member in this struct.

Remove idma64_alloc_desc. It now offers no readability advantages in this single usage.

Add __counted_by to get extra runtime analysis.

Apply the exact same treatment to struct idma64_dma and devm_kzalloc.

Signed-off-by Rosen Penev <[email protected]>

Reviewed-by Frank Li <[email protected]>

---
 drivers/dma/idma64.c | 30 ++++--------------------------
 drivers/dma/idma64.h |  4 ++--
 2 files changed, 6 insertions(+), 28 deletions(-)

drivers/dma/idma64.c +4 ~~-26~~

index 5fcd1befc92d..d914f50ec309 100644
@@ -192,23 +192,6 @@ static irqreturn_t idma64_irq(int irq, void *dev)
 
 /* ---------------------------------------------------------------------- */
 
-static struct idma64_desc *idma64_alloc_desc(unsigned int ndesc)
-{
-	struct idma64_desc *desc;
-
-	desc = kzalloc_obj(*desc, GFP_NOWAIT);
-	if (!desc)
-		return NULL;
-
-	desc->hw = kzalloc_objs(*desc->hw, ndesc, GFP_NOWAIT);
-	if (!desc->hw) {
-		kfree(desc);
-		return NULL;
-	}
-
-	return desc;
-}
-
 static void idma64_desc_free(struct idma64_chan *idma64c,
 		struct idma64_desc *desc)
 {

@@ -223,7 +206,6 @@ static void idma64_desc_free(struct idma64_chan *idma64c,
 		} while (i);
 	}
 
-	kfree(desc->hw);
 	kfree(desc);
 }

@@ -307,10 +289,12 @@ static struct dma_async_tx_descriptor *idma64_prep_slave_sg(
 	struct scatterlist *sg;
 	unsigned int i;
 
-	desc = idma64_alloc_desc(sg_len);
+	desc = kzalloc_flex(*desc, hw, sg_len, GFP_NOWAIT);
 	if (!desc)
 		return NULL;
 
+	desc->ndesc = sg_len;
+
 	for_each_sg(sgl, sg, sg_len, i) {
 		struct idma64_hw_desc *hw = &desc->hw[i];

@@ -326,7 +310,6 @@ static struct dma_async_tx_descriptor *idma64_prep_slave_sg(
 		hw->len = sg_dma_len(sg);
 	}
 
-	desc->ndesc = sg_len;
 	desc->direction = direction;
 	desc->status = DMA_IN_PROGRESS;

@@ -541,18 +524,13 @@ static int idma64_probe(struct idma64_chip *chip)
 	unsigned short i;
 	int ret;
 
-	idma64 = devm_kzalloc(chip->dev, sizeof(*idma64), GFP_KERNEL);
+	idma64 = devm_kzalloc(chip->dev, struct_size(idma64, chan, nr_chan), GFP_KERNEL);
 	if (!idma64)
 		return -ENOMEM;
 
 	idma64->regs = chip->regs;
 	chip->idma64 = idma64;
 
-	idma64->chan = devm_kcalloc(chip->dev, nr_chan, sizeof(*idma64->chan),
-				    GFP_KERNEL);
-	if (!idma64->chan)
-		return -ENOMEM;
-
 	idma64->all_chan_mask = (1 << nr_chan) - 1;
 
 	/* Turn off iDMA controller */

drivers/dma/idma64.h +2 -2

index d013b54356aa..1a67dbb24db5 100644
@@ -113,10 +113,10 @@ struct idma64_hw_desc {
 struct idma64_desc {
 	struct virt_dma_desc vdesc;
 	enum dma_transfer_direction direction;
-	struct idma64_hw_desc *hw;
 	unsigned int ndesc;
 	size_t length;
 	enum dma_status status;
+	struct idma64_hw_desc hw[] __counted_by(ndesc);
 };
 
 static inline struct idma64_desc *to_idma64_desc(struct virt_dma_desc *vdesc)

@@ -187,7 +187,7 @@ struct idma64 {
 
 	/* channels */
 	unsigned short all_chan_mask;
-	struct idma64_chan *chan;
+	struct idma64_chan chan[];
 };
 
 static inline struct idma64 *to_idma64(struct dma_device *ddev)

Signature · 1 line

2.55.0

[email protected]

AI review

2026-07-12 22:11 UTC #2 lore raw

Re: [PATCHv2 1/2] dmaengine: idma64: use kzalloc_flex

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

Pre-existing issues:
- [High] If idma64_prep_slave_sg() is called with sg_len == 0, it eventually calls idma64_desc_fill() which accesses desc->hw[UINT_MAX], causing a kernel panic. --

--- Patch [1]: [PATCHv2 1/2] dmaengine: idma64: use kzalloc_flex --- commit 54aec29a1bbe69497782adf915ba6169fd73e345
Author: Rosen Penev <[email protected]>

dmaengine: idma64: use kzalloc_flex

This commit refactors the idma64_desc and idma64 structures to use flexible array members. It simplifies allocations by using kzalloc_flex and adds __counted_by annotations for runtime bounds checking.

Quoted 4 lines

drivers/dma/idma64.c +0 -0
index 5fcd1befc92da..d914f50ec3096 100644

[ ... ]

drivers/dma/idma64.c static struct dma_async_tx_descriptor *idma64_prep_slave_sg( · line 297

Quoted 11 lines

drivers/dma/idma64.c +3 -1

@@ -307,10 +289,12 @@ static struct dma_async_tx_descriptor *idma64_prep_slave_sg(
 	struct scatterlist *sg;
 	unsigned int i;
 
-	desc = idma64_alloc_desc(sg_len);
+	desc = kzalloc_flex(*desc, hw, sg_len, GFP_NOWAIT);
 	if (!desc)
 		return NULL;
 
+	desc->ndesc = sg_len;
+

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds access if idma64_prep_slave_sg() is called with sg_len == 0?

If sg_len is 0, desc->ndesc is set to 0 and the for_each_sg() loop is bypassed. The function then calls idma64_desc_fill() which does the following:

drivers/dma/idma64.c:idma64_desc_fill() {
    ...
    unsigned int i = desc->ndesc;
    struct idma64_hw_desc *hw = &desc->hw[i - 1];
    ...
}

Since i is 0, i - 1 underflows to UINT_MAX, causing an out-of-bounds access on the hw flex array. With the new __counted_by bounds check, this will likely trigger a panic immediately.

Quoted 3 lines

drivers/dma/idma64.c +0 -0

 	for_each_sg(sgl, sg, sg_len, i) {
 		struct idma64_hw_desc *hw = &desc->hw[i];

Signature · 1 line

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

Rosen Penev

author

2026-07-12 22:15 UTC #3 lore raw

Re: [PATCHv2 1/2] dmaengine: idma64: use kzalloc_flex

Quoted 5 lines · On Sun, Jul 12, 2026 at 3:11 PM <[email protected]> wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] If idma64_prep_slave_sg() is called with sg_len == 0, it eventually calls idma64_desc_fill() which accesses desc->hw[UINT_MAX], causing a kernel panic.

Fixed in the second patch. This is just sashiko being broken with v2 patches.

Quoted 54 lines

--
--- Patch [1]: [PATCHv2 1/2] dmaengine: idma64: use kzalloc_flex --- commit 54aec29a1bbe69497782adf915ba6169fd73e345
Author: Rosen Penev <[email protected]>
dmaengine: idma64: use kzalloc_flex
This commit refactors the idma64_desc and idma64 structures to use flexible array members. It simplifies allocations by using kzalloc_flex and adds __counted_by annotations for runtime bounds checking.
Quoted 4 lines
drivers/dma/idma64.c +0 -0
index 5fcd1befc92da..d914f50ec3096 100644
[ ... ]
drivers/dma/idma64.c static struct dma_async_tx_descriptor *idma64_prep_slave_sg( · line 297
Quoted 11 lines
drivers/dma/idma64.c +3 -1
@@ -307,10 +289,12 @@ static struct dma_async_tx_descriptor *idma64_prep_slave_sg(
      struct scatterlist *sg;
      unsigned int i;
 
-     desc = idma64_alloc_desc(sg_len);
+     desc = kzalloc_flex(*desc, hw, sg_len, GFP_NOWAIT);
      if (!desc)
              return NULL;
 
+     desc->ndesc = sg_len;
+
[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds access if idma64_prep_slave_sg() is called with sg_len == 0?
If sg_len is 0, desc->ndesc is set to 0 and the for_each_sg() loop is bypassed. The function then calls idma64_desc_fill() which does the following:
drivers/dma/idma64.c:idma64_desc_fill() {
    ...
    unsigned int i = desc->ndesc;
    struct idma64_hw_desc *hw = &desc->hw[i - 1];
    ...
}
Since i is 0, i - 1 underflows to UINT_MAX, causing an out-of-bounds access on the hw flex array. With the new __counted_by bounds check, this will likely trigger a panic immediately.
drivers/dma/idma64.c static struct dma_async_tx_descriptor *idma64_prep_slave_sg( · line 300
Quoted 3 lines
drivers/dma/idma64.c +0 -0
      for_each_sg(sgl, sg, sg_len, i) {
              struct idma64_hw_desc *hw = &desc->hw[i];
 
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Andy Shevchenko

reviewer

2026-07-13 11:57 UTC #4 lore raw

Re: [PATCHv2 1/2] dmaengine: idma64: use kzalloc_flex

In reply to Rosen Penev

Quoted 8 lines · Rosen Penev

Simplifies allocations by using a flexible array member in this struct.
Remove idma64_alloc_desc. It now offers no readability advantages in this single usage.
Add __counted_by to get extra runtime analysis.
Apply the exact same treatment to struct idma64_dma and devm_kzalloc.

...

drivers/dma/idma64.c static struct dma_async_tx_descriptor *idma64_prep_slave_sg( · line 296

Quoted 11 lines

static struct dma_async_tx_descriptor *idma64_prep_slave_sg(

drivers/dma/idma64.c +2 -1

 	struct scatterlist *sg;
 	unsigned int i;
 
-	desc = idma64_alloc_desc(sg_len);
+	desc = kzalloc_flex(*desc, hw, sg_len, GFP_NOWAIT);
 	if (!desc)
 		return NULL;
 
+	desc->ndesc = sg_len;

There are two places where this is updated. Are you sure the code become correct after this change? Perhaps idma64_desc_free() needs additional care?

drivers/dma/idma64.c static struct dma_async_tx_descriptor *idma64_prep_slave_sg( · line 314

Quoted 9 lines

drivers/dma/idma64.c +0 -1

 	for_each_sg(sgl, sg, sg_len, i) {
 		struct idma64_hw_desc *hw = &desc->hw[i];
 
 		hw->len = sg_dma_len(sg);
 	}
 
-	desc->ndesc = sg_len;
 	desc->direction = direction;
 	desc->status = DMA_IN_PROGRESS;

In case the above is okay to do, move all three up to keep this block of assignments together.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Rosen Penev

author

2026-07-12 22:00 UTC #5 lore raw

PATCH v2 2/2 dmaengine: idma64: use sg_nents_for_dma to respect hardware descriptor length limit

In reply to Rosen Penev

The iDMA 64-bit hardware has a 17-bit block transfer size field in the CTL_HI register (IDMA64C_CTLH_BLOCK_TS_MASK = 0x1ffff). When a scatterlist entry exceeds this limit, the driver would silently truncate the length, transferring fewer bytes than intended.

Use sg_nents_for_dma() to compute the number of hardware descriptors needed after splitting large SG entries into chunks that fit within the hardware limit. Split the loop to iterate over each chunk.

Assisted-by opencode:big-pickle

Signed-off-by Rosen Penev <[email protected]>

---
 drivers/dma/idma64.c | 44 ++++++++++++++++++++++++++++++--------------
 drivers/dma/idma64.h |  3 ++-
 2 files changed, 32 insertions(+), 15 deletions(-)

drivers/dma/idma64.c +30 ~~-14~~

index d914f50ec309..6954ec2cdeae 100644
@@ -287,27 +287,43 @@ static struct dma_async_tx_descriptor *idma64_prep_slave_sg(
 	struct idma64_chan *idma64c = to_idma64_chan(chan);
 	struct idma64_desc *desc;
 	struct scatterlist *sg;
-	unsigned int i;
+	unsigned int i, nents;
+	int ndesc;
 
-	desc = kzalloc_flex(*desc, hw, sg_len, GFP_NOWAIT);
+	ndesc = sg_nents_for_dma(sgl, sg_len, IDMA64C_CTLH_BLOCK_TS_MASK);
+	if (ndesc <= 0)
+		return NULL;
+
+	desc = kzalloc_flex(*desc, hw, ndesc, GFP_NOWAIT);
 	if (!desc)
 		return NULL;
 
-	desc->ndesc = sg_len;
+	desc->ndesc = ndesc;
 
+	nents = 0;
 	for_each_sg(sgl, sg, sg_len, i) {
-		struct idma64_hw_desc *hw = &desc->hw[i];
-
-		/* Allocate DMA capable memory for hardware descriptor */
-		hw->lli = dma_pool_alloc(idma64c->pool, GFP_NOWAIT, &hw->llp);
-		if (!hw->lli) {
-			desc->ndesc = i;
-			idma64_desc_free(idma64c, desc);
-			return NULL;
+		dma_addr_t addr = sg_dma_address(sg);
+		unsigned int len = sg_dma_len(sg);
+
+		while (len) {
+			struct idma64_hw_desc *hwdesc = &desc->hw[nents++];
+			unsigned int chunk = min(len, IDMA64C_CTLH_BLOCK_TS_MASK);
+
+			hwdesc->lli = dma_pool_alloc(idma64c->pool, GFP_NOWAIT,
+						     &hwdesc->llp);
+			if (!hwdesc->lli) {
+				/* nents was already incremented by ++ above */
+				desc->ndesc = nents - 1;
+				idma64_desc_free(idma64c, desc);
+				return NULL;
+			}
+
+			hwdesc->phys = addr;
+			hwdesc->len = chunk;
+
+			addr += chunk;
+			len -= chunk;
 		}
-
-		hw->phys = sg_dma_address(sg);
-		hw->len = sg_dma_len(sg);
 	}
 
 	desc->direction = direction;

drivers/dma/idma64.h +2 -1

index 1a67dbb24db5..297a91594b31 100644
@@ -8,6 +8,7 @@
 #ifndef __DMA_IDMA64_H__
 #define __DMA_IDMA64_H__
 
+#include <linux/bits.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/spinlock.h>

@@ -51,7 +52,7 @@
 #define IDMA64C_CTLL_LLP_S_EN		(1 << 28)	/* src block chain */
 
 /* Bitfields in CTL_HI */
-#define IDMA64C_CTLH_BLOCK_TS_MASK	((1 << 17) - 1)
+#define IDMA64C_CTLH_BLOCK_TS_MASK	GENMASK_U32(16, 0)
 #define IDMA64C_CTLH_BLOCK_TS(x)	((x) & IDMA64C_CTLH_BLOCK_TS_MASK)
 #define IDMA64C_CTLH_DONE		(1 << 17)

Signature · 1 line

2.55.0

Andy Shevchenko

reviewer

2026-07-13 12:13 UTC #6 lore raw

Re: [PATCHv2 2/2] dmaengine: idma64: use sg_nents_for_dma to respect hardware descriptor length limit

Quoted 8 lines · Rosen Penev

The iDMA 64-bit hardware has a 17-bit block transfer size field in the CTL_HI register (IDMA64C_CTLH_BLOCK_TS_MASK = 0x1ffff). When a scatterlist entry exceeds this limit, the driver would silently truncate the length, transferring fewer bytes than intended.
Use sg_nents_for_dma() to compute the number of hardware descriptors needed after splitting large SG entries into chunks that fit within the hardware limit. Split the loop to iterate over each chunk.

I appreciate the intention, but... I have issues with the implementation.

Quoted 6 lines

Assisted-by opencode:big-pickle
Signed-off-by Rosen Penev <[email protected]>
---
 drivers/dma/idma64.c | 44 ++++++++++++++++++++++++++++++--------------
 drivers/dma/idma64.h |  3 ++-
 2 files changed, 32 insertions(+), 15 deletions(-)

First of all, the statistics and code readability. Next is the requirement for drivers to do that. This should be done on the DMAengine core level for all, this is software resplit and we just need a driver agreement of getting a such to be done before handing over to the driver's callback.

OTOH, there is an API to get DMA maximum segment size (note, that your split is incorrect since the BLOCK_TS is in "bus width" units, it may be up to 4 or 8 bytes and it depends on the alignment: so, in this form this patch is no go). The consumer drivers should actually call it before preparing SG list to make sure that resplit is not needed and the SG list is compatible with what HW capable of.

See dma_set_max_seg_size() and dma_get_max_seg_size().

...

Quoted 7 lines

#ifndef __DMA_IDMA64_H__
#define __DMA_IDMA64_H__
+#include <linux/bits.h>
#include <linux/device.h>
#include <linux/io.h>
#include <linux/spinlock.h>

Yeah, the inclusions should be revisit as many changes happened after the driver introduction in the area of how we split headers.

...

drivers/dma/idma64.h line 55

Quoted 3 lines

/* Bitfields in CTL_HI */

drivers/dma/idma64.h +1 -1

-#define IDMA64C_CTLH_BLOCK_TS_MASK	((1 << 17) - 1)
+#define IDMA64C_CTLH_BLOCK_TS_MASK	GENMASK_U32(16, 0)

Why? I think this becomes inconsistent. If you want to switch to bits.h, make it in a separate patch for all eligible definitions.

Signature · 2 lines

With Best Regards,
Andy Shevchenko