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 <[email protected]> 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/[email protected]?part=5
