[PATCH] drm/i915: Fix potential overflow of shmem scatterlist length

[PATCH] drm/i915: Fix potential overflow of shmem scatterlist length

[Janusz Krzysztofik]

When a scatterlists table of a GEM shmem object of size 4 GB or more is
populated with pages allocated from a folio, unsigned int .length
attribute of a scatterlist may get overflowed if total byte length of
pages allocated to that single scatterlist happens to reach or cross the
4GB limit.  As a consequence, users of the object may suffer from hitting
unexpected, premature end of the object's backing pages.

[278.780187] ------------[ cut here ]------------
[278.780377] WARNING: CPU: 1 PID: 2326 at drivers/gpu/drm/i915/i915_mm.c:55 remap_sg+0x199/0x1d0 [i915]
...
[278.780654] CPU: 1 UID: 0 PID: 2326 Comm: gem_mmap_offset Tainted: G S   U              6.17.0-rc1-CI_DRM_16981-ged823aaa0607+ #1 PREEMPT(voluntary)
[278.780656] Tainted: [S]=CPU_OUT_OF_SPEC, [U]=USER
[278.780658] Hardware name: Intel Corporation Meteor Lake Client Platform/MTL-P LP5x T3 RVP, BIOS MTLPFWI1.R00.3471.D91.2401310918 01/31/2024
[278.780659] RIP: 0010:remap_sg+0x199/0x1d0 [i915]
...
[278.780786] Call Trace:
[278.780787]  <TASK>
[278.780788]  ? __apply_to_page_range+0x3e6/0x910
[278.780795]  ? __pfx_remap_sg+0x10/0x10 [i915]
[278.780906]  apply_to_page_range+0x14/0x30
[278.780908]  remap_io_sg+0x14d/0x260 [i915]
[278.781013]  vm_fault_cpu+0xd2/0x330 [i915]
[278.781137]  __do_fault+0x3a/0x1b0
[278.781140]  do_fault+0x322/0x640
[278.781143]  __handle_mm_fault+0x938/0xfd0
[278.781150]  handle_mm_fault+0x12c/0x300
[278.781152]  ? lock_mm_and_find_vma+0x4b/0x760
[278.781155]  do_user_addr_fault+0x2d6/0x8e0
[278.781160]  exc_page_fault+0x96/0x2c0
[278.781165]  asm_exc_page_fault+0x27/0x30
...

That issue was apprehended by the author of a change that introduced it,
and potential risk even annotated with a comment, but then never addressed.

When adding folio pages to a scatterlist table, take care of byte length
of any single scatterlist not exceeding max_segment.

Fixes: 0b62af28f249b ("i915: convert shmem_sg_free_table() to use a folio_batch")
Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14809
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: stable@vger.kernel.org # v6.5+
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index c6c64ba29bc42..720a9ad39aa2a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -153,8 +153,12 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
 			}
 		} while (1);
 
-		nr_pages = min_t(unsigned long,
-				folio_nr_pages(folio), page_count - i);
+		nr_pages = min_array(((unsigned long[]) {
+					folio_nr_pages(folio),
+					page_count - i,
+					max_segment / PAGE_SIZE,
+				      }), 3);
+
 		if (!i ||
 		    sg->length >= max_segment ||
 		    folio_pfn(folio) != next_pfn) {
@@ -164,7 +168,9 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
 			st->nents++;
 			sg_set_folio(sg, folio, nr_pages * PAGE_SIZE, 0);
 		} else {
-			/* XXX: could overflow? */
+			nr_pages = min_t(unsigned long, nr_pages,
+					 (max_segment - sg->length) / PAGE_SIZE);
+
 			sg->length += nr_pages * PAGE_SIZE;
 		}
 		next_pfn = folio_pfn(folio) + nr_pages;
-- 
2.52.0

Re: [PATCH] drm/i915: Fix potential overflow of shmem scatterlist length

[Andi Shyti]

Hi Janusz,

...

> @@ -153,8 +153,12 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  			}
>  		} while (1);
>  

Perhaps we could add here:

	max_pages = max_segment >> PAGE_SHIFT;
	/* Just to be paranoic, but not necessary */
	if (!max_pages)
		max_pages = 1;


