[2/2] drm/i915/userptr: fix size calculation
diff mbox series

Message ID 20200116192809.843138-2-matthew.auld@intel.com
State New
Headers show
Series
  • [1/2] drm/i915/userptr: add user_size limit check
Related show

Commit Message

Matthew Auld Jan. 16, 2020, 7:28 p.m. UTC
If we create a rather large userptr object(e.g 1ULL << 32) we might
shift past the type-width of num_pages: (int)num_pages << PAGE_SHIFT,
resulting in a totally bogus sg_table, which fortunately will eventually
manifest as:

gen8_ppgtt_insert_huge:463 GEM_BUG_ON(iter->sg->length < page_size)
kernel BUG at drivers/gpu/drm/i915/gt/gen8_ppgtt.c:463!

Fixes: 5cc9ed4b9a7a ("drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl")
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 ++-
 drivers/gpu/drm/i915/gt/gen8_ppgtt.c        | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Chris Wilson Jan. 16, 2020, 7:33 p.m. UTC | #1
Quoting Matthew Auld (2020-01-16 19:28:09)
> If we create a rather large userptr object(e.g 1ULL << 32) we might
> shift past the type-width of num_pages: (int)num_pages << PAGE_SHIFT,
> resulting in a totally bogus sg_table, which fortunately will eventually
> manifest as:
> 
> gen8_ppgtt_insert_huge:463 GEM_BUG_ON(iter->sg->length < page_size)
> kernel BUG at drivers/gpu/drm/i915/gt/gen8_ppgtt.c:463!
> 
> Fixes: 5cc9ed4b9a7a ("drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 ++-
>  drivers/gpu/drm/i915/gt/gen8_ppgtt.c        | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index fef96a303d9d..81fa53495c9d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -405,6 +405,7 @@ static struct sg_table *
>  __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
>                                struct page **pvec, int num_pages)

Well that answers that question. Why not use unsigned long?

That's what the comment is all about, fixing the code! Not continuing to
bodge it.
-Chris
Chris Wilson Jan. 16, 2020, 7:34 p.m. UTC | #2
Quoting Matthew Auld (2020-01-16 19:28:09)
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index 077b8f7cf6cb..0d7820c49f5b 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -379,6 +379,7 @@ gen8_ppgtt_insert_pte(struct i915_ppgtt *ppgtt,
>         pd = i915_pd_entry(pdp, gen8_pd_index(idx, 2));
>         vaddr = kmap_atomic_px(i915_pt_entry(pd, gen8_pd_index(idx, 1)));
>         do {
> +               GEM_BUG_ON(iter->sg->length < PAGE_SIZE);

s/PAGE_SIZE/I915_GTT_PAGE_SIZE/

>                 vaddr[gen8_pd_index(idx, 0)] = pte_encode | iter->dma;
>  
>                 iter->dma += I915_GTT_PAGE_SIZE;
> -- 
> 2.20.1
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index fef96a303d9d..81fa53495c9d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -405,6 +405,7 @@  static struct sg_table *
 __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
 			       struct page **pvec, int num_pages)
 {
+	unsigned long size = (unsigned long)num_pages << PAGE_SHIFT;
 	unsigned int max_segment = i915_sg_segment_size();
 	struct sg_table *st;
 	unsigned int sg_page_sizes;
@@ -416,7 +417,7 @@  __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
 
 alloc_table:
 	ret = __sg_alloc_table_from_pages(st, pvec, num_pages,
-					  0, num_pages << PAGE_SHIFT,
+					  0, size,
 					  max_segment,
 					  GFP_KERNEL);
 	if (ret) {
diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
index 077b8f7cf6cb..0d7820c49f5b 100644
--- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
@@ -379,6 +379,7 @@  gen8_ppgtt_insert_pte(struct i915_ppgtt *ppgtt,
 	pd = i915_pd_entry(pdp, gen8_pd_index(idx, 2));
 	vaddr = kmap_atomic_px(i915_pt_entry(pd, gen8_pd_index(idx, 1)));
 	do {
+		GEM_BUG_ON(iter->sg->length < PAGE_SIZE);
 		vaddr[gen8_pd_index(idx, 0)] = pte_encode | iter->dma;
 
 		iter->dma += I915_GTT_PAGE_SIZE;