Re: [PATCH v4 5/5] drm/i915/gem: Remove iterator and use while loop

[sashiko-bot@kernel.org]

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

New issues:
- [High] Flawed scatterlist length overflow check bypasses overflow protection, causing IOVA truncation and IOMMU page table corruption.

Pre-existing issues:
- [High] Pre-existing memory leak in `shmem_sg_alloc_table` error path when scatterlist allocation fails after adding one or more folios to the first segment.
--

--- Patch [5]: [PATCH v4 5/5] drm/i915/gem: Remove iterator and use while loop ---
commit 08123ce95aacd0e0eea7518e3cd7a5c53bd75933
Author: Krzysztof Karas <krzysztof.karas@intel.com>

drm/i915/gem: Remove iterator and use while loop

This commit refactors the main loop in shmem_sg_alloc_table to use a while
loop instead of a for loop. It replaces the iterator with a variable tracking
the completed pages.

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index ac8022997553b..f8fe5e3a4eb40 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
[ ... ]
> @@ -223,7 +223,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  		 * and predict where the next folio begins.
>  		 */
>  		next_pfn = folio_pfn(folio) + folio_page_index + nr_pages;
> -		i += nr_pages - 1;
> +		pages_done += nr_pages;
>  
>  		/* Check that the i965g/gm workaround works. */
>  		GEM_BUG_ON(gfp & __GFP_DMA32 && next_pfn >= 0x00100000UL);

[Severity: High]
This is a pre-existing issue, but does this error path leak folios if we fail
while sg == st->sgl?

Looking at shmem_sg_alloc_table() in drivers/gpu/drm/i915/gem/i915_gem_shmem.c:

err_sg:
    sg_mark_end(sg);
    if (sg != st->sgl) {
        shmem_sg_free_table(st, mapping, false, false);
    } else {
        mapping_clear_unevictable(mapping);
        sg_free_table(st);
    }

If the scatterlist allocation fails after adding one or more folios to the
first segment, we jump to err_sg. At this point, no second scatterlist
segment was created, so sg still points to st->sgl.

The condition sg != st->sgl evaluates to false, causing us to skip
shmem_sg_free_table(). Does this mean the folios placed in the first segment
are never released, resulting in a memory leak under memory pressure?


[Severity: High]
Can the overflow check be bypassed in iommu_dma_map_sg() within
drivers/iommu/dma-iommu.c?

iommu_dma_map_sg() {
    ...
    if ((s_length & UINT_MAX) == 0) {
        /* Prevent possible wrapping upon assignment to s->length. */
        ret = -EOVERFLOW;
        goto out_restore_sg;
    }
    ...
}

If a caller maps a buffer larger than 4GB where the alignment calculation
causes s_length + s_iova_off to exceed UINT_MAX, but the lower 32 bits of
the aligned size are non-zero (for example, 0x100010000 with 64KB granules),
the bitwise check evaluates to false.

Does this bypass the intended overflow detection, causing iommu_map_sg() to
map a truncated number of bytes and potentially leading to unmapped IOVAs
remaining accessible via DMA?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723102542.3245495-1-krzysztof.karas@intel.com?part=5