> -		nr_pages = min_t(unsigned long,
> -				folio_nr_pages(folio), page_count - i);
> +		nr_pages = min_array(((unsigned long[]) {
> +					folio_nr_pages(folio),
> +					page_count - i,
> +					max_segment / PAGE_SIZE,

max_segment >> PAGE_SHIFT ?

For clarity this can be written as

		nr_pages = min_t(unsigned long,
				folio_nr_pages(folio), page_count - i);
		nr_pages = min_t(unsigned long, nr_pages, max_pages);

But these are nitpicks, it's then up to you to choose the style.

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

> +				      }), 3);
> +

Re: [PATCH] drm/i915: Fix potential overflow of shmem scatterlist length

[Janusz Krzysztofik]

Hi Andi,

Thanks for review.

On Wednesday, 25 February 2026 15:41:13 CET Andi Shyti wrote:
> Hi Janusz,
> 
> ...
> 
> > @@ -153,8 +153,12 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
> >  			}
> >  		} while (1);
> >  
> 
> Perhaps we could add here:
> 
> 	max_pages = max_segment >> PAGE_SHIFT;
> 	/* Just to be paranoic, but not necessary */
> 	if (!max_pages)

GEM_BUG_ON(!max_pages), I would rather say.  The max_segment comes from 
drivers/gpu/drm/i915/i915_scatterlist.h:i915_sg_segment_size(struct device *dev),
let's assume that can't be that much broken.

> 		max_pages = 1;
> 
> 
> > -		nr_pages = min_t(unsigned long,
> > -				folio_nr_pages(folio), page_count - i);
> > +		nr_pages = min_array(((unsigned long[]) {
> > +					folio_nr_pages(folio),
> > +					page_count - i,
> > +					max_segment / PAGE_SIZE,
> 
> max_segment >> PAGE_SHIFT ?

Yeah, shift seems more optimal than division here, however, I've just followed 
the patter used a few lines below. where we can see a multiplication, not a 
right shift, and since PAGE_SIZE is a constant, I hope the compiler will 
optimize that.

> 
> For clarity this can be written as
> 
> 		nr_pages = min_t(unsigned long,
> 				folio_nr_pages(folio), page_count - i);
> 		nr_pages = min_t(unsigned long, nr_pages, max_pages);

Do you think the min_array() is less clear?  Let's see what others say.

> 
> But these are nitpicks, it's then up to you to choose the style.
> 
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thank you,
Janusz

> 
> Thanks,
> Andi
> 
> > +				      }), 3);
> > +
> 

Re: [PATCH] drm/i915: Fix potential overflow of shmem scatterlist length

[Andi Shyti]

> > For clarity this can be written as
> > 
> > 		nr_pages = min_t(unsigned long,
> > 				folio_nr_pages(folio), page_count - i);
> > 		nr_pages = min_t(unsigned long, nr_pages, max_pages);
> 
> Do you think the min_array() is less clear?  Let's see what others say.

min_array() is clear, it's the ((unsigned long[]) { ... }) that
iis nice and fancy but of difficult first read. But, as I said,
it can stay, I don't have a strong opinion, maybe I'd have done
the same.

Andi

Re: [PATCH] drm/i915: Fix potential overflow of shmem scatterlist length

[Janusz Krzysztofik]

On Wednesday, 25 February 2026 16:38:18 CET Andi Shyti wrote:
> > > For clarity this can be written as
> > > 
> > > 		nr_pages = min_t(unsigned long,
> > > 				folio_nr_pages(folio), page_count - i);
> > > 		nr_pages = min_t(unsigned long, nr_pages, max_pages);
> > 
> > Do you think the min_array() is less clear?  Let's see what others say.
> 
> min_array() is clear, it's the ((unsigned long[]) { ... }) that
> iis nice and fancy but of difficult first read. But, as I said,
> it can stay, I don't have a strong opinion, maybe I'd have done
> the same.

Would you be more happy with a locally declared unsigned long table, 
initalized with those 3 values, nr_pages - i and max_segment << PAGE_SHIFT 
statically, folio_nr_pages(folio) once folio is ready, then passed to 
min_array()?

Thanks,
Janusz

> 
> Andi
> 

Re: [PATCH] drm/i915: Fix potential overflow of shmem scatterlist length

[Andi Shyti]

