[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

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

[Andi Shyti]

Hi Janusz,

On Tue, Feb 24, 2026 at 10:49:06AM +0100, Janusz Krzysztofik wrote:
> 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>

Applied to drm-intel-gt-next.

Thanks,
Andi