Re: [PATCH v8 2/2] dma-buf: Split sgl by largest page-aligned chunk

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Deterministic NULL pointer dereference on 32-bit architectures due to mismatch between scatterlist allocation and iteration count.

Pre-existing issues:
- [High] Piecewise unmapping of chunked DMA regions violates DMA API rules.
- [High] Error handling path shadows the loop counter and unmaps uninitialized scatterlist entries.
--

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

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

This commit changes fill_sg_entry() to split scatterlists by
ALIGN_DOWN(UINT_MAX, PAGE_SIZE) rather than UINT_MAX to ensure entries
remain page-aligned. Page-aligned entries prevent failures in hardware
DMA engines that require explicit address alignment and avoid edge cases
in ib_umem_find_best_pgsz().

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 80f6ab2f4809d..833be519e1e6a 100644
> --- a/drivers/dma-buf/dma-buf-mapping.c
> +++ b/drivers/dma-buf/dma-buf-mapping.c
> @@ -6,16 +6,17 @@
>  #include <linux/dma-buf-mapping.h>
>  #include <linux/dma-resv.h>
>  #include <linux/overflow.h>
> +#include <linux/align.h>
> +
> +#define MAX_SG_ENT_SZ ALIGN_DOWN(UINT_MAX, PAGE_SIZE)
>  
>  static struct scatterlist *fill_sg_entry(struct scatterlist *sgl, size_t length,
>  					 dma_addr_t addr)
>  {
> -	unsigned int len, nents;
> -	unsigned 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);

[Severity: High]
This is a pre-existing issue, but does chunking the mapped physical region into
smaller pieces here cause a violation of the DMA API during teardown?

During dma_buf_free_sgt() or the err_unmap_dma path, the code iterates over this
scatterlist and calls dma_unmap_phys() on each split chunk individually:

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 DMA API mandates that unmapping must use the exact identical start address
and length that was originally passed to dma_map_phys().

>  		length -= len;
>  		/*
>  		 * DMABUF abuses scatterlist to create a scatterlist
> @@ -25,8 +26,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: Critical]
Since calc_sg_nents() calculates too few entries for lengths > 4096 on 32-bit
architectures due to integer wrapping, does this sg_set_page() unconditionally
dereference the NULL pointer returned by sg_next() when the undersized
scatterlist allocation is exhausted?

> -		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);
>  	}
>  
> @@ -42,7 +45,7 @@ static unsigned int calc_sg_nents(struct dma_iova_state *state,
>  
>  	if (!state || !dma_use_iova(state)) {
>  		for (i = 0; i < nr_ranges; i++) {
> -			unsigned int added = DIV_ROUND_UP(phys_vec[i].len, UINT_MAX);
> +			unsigned int added = DIV_ROUND_UP(phys_vec[i].len, MAX_SG_ENT_SZ);

[Severity: Critical]
Does this calculation severely under-calculate or return 0 for the number of
required SG entries on 32-bit architectures?

On 32-bit architectures, MAX_SG_ENT_SZ expands to 0xFFFFF000. For lengths
greater than 4096, DIV_ROUND_UP() expands to (len + 0xFFFFF000 - 1) / 0xFFFFF000,
which will wrap around the 32-bit addition and yield an artificially low or zero
number of entries.

[Severity: High]
This is a pre-existing issue, but does the error handling path following this
allocation unmap uninitialized scatterlist entries if dma_map_phys() fails?

When dma_map_phys() fails, the err_unmap_dma path in dma_buf_phys_vec_to_sgt()
attempts cleanup:

drivers/dma-buf/dma-buf-mapping.c:dma_buf_phys_vec_to_sgt() {
    ...
err_unmap_dma:
    ...
	} 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);
	}
    ...
}

The for_each_sgtable_dma_sg macro internally shadows the outer failure index i
and initializes its own counter to 0. It then unconditionally iterates through
all sgt->nents allocated entries. Because the failure occurred early, trailing
entries remain zero-initialized, causing dma_unmap_phys() to be invoked on
addresses of 0.

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