Re: [PATCH v2] dma-buf: Split sgl into page-aligned 2G chunks

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Arithmetic overflow in `calc_sg_nents` on 32-bit systems causes an under-allocation of scatterlist entries, leading to a NULL pointer dereference in `fill_sg_entry`.

Pre-existing issues:
- [High] Incorrect DMA unmapping of split scatterlist chunks and unpopulated entries violates the DMA API and corrupts IOMMU state.
--

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

dma-buf: Split sgl into page-aligned 2G chunks

Currently, fill_sg_entry() splits the scatterlist using UINT_MAX. This patch
changes the split to 2G chunks to satisfy page alignment and natural hardware
boundaries for DMA engines.

Link: https://lore.kernel.org/all/20260609165431.778061F00893@smtp.kernel.org/

> diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
> index 794acff2546a3..2d88e08c5ebf9 100644
> --- a/drivers/dma-buf/dma-buf-mapping.c
> +++ b/drivers/dma-buf/dma-buf-mapping.c
> @@ -5,16 +5,17 @@
>   */
>  #include <linux/dma-buf-mapping.h>
>  #include <linux/dma-resv.h>
> +#include <linux/sizes.h>
> +
> +#define MAX_SG_ENT_SZ ((size_t)SZ_2G)
>  
>  static struct scatterlist *fill_sg_entry(struct scatterlist *sgl, size_t length,
>  					 dma_addr_t addr)
>  {
> -	unsigned int len, nents;
> -	int i;
> +	size_t len;
>  
> -	nents = DIV_ROUND_UP(length, UINT_MAX);
> -	for (i = 0; i < nents; i++) {
> -		len = min_t(size_t, length, UINT_MAX);
> +	while (length) {
> +		len = min(length, MAX_SG_ENT_SZ);
>  		length -= len;
>  		/*
>  		 * DMABUF abuses scatterlist to create a scatterlist
> @@ -24,11 +25,12 @@ 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: Critical]
Will this lead to a NULL pointer dereference on 32-bit systems?

If phys_vec[i].len is greater than 2GB (e.g. 0x80000001), the DIV_ROUND_UP
macro in calc_sg_nents() will overflow because size_t is 32-bit and
MAX_SG_ENT_SZ is 0x80000000:

    (0x80000001 + 0x80000000 - 1) / 0x80000000 == 0

This means no SGL entries are calculated or allocated for this region. However,
the while loop in fill_sg_entry() will still iterate to process the remaining
length. It will eventually advance past the end of the undersized scatterlist
table via:

    sgl = sg_next(sgl);

which becomes NULL, and then dereference it on the next iteration via
sg_set_page(sgl, ...).

> -		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);
>  	}
> -
>  	return sgl;
>  }
>  
> @@ -41,14 +43,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);
>  	} 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);
>  	}
>  
>  	return nents;

[Severity: High]
This isn't a bug introduced by this patch, but does iterating over the SGL to
unmap chunked entries violate the DMA API?

In dma_buf_phys_vec_to_sgt(), dma_map_phys() maps the full, contiguous original
length of phys_vec[i].len. However, fill_sg_entry() splits this mapped region
into smaller chunks.

When unmapping, both the error path in dma_buf_phys_vec_to_sgt() and the
teardown in dma_buf_free_sgt() iterate over the split SGL entries:

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);
    ...
}

Does the DMA API permit unmapping partial chunks instead of the exact length
and address originally mapped?

Additionally, in the error path of dma_buf_phys_vec_to_sgt():

drivers/dma-buf/dma-buf-mapping.c:dma_buf_phys_vec_to_sgt() {
    ...
err_unmap_dma:
	if (!i || !dma->state) {
		; /* Do nothing */
	...
	} else {
		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);
	}
    ...
}

Is it safe to reuse the outer loop variable i here? Since i was tracking the
successfully mapped count, using it in for_each_sgtable_dma_sg() clobbers it
and forces iteration up to the sgt->nents capacity, which could unmap
unpopulated zero-initialized entries or page 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623015459.1153884-1-xuehaohu@google.com?part=1