On Wed, Feb 25, 2026 at 06:29:12PM +0100, Janusz Krzysztofik wrote:
> On Wednesday, 25 February 2026 16:38:18 CET Andi Shyti wrote:
> > > > For clarity this can be written as
> > > > 
> > > > 		nr_pages = min_t(unsigned long,
> > > > 				folio_nr_pages(folio), page_count - i);
> > > > 		nr_pages = min_t(unsigned long, nr_pages, max_pages);
> > > 
> > > Do you think the min_array() is less clear?  Let's see what others say.
> > 
> > min_array() is clear, it's the ((unsigned long[]) { ... }) that
> > iis nice and fancy but of difficult first read. But, as I said,
> > it can stay, I don't have a strong opinion, maybe I'd have done
> > the same.
> 
> Would you be more happy with a locally declared unsigned long table, 
> initalized with those 3 values, nr_pages - i and max_segment << PAGE_SHIFT 
> statically, folio_nr_pages(folio) once folio is ready, then passed to 
> min_array()?

mine was only an idea, I'm fine with what it is now if no one has
anything against it. Perhaps, just a little comment could help
understand why we are evaluating out of three elements (but we
are talking trivial details here :) ).

Thanks,
Andi

Re: [PATCH] drm/i915: Fix potential overflow of shmem scatterlist length

[Sebastian Brzezinka]

On Thu Feb 26, 2026 at 1:23 PM CET, Andi Shyti wrote:
> On Wed, Feb 25, 2026 at 06:29:12PM +0100, Janusz Krzysztofik wrote:
>> On Wednesday, 25 February 2026 16:38:18 CET Andi Shyti wrote:
>> > > > For clarity this can be written as
>> > > > 
>> > > > 		nr_pages = min_t(unsigned long,
>> > > > 				folio_nr_pages(folio), page_count - i);
>> > > > 		nr_pages = min_t(unsigned long, nr_pages, max_pages);
>> > > 
>> > > Do you think the min_array() is less clear?  Let's see what others say.
>> > 
>> > min_array() is clear, it's the ((unsigned long[]) { ... }) that
>> > iis nice and fancy but of difficult first read. But, as I said,
>> > it can stay, I don't have a strong opinion, maybe I'd have done
>> > the same.
>> 
>> Would you be more happy with a locally declared unsigned long table, 
>> initalized with those 3 values, nr_pages - i and max_segment << PAGE_SHIFT 
>> statically, folio_nr_pages(folio) once folio is ready, then passed to 
>> min_array()?
>
> mine was only an idea, I'm fine with what it is now if no one has
> anything against it. Perhaps, just a little comment could help
> understand why we are evaluating out of three elements (but we
> are talking trivial details here :) ).
>
> Thanks,
> Andi
There’s also a min3 function, but you’d need to cast the
variables, so using an array is ok in my opinion.

Best regards,
Sebastian

Claude review: drm/i915: Fix potential overflow of shmem scatterlist length

[Claude Code Review Bot]

Overall Series Review

Subject: drm/i915: Fix potential overflow of shmem scatterlist length
Author: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Patches: 7
Reviewed: 2026-02-27T15:21:32.541027

---

This is a single-patch fix for a real overflow bug in the i915 shmem scatterlist allocation path. The `sg->length` field is `unsigned int` (32-bit), and when contiguous folios are coalesced into a single scatterlist entry, the accumulated length can overflow past 4GB. The bug was acknowledged by the original author with a `/* XXX: could overflow? */` comment but never fixed. The fix is targeted at the right code and the commit message, Fixes tag, stack trace, and Cc: stable are all correct.

However, the patch introduces a **new correctness issue**: when a folio must be split across two sg entries (because it straddles the `max_segment` boundary), the code re-fetches the same folio in the next loop iteration but maps it from offset 0, double-mapping the pages at the start of the folio while losing the pages at the tail. There is also a minor style concern with the use of `min_array` and a compound literal where `min3` would be more conventional.

**Verdict**: The fix addresses a real, triggerable bug but needs a respin to correctly handle partial-folio splits.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/i915: Fix potential overflow of shmem scatterlist length

[Claude Code Review Bot]

Patch Review

**Commit message**: Well-written. The problem description, stack trace, Fixes tag (`0b62af28f249b`), Closes link, and `Cc: stable` are all appropriate. Good that it references the original `/* XXX: could overflow? */` comment.

**Hunk 1 — Clamping `nr_pages` at allocation (lines 153–142 of the diff)**:

