Re: [PATCH v5] dma-buf: Fix silent overflow for phys vec to sgt

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The error path `err_unmap_dma` overwrites the failure loop index `i` and unmaps uninitialized scatterlist entries.
- [High] IOVA space allocated by `dma_iova_try_alloc()` is persistently leaked on multiple error paths.
- [High] DIV_ROUND_UP(length, UINT_MAX) silently evaluates to 0 on 32-bit architectures for any length >= 2, completely breaking mappings.
- [High] The error unmap path illegally calls `dma_unmap_phys` on split scatterlist chunks rather than the original contiguous mapped regions.
- [High] dma_iova_link() is repeatedly called with a hardcoded offset of 0, causing all physical ranges to be mapped overlapping at the start of the IOVA space.
- [High] Setting `orig_nents = 0` fundamentally breaks `sg_free_table()`, permanently leaking the scatterlist array.
- [High] Mismatch between accumulated `mapped_len` and provided `size` leads to SG table overflow and NULL pointer dereference.
- [High] Splitting physical ranges exactly at `UINT_MAX` generates unaligned intermediate DMA segments.
--

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

dma-buf: Fix silent overflow for phys vec to sgt

This commit changes mapped_len and nents to size_t to prevent silent 
wrap-around when accumulating lengths larger than 4G. It ensures sizes
are correctly calculated and guards against UINT_MAX overflows.

> diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
> index 794acff2546a3..607b7998463d0 100644
> --- a/drivers/dma-buf/dma-buf-mapping.c
> +++ b/drivers/dma-buf/dma-buf-mapping.c
> @@ -10,7 +10,7 @@ static struct scatterlist *fill_sg_entry(struct scatterlist *sgl, size_t length,
>  					 dma_addr_t addr)
>  {
>  	unsigned int len, nents;
> -	int i;
> +	unsigned int i;
>  
>  	nents = DIV_ROUND_UP(length, UINT_MAX);

[Severity: High]
This is a pre-existing issue, but does DIV_ROUND_UP evaluate to 0 on 32-bit
architectures?

On 32-bit systems, size_t is 32-bit. The macro DIV_ROUND_UP(length, UINT_MAX)
expands to (length + 0xFFFFFFFF - 1) / 0xFFFFFFFF. For any length >= 2, the
addition overflows the 32-bit limit, wrapping around to a small value.
Dividing this wrapped value by 0xFFFFFFFF yields 0.

Could this evaluate nents to 0 and skip the loop entirely?

>  	for (i = 0; i < nents; i++) {
>  		len = min_t(size_t, length, UINT_MAX);

[Severity: High]
This isn't a bug introduced by this patch, but does splitting physical ranges
exactly at UINT_MAX generate unaligned intermediate DMA segments?

When a physical range exceeds UINT_MAX (4GB - 1 bytes), this loop splits it
into multiple scatterlist entries. Because UINT_MAX is an odd number
(0xFFFFFFFF), the first segment receives an unaligned length, and the next
segment begins at an unaligned address (addr + 0xFFFFFFFF).

Since hardware DMA controllers often require segment lengths and addresses to
be properly aligned, could submitting an odd-aligned DMA segment trigger
hardware DMA faults?

[ ... ]
> @@ -36,7 +36,7 @@ static unsigned int calc_sg_nents(struct dma_iova_state *state,
>  				  struct phys_vec *phys_vec, size_t nr_ranges,
>  				  size_t size)
>  {
> -	unsigned int nents = 0;
> +	size_t nents = 0;
>  	size_t i;
>  
>  	if (!state || !dma_use_iova(state)) {
> @@ -51,6 +51,9 @@ static unsigned int calc_sg_nents(struct dma_iova_state *state,
>  		nents = DIV_ROUND_UP(size, UINT_MAX);

[Severity: High]
This is a pre-existing issue, but on 32-bit architectures, will this also wrap
around to 0 for any size >= 2 due to the DIV_ROUND_UP macro?

This would evaluate nents to 0, which immediately fails the mapping with
-EINVAL in the new patch code, potentially breaking DMA mappings for buffers
>= 2 bytes on 32-bit platforms.

>  	}
>  
> +	if (nents > UINT_MAX)
> +		return 0;
> +
>  	return nents;
>  }
[ ... ]
> @@ -133,6 +137,11 @@ 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);
> +	if (!nents) {
> +		ret = -EINVAL;
> +		goto err_free_state;

[Severity: High]
This isn't a bug introduced by this patch, but does jumping to
err_free_state leak the IOVA space?

When using PCI_P2PDMA_MAP_THRU_HOST_BRIDGE, an IOVA region is
pre-allocated. The err_free_state path frees dma->state but never calls
dma_iova_destroy().

Could this persistently leak the IOVA space allocated by
dma_iova_try_alloc()?

> +	}
> +
>  	ret = sg_alloc_table(&dma->sgt, nents, GFP_KERNEL | __GFP_ZERO);
>  	if (ret)
>  		goto err_free_state;
>
>  	sgl = dma->sgt.sgl;
>  
>  	for (i = 0; i < nr_ranges; i++) {
>  		if (!dma->state) {
>  			addr = pci_p2pdma_bus_addr_map(provider,
>  						       phys_vec[i].paddr);
>  		} else if (dma_use_iova(dma->state)) {
>  			ret = dma_iova_link(attach->dev, dma->state,
>  					    phys_vec[i].paddr, 0,

[Severity: High]
This is a pre-existing issue, but does passing a hardcoded 0 for the offset
cause all physical ranges to overlap?

Inside the mapping loop, physical ranges are linked into the IOVA space via
dma_iova_link(). The fourth parameter expects the offset within the IOVA space.
Passing a hardcoded 0 instead of the accumulated mapped_len causes every
subsequent physical range to overwrite the exact same starting region.

Could this corrupt the DMA mapping arrangement and break DMA isolation?

>  					    phys_vec[i].len, dir,
>  					    DMA_ATTR_MMIO);
[ ... ]
>  	if (dma->state && dma_use_iova(dma->state)) {
>  		WARN_ON_ONCE(mapped_len != size);

[Severity: High]
This is a pre-existing issue, but what happens if mapped_len exceeds size?

The function emits a warning but does not abort execution, passing the larger
mapped_len to fill_sg_entry(). Because the iteration count inside
fill_sg_entry will exceed the allocated SGL nents, sg_next(sgl) will
eventually return NULL.

Could this lead to a NULL pointer dereference in the next iteration?

>  		ret = dma_iova_sync(attach->dev, dma->state, 0, mapped_len);
>  		if (ret)
>  			goto err_unmap_dma;
[ ... ]
>  	/*
>  	 * No CPU list included — set orig_nents = 0 so others can detect
>  	 * this via SG table (use nents only).
>  	 */
>  	dma->sgt.orig_nents = 0;

[Severity: High]
This is a pre-existing issue, but does manually zeroing out orig_nents
break sg_free_table()?

When the mapping is later destroyed, the code calls sg_free_table().
Internally, __sg_free_table() relies on table->orig_nents as the loop
condition to free the underlying scatterlist chunks.

Because it is 0, will the internal while (table->orig_nents) loop immediately
exit without executing kfree(), silently leaking the entire SG table
allocation?

>  
>  	/*
>  	 * SGL must be NULL to indicate that SGL is the last one
>  	 * and we allocated correct number of entries in sg_alloc_table()
>  	 */
>  	WARN_ON_ONCE(sgl);
>  	return &dma->sgt;
>  
>  err_unmap_dma:
>  	if (!i || !dma->state) {
>  		; /* Do nothing */

[Severity: High]
This is a pre-existing issue, but does the !i check incorrectly skip IOVA
cleanup if linking fails on the very first physical range?

If the mapping loop fails on its first iteration (i == 0), the err_unmap_dma
handler hits this condition and skips dma_iova_destroy, leaking the space.

>  	} else if (dma_use_iova(dma->state)) {
>  		dma_iova_destroy(attach->dev, dma->state, mapped_len, dir,
>  				 DMA_ATTR_MMIO);
>  	} 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);

[Severity: High]
This is a pre-existing issue, but does this loop overwrite the failure loop
index i and unmap uninitialized scatterlist entries?

The error handling loop for_each_sgtable_dma_sg resets the index i to 0 and
iterates up to (sgt)->nents. Because the scatterlist table was allocated at
full capacity with __GFP_ZERO, unpopulated SGL entries have
sg_dma_address == 0 and sg_dma_len == 0.

Could calling dma_unmap_phys on these uninitialized entries violate the DMA API
and trigger IOMMU faults?

[Severity: High]
This isn't a bug introduced by this patch, but does calling dma_unmap_phys
on split scatterlist chunks violate the DMA API?

The code maps the full physical memory range dynamically using dma_map_phys()
as a single contiguous mapping. The helper fill_sg_entry() then splits this
mapping into multiple SG list entries.

If an error occurs, the cleanup path iterates over the scattered pieces and
calls dma_unmap_phys() on the individual split chunks.

Since the DMA API requires that unmapping must be performed using the exact
contiguous address and length originally returned by the map function, could
unmapping sub-ranges cause IOMMU state corruption?

>  	}
>  	sg_free_table(&dma->sgt);

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