* [PATCH] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg
@ 2026-09-09 3:38 Baineng Shou
2026-09-09 3:50 ` sashiko-bot
2026-09-09 21:01 ` Frank Li
0 siblings, 2 replies; 6+ messages in thread
From: Baineng Shou @ 2026-09-09 3:38 UTC (permalink / raw)
To: vkoul; +Cc: Frank.Li, dmaengine, linux-kernel, Baineng Shou
In mmp_pdma_prep_slave_sg(), for_each_sg() iterates the scatterlist
putting each entry into 'sg', but the entry length is read from 'sgl'
(the list head) instead of 'sg' (the current entry):
for_each_sg(sgl, sg, sg_len, i) {
addr = sg_dma_address(sg);
avail = sg_dma_len(sgl); /* should be 'sg' */
Consequently 'avail' is always the length of the first entry. For
multi-sg lists this causes out-of-bounds reads when a later entry is
shorter than the first, and silent data loss when it is longer.
Single-sg or uniformly-sized lists happen to mask the issue.
Fixes: c8acd6aa6bed3 ("dmaengine: mmp-pdma support")
Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
---
drivers/dma/mmp_pdma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index 386e85cd4882..e90fd2023af7 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -713,7 +713,7 @@ mmp_pdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
for_each_sg(sgl, sg, sg_len, i) {
addr = sg_dma_address(sg);
- avail = sg_dma_len(sgl);
+ avail = sg_dma_len(sg);
do {
len = min_t(size_t, avail, PDMA_MAX_DESC_BYTES);
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg
2026-09-09 3:38 [PATCH] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg Baineng Shou
@ 2026-09-09 3:50 ` sashiko-bot
2026-09-09 21:01 ` Frank Li
1 sibling, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-09-09 3:50 UTC (permalink / raw)
To: Baineng Shou; +Cc: vkoul, dmaengine, Frank.Li
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-After-Free during descriptor list cleanup on error paths.
- [High] Tasklets are not killed during device removal, leading to a potential Use-After-Free.
- [High] Stale state and missing initialization for `chan->dir` and `chan->dcmd` breaking memory-to-memory DMA.
- [High] Unprotected overwrite of channel-wide `byte_align` flag by subsequent prep calls.
--
commit 67c707d8b7f67369582be4fc75b9289a8abcab3a
Author: Baineng Shou <shoubaineng@gmail.com>
dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg
In mmp_pdma_prep_slave_sg(), for_each_sg() iterates the scatterlist
putting each entry into 'sg', but the entry length is read from 'sgl'
(the list head) instead of 'sg' (the current entry):
[ ... ]
Single-sg or uniformly-sized lists happen to mask the issue.
> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index 386e85cd4882a..e90fd2023af71 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c
> @@ -713,7 +713,7 @@ mmp_pdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
>
> for_each_sg(sgl, sg, sg_len, i) {
> addr = sg_dma_address(sg);
> - avail = sg_dma_len(sgl);
> + avail = sg_dma_len(sg);
>
> do {
> len = min_t(size_t, avail, PDMA_MAX_DESC_BYTES);
[Severity: High]
This isn't a regression introduced by this patch, but this lockless
modification of chan->byte_align appears to silently overwrite the hardware
alignment requirement for any earlier descriptors that are still pending or
running:
drivers/dma/mmp_pdma.c:mmp_pdma_prep_slave_sg() {
...
do {
len = min_t(size_t, avail, PDMA_MAX_DESC_BYTES);
if (addr & 0x7)
chan->byte_align = true;
...
}
If a client prepares multiple scatterlists on the same channel before
submitting them, does the last prepared scatterlist overwrite
chan->byte_align for the entire channel?
When earlier transactions are eventually started via enable_chan(), they
are submitted to hardware with the overwritten alignment configuration:
drivers/dma/mmp_pdma.c:enable_chan() {
...
if (phy->vchan->byte_align)
dalgn |= 1 << phy->idx;
...
}
Could this lead to hardware misbehavior or data corruption due to incorrect
alignment configuration?
[Severity: High]
This is a pre-existing regression, but I have a question about the error
handling path further down in mmp_pdma_prep_slave_sg():
drivers/dma/mmp_pdma.c:mmp_pdma_prep_slave_sg() {
...
fail:
if (first)
mmp_pdma_free_desc_list(chan, &first->tx_list);
return NULL;
}
The cleanup sequence explicitly passes &first->tx_list, which is embedded
in the descriptor to be freed.
drivers/dma/mmp_pdma.c:mmp_pdma_free_desc_list() {
...
list_for_each_entry_safe(desc, _desc, list, node) {
list_del(&desc->node);
dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
}
...
}
The list iteration macro safely caches the next element, but it reads from
and writes to the list head during the update and list_del() steps.
If the list head (&first->tx_list) is embedded in a freed element (because
the 'first' descriptor is freed during the first iteration), wouldn't this
result in a use-after-free when subsequent list manipulations or the loop's
next-entry calculation dereference the freed memory?
[Severity: High]
This is a pre-existing regression, but there might be an initialization
problem in mmp_pdma_prep_memcpy() when reusing channels:
drivers/dma/mmp_pdma.c:mmp_pdma_prep_memcpy() {
...
if (!chan->dir) {
chan->dir = DMA_MEM_TO_MEM;
chan->dcmd = DCMD_INCTRGADDR | DCMD_INCSRCADDR;
chan->dcmd |= DCMD_BURST32;
}
...
}
Does the !chan->dir condition fail to clear stale state from a previous slave
DMA session?
When a channel allocated from the shared pool is used for slave DMA (e.g.,
DMA_MEM_TO_DEV), it modifies chan->dir and chan->dcmd. When the channel is
freed via mmp_pdma_free_chan_resources(), these fields are not cleared.
If the channel is later requested for dma_memcpy, mmp_pdma_prep_memcpy() will
use the stale state. Could this incorrectly omit DCMD_INCTRGADDR (leaving the
target address static) and apply the wrong address increment logic, resulting
in corrupted memory writes?
[Severity: High]
This is a pre-existing regression, but I noticed a potential teardown
problem in the device removal path:
drivers/dma/mmp_pdma.c:mmp_pdma_remove() {
...
dma_async_device_unregister(&pdev->device);
}
The device removal teardown is missing necessary tasklet_kill() calls for
all initialized channels before their memory backing is freed by devres.
Since chan is managed by devm_kzalloc(), it is automatically freed after
mmp_pdma_remove() completes.
If a tasklet was scheduled right before free_irq() but hasn't executed,
will it run after removal completes and access the freed chan structure,
causing a use-after-free?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260909033841.1395793-1-shoubaineng@gmail.com?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg
2026-09-09 3:38 [PATCH] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg Baineng Shou
2026-09-09 3:50 ` sashiko-bot
@ 2026-09-09 21:01 ` Frank Li
2026-09-10 2:16 ` [PATCH v2] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg() Baineng Shou
1 sibling, 1 reply; 6+ messages in thread
From: Frank Li @ 2026-09-09 21:01 UTC (permalink / raw)
To: Baineng Shou; +Cc: vkoul, Frank.Li, dmaengine, linux-kernel
On Wed, Sep 09, 2026 at 11:38:41AM +0800, Baineng Shou wrote:
Need () for funciton mmp_pdma_prep_slave_sg()
Frank
>
> In mmp_pdma_prep_slave_sg(), for_each_sg() iterates the scatterlist
> putting each entry into 'sg', but the entry length is read from 'sgl'
> (the list head) instead of 'sg' (the current entry):
>
> for_each_sg(sgl, sg, sg_len, i) {
> addr = sg_dma_address(sg);
> avail = sg_dma_len(sgl); /* should be 'sg' */
>
> Consequently 'avail' is always the length of the first entry. For
> multi-sg lists this causes out-of-bounds reads when a later entry is
> shorter than the first, and silent data loss when it is longer.
> Single-sg or uniformly-sized lists happen to mask the issue.
>
> Fixes: c8acd6aa6bed3 ("dmaengine: mmp-pdma support")
> Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
> ---
> drivers/dma/mmp_pdma.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index 386e85cd4882..e90fd2023af7 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c
> @@ -713,7 +713,7 @@ mmp_pdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
>
> for_each_sg(sgl, sg, sg_len, i) {
> addr = sg_dma_address(sg);
> - avail = sg_dma_len(sgl);
> + avail = sg_dma_len(sg);
>
> do {
> len = min_t(size_t, avail, PDMA_MAX_DESC_BYTES);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg()
2026-09-09 21:01 ` Frank Li
@ 2026-09-10 2:16 ` Baineng Shou
2026-09-10 2:27 ` sashiko-bot
2026-09-10 16:38 ` Frank Li
0 siblings, 2 replies; 6+ messages in thread
From: Baineng Shou @ 2026-09-10 2:16 UTC (permalink / raw)
To: vkoul; +Cc: Frank.Li, dmaengine, linux-kernel, Baineng Shou
In mmp_pdma_prep_slave_sg(), for_each_sg() iterates the scatterlist
putting each entry into 'sg', but the entry length is read from 'sgl'
(the list head) instead of 'sg' (the current entry):
for_each_sg(sgl, sg, sg_len, i) {
addr = sg_dma_address(sg);
avail = sg_dma_len(sgl); /* should be 'sg' */
Consequently 'avail' is always the length of the first entry. For
multi-sg lists this causes out-of-bounds reads when a later entry is
shorter than the first, and silent data loss when it is longer.
Single-sg or uniformly-sized lists happen to mask the issue.
Fixes: c8acd6aa6bed3 ("dmaengine: mmp-pdma support")
Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
---
Changes in v2:
- Add () to the function name in the subject, per Frank Li's review.
drivers/dma/mmp_pdma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index 386e85cd4882..e90fd2023af7 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -713,7 +713,7 @@ mmp_pdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
for_each_sg(sgl, sg, sg_len, i) {
addr = sg_dma_address(sg);
- avail = sg_dma_len(sgl);
+ avail = sg_dma_len(sg);
do {
len = min_t(size_t, avail, PDMA_MAX_DESC_BYTES);
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg()
2026-09-10 2:16 ` [PATCH v2] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg() Baineng Shou
@ 2026-09-10 2:27 ` sashiko-bot
2026-09-10 16:38 ` Frank Li
1 sibling, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-09-10 2:27 UTC (permalink / raw)
To: Baineng Shou; +Cc: vkoul, Frank.Li, dmaengine
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-after-free of list head in error path when freeing descriptor chain because the list head is embedded in the first element being freed.
--
commit 5f1b06ea0ed695c8b6d14358fdfb48649d799152
Author: Baineng Shou <shoubaineng@gmail.com>
dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg()
This commit fixes a bug in mmp_pdma_prep_slave_sg() where for_each_sg()
read the entry length from the list head instead of the current entry.
This led to out-of-bounds reads or silent data loss when handling
multi-sg lists of varying entry lengths.
> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index 386e85cd4882a..e90fd2023af71 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c
> @@ -713,7 +713,7 @@ mmp_pdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
>
> for_each_sg(sgl, sg, sg_len, i) {
> addr = sg_dma_address(sg);
> - avail = sg_dma_len(sgl);
> + avail = sg_dma_len(sg);
>
> do {
> len = min_t(size_t, avail, PDMA_MAX_DESC_BYTES);
[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
if a subsequent descriptor allocation fails further down in this function.
If a subsequent allocation fails, the error path is taken:
drivers/dma/mmp_pdma.c:mmp_pdma_prep_slave_sg() {
...
fail:
if (first)
mmp_pdma_free_desc_list(chan, &first->tx_list);
return NULL;
}
Inside mmp_pdma_free_desc_list(), list_for_each_entry_safe() iterates
over the list and frees each descriptor:
drivers/dma/mmp_pdma.c:mmp_pdma_free_desc_list() {
...
list_for_each_entry_safe(desc, _desc, list, node) {
list_del(&desc->node);
dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
}
}
Because the list head (&first->tx_list) is embedded within the first
descriptor, it is freed during the first iteration.
If there are multiple elements in the list, the second iteration calls
list_del(&desc->node), which modifies pointers that point back to the
now-freed list head.
Does this result in a use-after-free write into the freed descriptor?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910021652.1296640-1-shoubaineng@gmail.com?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg()
2026-09-10 2:16 ` [PATCH v2] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg() Baineng Shou
2026-09-10 2:27 ` sashiko-bot
@ 2026-09-10 16:38 ` Frank Li
1 sibling, 0 replies; 6+ messages in thread
From: Frank Li @ 2026-09-10 16:38 UTC (permalink / raw)
To: Baineng Shou; +Cc: vkoul, Frank.Li, dmaengine, linux-kernel
On Thu, Sep 10, 2026 at 10:16:52AM +0800, Baineng Shou wrote:
> [You don't often get email from shoubaineng@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> In mmp_pdma_prep_slave_sg(), for_each_sg() iterates the scatterlist
> putting each entry into 'sg', but the entry length is read from 'sgl'
> (the list head) instead of 'sg' (the current entry):
>
> for_each_sg(sgl, sg, sg_len, i) {
> addr = sg_dma_address(sg);
> avail = sg_dma_len(sgl); /* should be 'sg' */
>
> Consequently 'avail' is always the length of the first entry. For
> multi-sg lists this causes out-of-bounds reads when a later entry is
> shorter than the first, and silent data loss when it is longer.
> Single-sg or uniformly-sized lists happen to mask the issue.
>
> Fixes: c8acd6aa6bed3 ("dmaengine: mmp-pdma support")
> Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
> ---
Don't post new v2 patch to old email v1 thread.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Changes in v2:
> - Add () to the function name in the subject, per Frank Li's review.
>
> drivers/dma/mmp_pdma.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index 386e85cd4882..e90fd2023af7 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c
> @@ -713,7 +713,7 @@ mmp_pdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
>
> for_each_sg(sgl, sg, sg_len, i) {
> addr = sg_dma_address(sg);
> - avail = sg_dma_len(sgl);
> + avail = sg_dma_len(sg);
>
> do {
> len = min_t(size_t, avail, PDMA_MAX_DESC_BYTES);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-10 16:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-09 3:38 [PATCH] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg Baineng Shou
2026-09-09 3:50 ` sashiko-bot
2026-09-09 21:01 ` Frank Li
2026-09-10 2:16 ` [PATCH v2] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg() Baineng Shou
2026-09-10 2:27 ` sashiko-bot
2026-09-10 16:38 ` Frank Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).