Re: [PATCH v3] dma-buf: Split sgl by largest page-aligned chunk

[sashiko-bot@kernel.org]

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

New issues:
- [High] Integer Overflow / NULL Pointer Dereference in sg mapping calculation

Pre-existing issues:
- [High] 32-bit Integer Overflow causing incomplete DMA mappings
- [High] DMA API Violation (Mismatched Map/Unmap Size)
- [High] IOVA Resource Leak on Error Paths
--

--- Patch [1]: [PATCH v3] dma-buf: Split sgl by largest page-aligned chunk ---
commit 6c6b736bcfe57779f8dd4452cee1c6738c03f76d
Author: David Hu <xuehaohu@google.com>

dma-buf: Split sgl by largest page-aligned chunk

This commit updates scatterlist chunking in dma-buf to use 
ALIGN_DOWN(UINT_MAX, PAGE_SIZE) instead of UINT_MAX. This ensures that 
scatterlist DMA addresses and lengths remain strictly page-aligned, which 
avoids truncation on hardware engines and fixes a fallback issue in 
ib_umem_find_best_pgsz().

Closes: https://lore.kernel.org/all/20260609165431.778061F00893@smtp.kernel.org/
Fixes: 3aa31a8bb11e ("dma-buf: provide phys_vec to scatter-gather mapping routine")

> diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
> index 794acff2546a3..50ded9daf5fb3 100644
> --- a/drivers/dma-buf/dma-buf-mapping.c
> +++ b/drivers/dma-buf/dma-buf-mapping.c
[ ... ]
> @@ -24,8 +25,10 @@ static struct scatterlist *fill_sg_entry(struct scatterlist *sgl, size_t length,
>  		 * does not require the CPU list for mapping or unmapping.
>  		 */
>  		sg_set_page(sgl, NULL, 0, 0);

[Severity: High]
Does this new while loop correctly handle an undersized scatterlist?

If calc_sg_nents() returns 0 due to the 32-bit overflow mentioned below,
this while loop still processes the full length. It will exhaust the
SGL entries, receive a NULL from sg_next(), and dereference it during
the next call to sg_set_page().

> -		sg_dma_address(sgl) = addr + (dma_addr_t)i * UINT_MAX;
> +		sg_dma_address(sgl) = addr;
>  		sg_dma_len(sgl) = len;
> +		addr += len;
> +		/* Unconditionally advance. On last segment, this becomes NULL */
>  		sgl = sg_next(sgl);
>  	}
>  
> @@ -41,14 +44,14 @@ static unsigned int calc_sg_nents(struct dma_iova_state *state,
>  
>  	if (!state || !dma_use_iova(state)) {
>  		for (i = 0; i < nr_ranges; i++)
> -			nents += DIV_ROUND_UP(phys_vec[i].len, UINT_MAX);
> +			nents += DIV_ROUND_UP(phys_vec[i].len, MAX_SG_ENT_SZ);
                           ^^^^

[Severity: High]
Can this DIV_ROUND_UP() overflow on 32-bit platforms?

MAX_SG_ENT_SZ is ALIGN_DOWN(0xFFFFFFFF, PAGE_SIZE), which is 0xFFFFF000.
The DIV_ROUND_UP(len, MAX_SG_ENT_SZ) expands to
(len + 0xFFFFF000 - 1) / 0xFFFFF000. If len is 4097 or greater, this
addition overflows a 32-bit integer and wraps around, yielding 0. This
allocates 0 scatterlist entries for that chunk.

>  	} else {
>  		/*
>  		 * In IOVA case, there is only one SG entry which spans
>  		 * for whole IOVA address space, but we need to make sure
>  		 * that it fits sg->length, maybe we need more.
>  		 */
> -		nents = DIV_ROUND_UP(size, UINT_MAX);
> +		nents = DIV_ROUND_UP(size, MAX_SG_ENT_SZ);

[Severity: High]
Similarly, can this calculation overflow in the IOVA case when processing
sizes >= 4097 bytes on a 32-bit platform?

>  	}
>  
>  	return nents;

[ ... ]

While reviewing this code, I noticed a few pre-existing issues in the same
file that you might want to address:

[Severity: High]
This isn't a bug introduced by this patch, but does mapped_len silently wrap
around on 64-bit platforms?

drivers/dma-buf/dma-buf-mapping.c:dma_buf_phys_vec_to_sgt() {
    unsigned int nents, mapped_len = 0;

    [ ... ]

    for (i = 0; i < nr_ranges; i++) {

        [ ... ]

        mapped_len += phys_vec[i].len;
    }
}

Because mapped_len is a 32-bit unsigned int and phys_vec[i].len is a 64-bit
size_t, calculating a total mapping size larger than 4GB overflows mapped_len.


[Severity: High]
This isn't a bug introduced by this patch, but does the error path leak the
IOVA allocation?

drivers/dma-buf/dma-buf-mapping.c:dma_buf_phys_vec_to_sgt() {

    [ ... ]

    dma_iova_try_alloc(attach->dev, dma->state, 0, size);

    [ ... ]

    ret = sg_alloc_table(&dma->sgt, nents, GFP_KERNEL | __GFP_ZERO);
    if (ret)
        goto err_free_state;

    [ ... ]

err_unmap_dma:
    if (!i || !dma->state) {
        ; /* Do nothing */
    } else if (dma_use_iova(dma->state)) {
        dma_iova_destroy(attach->dev, dma->state, mapped_len, dir,
                 DMA_ATTR_MMIO);

    [ ... ]

}

If sg_alloc_table() fails, the code branches to err_free_state, returning
without calling dma_iova_destroy(). Additionally, if mapping the first range
fails (i == 0), the err_unmap_dma path executes the empty if-block and skips
cleanup.


[Severity: High]
This isn't a bug introduced by this patch, but does this unmap sequence
violate the requirement to match map and unmap sizes?

drivers/dma-buf/dma-buf-mapping.c:dma_buf_phys_vec_to_sgt() {

    [ ... ]

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

    [ ... ]

}

drivers/dma-buf/dma-buf-mapping.c:dma_buf_free_sgt() {

    [ ... ]

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

    [ ... ]

}

The physical memory regions are mapped as single, contiguous blocks using
phys_vec[i].len, but the teardown path unmaps them in smaller chunks
iterating over the split scatterlist.

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