Re: [PATCH v8 1/2] dma-buf: Fix silent overflow for phys vec to sgt

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Incorrect offset parameter passed to dma_iova_link() causes all physical vectors to overwrite each other at the same IOVA address.
- [Critical] Partial unmapping of DMA mappings via segmented scatterlist entries violates the DMA API.
- [High] DIV_ROUND_UP macro wraps around on 32-bit platforms, causing calc_sg_nents to evaluate to 0 and failing the mapping.
- [High] Setting sgt.orig_nents = 0 breaks sg_free_table, causing a silent memory leak of the scatterlist.
- [High] Error path unmaps all uninitialized scatterlist entries due to an incorrect loop iterator macro shadowing the index.
- [High] IOVA space allocated by dma_iova_try_alloc() is leaked on multiple error paths.
--

commit 3ea75e22b2b7ea7287fcbeb0fa39a0b99a722743
Author: David Hu <xuehaohu@google.com>

dma-buf: Fix silent overflow for phys vec to sgt
    
This commit changes mapped_len to a 64-bit size_t to prevent silent wrap-around
during length accumulation for MMIO greater than 4G. It also introduces
check_add_overflow() in calc_sg_nents() for safer scatterlist entry
accumulation.

> diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
> index 794acff2546a3..80f6ab2f4809d 100644
> --- a/drivers/dma-buf/dma-buf-mapping.c
> +++ b/drivers/dma-buf/dma-buf-mapping.c

[ ... ]

> @@ -40,8 +41,12 @@ static unsigned int calc_sg_nents(struct dma_iova_state *state,
>  	size_t i;
>  
>  	if (!state || !dma_use_iova(state)) {
> -		for (i = 0; i < nr_ranges; i++)
> -			nents += DIV_ROUND_UP(phys_vec[i].len, UINT_MAX);
> +		for (i = 0; i < nr_ranges; i++) {
> +			unsigned int added = DIV_ROUND_UP(phys_vec[i].len, UINT_MAX);

[Severity: High]
This is a pre-existing issue, but could DIV_ROUND_UP wrap around on 32-bit
platforms here?

Since size_t is 32-bit on those architectures, phys_vec[i].len + UINT_MAX - 1
wraps around the maximum 32-bit value. For any chunk length larger than a page,
added will evaluate to 0, completely failing the DMA mapping.

> +
> +			if (check_add_overflow(nents, added, &nents))
> +				return 0;
> +		}

[ ... ]

> @@ -133,6 +139,8 @@ struct sg_table *dma_buf_phys_vec_to_sgt(struct dma_buf_attachment *attach,
>  	}
>  
>  	nents = calc_sg_nents(dma->state, phys_vec, nr_ranges, size);
> +
> +	/* sg_alloc_table will cleanly fail and return -EINVAL if nents == 0 */
>  	ret = sg_alloc_table(&dma->sgt, nents, GFP_KERNEL | __GFP_ZERO);
>  	if (ret)
>  		goto err_free_state;

[Severity: High]
This is a pre-existing issue, but is it possible this jump leaks the allocated
IOVA space?

If sg_alloc_table fails, the jump to err_free_state frees dma->state, but
doesn't call dma_iova_destroy().

Also, since this patch updates this function, I noticed a few other
pre-existing issues further down in dma_buf_phys_vec_to_sgt():

[Severity: Critical]
This is a pre-existing issue, but will the hardcoded zero offset overwrite
mappings at the start of the IOVA space?

In the loop later in this function:

    } else if (dma_use_iova(dma->state)) {
        ret = dma_iova_link(attach->dev, dma->state,
                            phys_vec[i].paddr, 0,
                            phys_vec[i].len, dir,
                            DMA_ATTR_MMIO);

Passing the literal 0 as the offset for every vector causes all physical
vectors to be mapped to the exact same starting IOVA address. Should this use
the accumulated mapped_len instead?

[Severity: Critical]
This is a pre-existing issue, but does this chunk-by-chunk unmap violate the
DMA API?

The code maps contiguous physical memory as a single chunk:

    addr = dma_map_phys(attach->dev, phys_vec[i].paddr,
                        phys_vec[i].len, dir, DMA_ATTR_MMIO);

But then in fill_sg_entry(), it splits this mapping into multiple scatterlist
entries. During teardown in dma_buf_free_sgt() and err_unmap_dma, it unmaps
these segmented pieces:

    for_each_sgtable_dma_sg(&dma->sgt, sgl, i)
        dma_unmap_phys(attach->dev, sg_dma_address(sgl),
                       sg_dma_len(sgl), dir, DMA_ATTR_MMIO);

Can this partial unmapping of a single mapped region corrupt IOMMU state
or cause undefined behavior?

[Severity: High]
This is a pre-existing issue, but does the error path iterate over
uninitialized scatterlist entries and leak IOVA space?

Looking at err_unmap_dma:

err_unmap_dma:
    if (!i || !dma->state) {
        ; /* Do nothing */

If an error occurs on the first loop iteration, the !i check safely returns but
skips dma_iova_destroy(), leaking the IOVA space here as well.

Furthermore, the loop down in that error path shadows the index variable:

    } else {
        for_each_sgtable_dma_sg(&dma->sgt, sgl, i)
            dma_unmap_phys(...)
    }

Because for_each_sgtable_dma_sg uses i as its internal iterator (iterating
up to dma->sgt.nents), it resets i to 0 and processes all entries, including
uninitialized zero-elements, rather than stopping at the count of successfully
mapped segments.

[Severity: High]
This is a pre-existing issue, but would clearing orig_nents cause a silent
memory leak?

At the end of dma_buf_phys_vec_to_sgt():

    /*
     * No CPU list included — set orig_nents = 0 so others can detect
     * this via SG table (use nents only).
     */
    dma->sgt.orig_nents = 0;

When dma_buf_free_sgt() eventually calls sg_free_table(), the underlying
__sg_free_table() uses table->orig_nents to determine how many chunks to free.
Since it is 0, won't the free loop terminate immediately and silently leak the
entire scatterlist memory allocation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901170849.4052816-1-dhu@x6u.co?part=1