```c
-		nr_pages = min_t(unsigned long,
-				folio_nr_pages(folio), page_count - i);
+		nr_pages = min_array(((unsigned long[]) {
+					folio_nr_pages(folio),
+					page_count - i,
+					max_segment / PAGE_SIZE,
+				      }), 3);
```

**Style concern**: Using `min_array` with a compound literal is unusual for kernel code. The kernel provides `min3()` (defined at `include/linux/minmax.h:143`) which is the conventional way to take the minimum of three values. Alternatively, chaining two `min_t()` calls would be clearer:

```c
nr_pages = min_t(unsigned long, folio_nr_pages(folio), page_count - i);
nr_pages = min_t(unsigned long, nr_pages, max_segment / PAGE_SIZE);
```

**Correctness concern**: Adding `max_segment / PAGE_SIZE` as a limit here means that if a single folio is larger than `max_segment`, `nr_pages` will be clamped and `i` will advance only partway through the folio (`i += nr_pages - 1`). The next iteration calls `shmem_read_folio_gfp(mapping, i, gfp)` which returns the **same folio**, but `folio_nr_pages(folio)` still returns the full folio size, and `sg_set_folio(sg, folio, nr_pages * PAGE_SIZE, 0)` maps from **offset 0** — re-mapping pages already covered. In practice this is unlikely since shmem folios don't typically exceed `max_segment` (~4GB), but it's a latent bug.

**Hunk 2 — Clamping in the coalescing branch (lines 150–155 of the diff)**:

```c
-			/* XXX: could overflow? */
+			nr_pages = min_t(unsigned long, nr_pages,
+					 (max_segment - sg->length) / PAGE_SIZE);
+
 			sg->length += nr_pages * PAGE_SIZE;
```

**This is the core of the fix** and addresses the overflow directly. However:

**Bug introduced**: When `nr_pages` is clamped below the folio's actual remaining pages, the loop advances `i` by only the clamped count. The next iteration re-fetches the same folio (since `i` still falls within it). Then:

1. `folio_pfn(folio)` returns the **head page PFN**, not the PFN at index `i` within the folio
2. `folio_pfn(folio) != next_pfn` evaluates true → new sg entry is started
3. `sg_set_folio(sg, folio, nr_pages * PAGE_SIZE, 0)` maps from **offset 0** in the folio

This means the first N pages of the folio are double-mapped (once in the old sg entry, once in the new), while the remaining pages at the tail of the folio that should have been mapped in the new entry are lost. The backing store of the GEM object would have incorrect physical pages.

**Concrete example**: Suppose `sg->length` is at `max_segment - 1MB`, and a 2MB folio arrives for coalescing. `nr_pages` is clamped to 256 (1MB worth). The remaining 256 pages of the folio should be mapped starting at offset 1MB within the folio, but instead they'd be mapped from offset 0, duplicating the first 256 pages.

**Suggested approach**: Track the offset within the folio when splitting is needed. Something like:

```c
unsigned long folio_ofs = i - folio->index;

nr_pages = min_t(unsigned long,
                 folio_nr_pages(folio) - folio_ofs, page_count - i);
nr_pages = min_t(unsigned long, nr_pages, max_segment / PAGE_SIZE);

/* ... */
sg_set_folio(sg, folio, nr_pages * PAGE_SIZE, folio_ofs * PAGE_SIZE);
```

And in the coalescing branch, the remaining-space clamping should account for the fact that unclaimed folio pages need proper offset tracking in the subsequent iteration.

**Edge case**: If `max_segment` is not page-aligned and `sg->length` equals `max_segment` rounded down to the nearest page, then `(max_segment - sg->length) / PAGE_SIZE` could be 0, making `nr_pages = 0` and causing `i += nr_pages - 1` to underflow (since `i` is `unsigned long`). This is guarded by the `sg->length >= max_segment` check in the preceding `if`, but only if `max_segment` is page-aligned. In practice `i915_sg_segment_size()` returns page-aligned values, so this is safe but fragile.

**Summary**: The patch correctly identifies and addresses a real overflow bug, but the implementation introduces a folio-splitting correctness issue when a folio straddles the `max_segment` boundary. I'd recommend a respin that properly tracks the offset within the folio. The `min_array` usage should also be replaced with `min3` or chained `min_t` calls for conventional style.

---
Generated by Claude Code Patch Reviewer