Re: [PATCH v2] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg()

[sashiko-bot@kernel.org]

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