* Re: [PATCH v5] dma-buf: Fix silent overflow for phys vec to sgt
2026-06-01 20:00 [PATCH v5] dma-buf: Fix silent overflow for phys vec to sgt David Hu
@ 2026-06-01 20:13 ` sashiko-bot
2026-06-03 7:04 ` Tian, Kevin
2026-06-04 9:43 ` Leon Romanovsky
2 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-06-01 20:13 UTC (permalink / raw)
To: David Hu; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH v5] dma-buf: Fix silent overflow for phys vec to sgt
2026-06-01 20:00 [PATCH v5] dma-buf: Fix silent overflow for phys vec to sgt David Hu
2026-06-01 20:13 ` sashiko-bot
@ 2026-06-03 7:04 ` Tian, Kevin
2026-06-04 9:43 ` Leon Romanovsky
2 siblings, 0 replies; 5+ messages in thread
From: Tian, Kevin @ 2026-06-03 7:04 UTC (permalink / raw)
To: David, Hu, Sumit Semwal, Christian König
Cc: Jason Gunthorpe, Nicolin Chen, Leon Romanovsky, Ankit Agrawal,
Alex Williamson, linux-media@vger.kernel.org,
dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
linux-kernel@vger.kernel.org, iommu@lists.linux.dev,
jmoroni@google.com, praan@google.com, David, Hu,
stable@vger.kernel.org
> From: David Hu <xuehaohu@google.com>
> Sent: Tuesday, June 2, 2026 4:00 AM
>
> In case MMIO size is bigger than 4G and peer2peer DMA goes
> through host bridge, we trigger a code path that assigns the
> total linked IOVA (which is greater than 4G) to mapped_len.
>
> Previously, `mapped_len` was declared as 32-bit `unsigned int`.
> When accumulating `size_t` lengths, this leads to a silent wrap-around.
> This truncation causes truncated lengths to be passed to functions
> like `fill_sg_entry()`.
>
> Fix this by changing `mapped_len` to `size_t` (64-bit). While
> at it, fix similar potential overflow issues in `calc_sg_nents`
> by using `size_t` for `nents` and checking against `UINT_MAX`
> and using `unsigned int` for the loop iterator in `fill_sg_entry`
> to match.
>
> Fixes: 3aa31a8bb11e ("dma-buf: provide phys_vec to scatter-gather mapping
> routine")
> Cc: stable@vger.kernel.org
> Cc: iommu@lists.linux.dev
> Reviewed-by: Pranjal Shrivastava <praan@google.com>
> Signed-off-by: David Hu <xuehaohu@google.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v5] dma-buf: Fix silent overflow for phys vec to sgt
2026-06-01 20:00 [PATCH v5] dma-buf: Fix silent overflow for phys vec to sgt David Hu
2026-06-01 20:13 ` sashiko-bot
2026-06-03 7:04 ` Tian, Kevin
@ 2026-06-04 9:43 ` Leon Romanovsky
2026-06-04 19:36 ` David Hu
2 siblings, 1 reply; 5+ messages in thread
From: Leon Romanovsky @ 2026-06-04 9:43 UTC (permalink / raw)
To: David Hu
Cc: Sumit Semwal, Christian König, Jason Gunthorpe, Nicolin Chen,
Kevin Tian, Ankit Agrawal, Alex Williamson, linux-media,
dri-devel, linaro-mm-sig, linux-kernel, iommu, jmoroni, praan,
stable
On Mon, Jun 01, 2026 at 08:00:12PM +0000, David Hu wrote:
> In case MMIO size is bigger than 4G and peer2peer DMA goes
> through host bridge, we trigger a code path that assigns the
> total linked IOVA (which is greater than 4G) to mapped_len.
>
> Previously, `mapped_len` was declared as 32-bit `unsigned int`.
> When accumulating `size_t` lengths, this leads to a silent wrap-around.
> This truncation causes truncated lengths to be passed to functions
> like `fill_sg_entry()`.
>
> Fix this by changing `mapped_len` to `size_t` (64-bit). While
> at it, fix similar potential overflow issues in `calc_sg_nents`
> by using `size_t` for `nents` and checking against `UINT_MAX`
> and using `unsigned int` for the loop iterator in `fill_sg_entry`
> to match.
>
> Fixes: 3aa31a8bb11e ("dma-buf: provide phys_vec to scatter-gather mapping routine")
> Cc: stable@vger.kernel.org
> Cc: iommu@lists.linux.dev
> Reviewed-by: Pranjal Shrivastava <praan@google.com>
> Signed-off-by: David Hu <xuehaohu@google.com>
> ---
> Changes in v5:
> - Removed WARN_ON_ONCE from calc_sg_nents() to avoid log noise (Jason).
> - Added explicit check for `!nents` in dma_buf_phys_vec_to_sgt() to
> cleanly return -EINVAL on overflow (Jason).
>
> Changes in v4:
> - Added WARN_ON_ONCE() to the nents overflow check to prevent silent
> failures (Claude Bot).
>
> Changes in v3:
> - Removed leftover sentence fragment from the commit message.
> - Kept `nents = 0` initialization (previously stated as removed in the
> v2 changelog) as it is strictly required for the `+=` accumulation
> loop in `calc_sg_nents()`.
>
> Changes in v2:
> - Fixed 'IVOA' -> 'IOVA' typo and expanded commit message (Claude Bot).
> - Added Reverse Xmas tree formatting (Pranjal).
> - Folded in extra bounds checking for calc_sg_nents() (Pranjal).
> - Folded in type consistency fix for fill_sg_entry() (Pranjal).
>
> drivers/dma-buf/dma-buf-mapping.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
> index 794acff2546a..607b7998463d 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);
> for (i = 0; i < nents; i++) {
> @@ -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);
> }
>
> + if (nents > UINT_MAX)
I would suggest to use check_add_overflow() while calculating nents
instead of this check.
> + return 0;
> +
> return nents;
> }
>
> @@ -95,9 +98,10 @@ struct sg_table *dma_buf_phys_vec_to_sgt(struct dma_buf_attachment *attach,
> size_t nr_ranges, size_t size,
> enum dma_data_direction dir)
> {
> - unsigned int nents, mapped_len = 0;
> struct dma_buf_dma *dma;
> struct scatterlist *sgl;
> + size_t mapped_len = 0;
> + unsigned int nents;
> dma_addr_t addr;
> size_t i;
> int ret;
> @@ -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;
> + }
Technically, this hunk is not necessary, since sg_alloc_table() will
return -EINVAL when nents == 0. At least, that is the behavior I relied on.
Thanks
> +
> ret = sg_alloc_table(&dma->sgt, nents, GFP_KERNEL | __GFP_ZERO);
> if (ret)
> goto err_free_state;
> --
> 2.54.0.929.g9b7fa37559-goog
>
^ permalink raw reply [flat|nested] 5+ messages in thread