diff mbox series

[v2,3/7] drm/i915: Check for integer truncation on scatterlist creation

Message ID 20220705122455.3866745-4-gwan-gyeong.mun@intel.com (mailing list archive)
State New, archived
Headers show
Series Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation | expand

Commit Message

Gwan-gyeong Mun July 5, 2022, 12:24 p.m. UTC 
From: Chris Wilson <chris@chris-wilson.co.uk>

There is an impedance mismatch between the scatterlist API using unsigned
int and our memory/page accounting in unsigned long. That is we may try
to create a scatterlist for a large object that overflows returning a
small table into which we try to fit very many pages. As the object size
is under control of userspace, we have to be prudent and catch the
conversion errors.

To catch the implicit truncation as we switch from unsigned long into the
scatterlist's unsigned int, we use overflows_type check and report
E2BIG prior to the operation. This is already used in our create ioctls to
indicate if the uABI request is simply too large for the backing store.
Failing that type check, we have a second check at sg_alloc_table time
to make sure the values we are passing into the scatterlist API are not
truncated.

It uses pgoff_t for locals that are dealing with page indices, in this
case, the page count is the limit of the page index.
And it uses safe_conversion() macro which performs a type conversion (cast)
of an integer value into a new variable, checking that the destination is
large enough to hold the source value.

v2: Move added i915_utils's macro into drm_util header (Jani N)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Brian Welty <brian.welty@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_internal.c | 6 ++++--
 drivers/gpu/drm/i915/gem/i915_gem_object.h   | 3 ---
 drivers/gpu/drm/i915/gem/i915_gem_phys.c     | 4 ++++
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c    | 5 ++++-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c      | 4 ++++
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c  | 5 ++++-
 drivers/gpu/drm/i915/gvt/dmabuf.c            | 9 +++++----
 drivers/gpu/drm/i915/i915_scatterlist.h      | 8 ++++++++
 8 files changed, 33 insertions(+), 11 deletions(-)

Comments

Mauro Carvalho Chehab July 5, 2022, 2:48 p.m. UTC | #1 
On Tue,  5 Jul 2022 15:24:51 +0300
Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote:

> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> There is an impedance mismatch between the scatterlist API using unsigned
> int and our memory/page accounting in unsigned long. That is we may try
> to create a scatterlist for a large object that overflows returning a
> small table into which we try to fit very many pages. As the object size
> is under control of userspace, we have to be prudent and catch the
> conversion errors.
> 
> To catch the implicit truncation as we switch from unsigned long into the
> scatterlist's unsigned int, we use overflows_type check and report
> E2BIG prior to the operation. This is already used in our create ioctls to
> indicate if the uABI request is simply too large for the backing store.
> Failing that type check, we have a second check at sg_alloc_table time
> to make sure the values we are passing into the scatterlist API are not
> truncated.
> 
> It uses pgoff_t for locals that are dealing with page indices, in this
> case, the page count is the limit of the page index.
> And it uses safe_conversion() macro which performs a type conversion (cast)
> of an integer value into a new variable, checking that the destination is
> large enough to hold the source value.
> 
> v2: Move added i915_utils's macro into drm_util header (Jani N)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Brian Welty <brian.welty@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_internal.c | 6 ++++--
>  drivers/gpu/drm/i915/gem/i915_gem_object.h   | 3 ---
>  drivers/gpu/drm/i915/gem/i915_gem_phys.c     | 4 ++++
>  drivers/gpu/drm/i915/gem/i915_gem_shmem.c    | 5 ++++-
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c      | 4 ++++
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c  | 5 ++++-
>  drivers/gpu/drm/i915/gvt/dmabuf.c            | 9 +++++----
>  drivers/gpu/drm/i915/i915_scatterlist.h      | 8 ++++++++
>  8 files changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> index c698f95af15f..ff2e6e780631 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> @@ -37,10 +37,13 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
>  	struct sg_table *st;
>  	struct scatterlist *sg;
>  	unsigned int sg_page_sizes;
> -	unsigned int npages;
> +	pgoff_t npages; /* restricted by sg_alloc_table */
>  	int max_order;
>  	gfp_t gfp;
>  
> +	if (!safe_conversion(&npages, obj->base.size >> PAGE_SHIFT))
> +		return -E2BIG;
> +
>  	max_order = MAX_ORDER;
>  #ifdef CONFIG_SWIOTLB
>  	if (is_swiotlb_active(obj->base.dev->dev)) {
> @@ -67,7 +70,6 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
>  	if (!st)
>  		return -ENOMEM;
>  
> -	npages = obj->base.size / PAGE_SIZE;
>  	if (sg_alloc_table(st, npages, GFP_KERNEL)) {
>  		kfree(st);
>  		return -ENOMEM;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index a60c6f4517d5..31bb09dccf2f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -26,9 +26,6 @@ enum intel_region_id;
>   * this and catch if we ever need to fix it. In the meantime, if you do
>   * spot such a local variable, please consider fixing!
>   *
> - * Aside from our own locals (for which we have no excuse!):
> - * - sg_table embeds unsigned int for nents
> - *
>   * We can check for invalidly typed locals with typecheck(), see for example
>   * i915_gem_object_get_sg().
>   */
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> index 0d0e46dae559..88ba7266a3a5 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> @@ -28,6 +28,10 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
>  	void *dst;
>  	int i;
>  
> +	/* Contiguous chunk, with a single scatterlist element */
> +	if (overflows_type(obj->base.size, sg->length))
> +		return -E2BIG;
> +
>  	if (GEM_WARN_ON(i915_gem_object_needs_bit17_swizzle(obj)))
>  		return -EINVAL;
>  
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 4eed3dd90ba8..604e8829e8ea 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -193,13 +193,16 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj)
>  	struct drm_i915_private *i915 = to_i915(obj->base.dev);
>  	struct intel_memory_region *mem = obj->mm.region;
>  	struct address_space *mapping = obj->base.filp->f_mapping;
> -	const unsigned long page_count = obj->base.size / PAGE_SIZE;
>  	unsigned int max_segment = i915_sg_segment_size();
>  	struct sg_table *st;
>  	struct sgt_iter sgt_iter;
> +	pgoff_t page_count;
>  	struct page *page;
>  	int ret;
>  
> +	if (!safe_conversion(&page_count, obj->base.size >> PAGE_SHIFT))
> +		return -E2BIG;
> +
>  	/*
>  	 * Assert that the object is not currently in any GPU domain. As it
>  	 * wasn't in the GTT, there shouldn't be any way it could have been in
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 50a02d850139..cdcb3ee0c433 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -815,6 +815,10 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
>  {
>  	struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS];
>  	struct ttm_placement placement;
> +	pgoff_t num_pages;
> +
> +	if (!safe_conversion(&num_pages, obj->base.size >> PAGE_SHIFT))
> +		return -E2BIG;
>  
>  	GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS);
>  
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 094f06b4ce33..25785c3a0083 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -128,13 +128,16 @@ static void i915_gem_object_userptr_drop_ref(struct drm_i915_gem_object *obj)
>  
>  static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>  {
> -	const unsigned long num_pages = obj->base.size >> PAGE_SHIFT;
>  	unsigned int max_segment = i915_sg_segment_size();
>  	struct sg_table *st;
>  	unsigned int sg_page_sizes;
>  	struct page **pvec;
> +	pgoff_t num_pages; /* limited by sg_alloc_table_from_pages_segment */
>  	int ret;
>  
> +	if (!safe_conversion(&num_pages, obj->base.size >> PAGE_SHIFT))
> +		return -E2BIG;
> +
>  	st = kmalloc(sizeof(*st), GFP_KERNEL);
>  	if (!st)
>  		return -ENOMEM;
> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c
> index 01e54b45c5c1..795270cb4ec2 100644
> --- a/drivers/gpu/drm/i915/gvt/dmabuf.c
> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> @@ -42,8 +42,7 @@
>  
>  #define GEN8_DECODE_PTE(pte) (pte & GENMASK_ULL(63, 12))
>  
> -static int vgpu_gem_get_pages(
> -		struct drm_i915_gem_object *obj)
> +static int vgpu_gem_get_pages(struct drm_i915_gem_object *obj)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>  	struct intel_vgpu *vgpu;
> @@ -52,7 +51,10 @@ static int vgpu_gem_get_pages(
>  	int i, j, ret;
>  	gen8_pte_t __iomem *gtt_entries;
>  	struct intel_vgpu_fb_info *fb_info;
> -	u32 page_num;
> +	pgoff_t page_num;
> +
> +	if (!safe_conversion(&page_num, obj->base.size >> PAGE_SHIFT))
> +		return -E2BIG;
>  
>  	fb_info = (struct intel_vgpu_fb_info *)obj->gvt_info;
>  	if (drm_WARN_ON(&dev_priv->drm, !fb_info))
> @@ -66,7 +68,6 @@ static int vgpu_gem_get_pages(
>  	if (unlikely(!st))
>  		return -ENOMEM;
>  
> -	page_num = obj->base.size >> PAGE_SHIFT;
>  	ret = sg_alloc_table(st, page_num, GFP_KERNEL);
>  	if (ret) {
>  		kfree(st);
> diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h b/drivers/gpu/drm/i915/i915_scatterlist.h
> index 12c6a1684081..c4d4d3c84cff 100644
> --- a/drivers/gpu/drm/i915/i915_scatterlist.h
> +++ b/drivers/gpu/drm/i915/i915_scatterlist.h
> @@ -218,4 +218,12 @@ struct i915_refct_sgt *i915_rsgt_from_mm_node(const struct drm_mm_node *node,
>  struct i915_refct_sgt *i915_rsgt_from_buddy_resource(struct ttm_resource *res,
>  						     u64 region_start);
>  
> +/* Wrap scatterlist.h to sanity check for integer truncation */
> +typedef unsigned int __sg_size_t; /* see linux/scatterlist.h */
> +#define sg_alloc_table(sgt, nents, gfp) \
> +	overflows_type(nents, __sg_size_t) ? -E2BIG : (sg_alloc_table)(sgt, (__sg_size_t)(nents), gfp)
> +
> +#define sg_alloc_table_from_pages_segment(sgt, pages, npages, offset, size, max_segment, gfp) \
> +	overflows_type(npages, __sg_size_t) ? -E2BIG : (sg_alloc_table_from_pages_segment)(sgt, pages, (__sg_size_t)(npages), offset, size, max_segment, gfp)
> +
>  #endif
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
index c698f95af15f..ff2e6e780631 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
@@ -37,10 +37,13 @@  static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
 	struct sg_table *st;
 	struct scatterlist *sg;
 	unsigned int sg_page_sizes;
-	unsigned int npages;
+	pgoff_t npages; /* restricted by sg_alloc_table */
 	int max_order;
 	gfp_t gfp;
 
+	if (!safe_conversion(&npages, obj->base.size >> PAGE_SHIFT))
+		return -E2BIG;
+
 	max_order = MAX_ORDER;
 #ifdef CONFIG_SWIOTLB
 	if (is_swiotlb_active(obj->base.dev->dev)) {
@@ -67,7 +70,6 @@  static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
 	if (!st)
 		return -ENOMEM;
 
-	npages = obj->base.size / PAGE_SIZE;
 	if (sg_alloc_table(st, npages, GFP_KERNEL)) {
 		kfree(st);
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index a60c6f4517d5..31bb09dccf2f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -26,9 +26,6 @@  enum intel_region_id;
  * this and catch if we ever need to fix it. In the meantime, if you do
  * spot such a local variable, please consider fixing!
  *
- * Aside from our own locals (for which we have no excuse!):
- * - sg_table embeds unsigned int for nents
- *
  * We can check for invalidly typed locals with typecheck(), see for example
  * i915_gem_object_get_sg().
  */
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
index 0d0e46dae559..88ba7266a3a5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
@@ -28,6 +28,10 @@  static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
 	void *dst;
 	int i;
 
+	/* Contiguous chunk, with a single scatterlist element */
+	if (overflows_type(obj->base.size, sg->length))
+		return -E2BIG;
+
 	if (GEM_WARN_ON(i915_gem_object_needs_bit17_swizzle(obj)))
 		return -EINVAL;
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 4eed3dd90ba8..604e8829e8ea 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -193,13 +193,16 @@  static int shmem_get_pages(struct drm_i915_gem_object *obj)
 	struct drm_i915_private *i915 = to_i915(obj->base.dev);
 	struct intel_memory_region *mem = obj->mm.region;
 	struct address_space *mapping = obj->base.filp->f_mapping;
-	const unsigned long page_count = obj->base.size / PAGE_SIZE;
 	unsigned int max_segment = i915_sg_segment_size();
 	struct sg_table *st;
 	struct sgt_iter sgt_iter;
+	pgoff_t page_count;
 	struct page *page;
 	int ret;
 
+	if (!safe_conversion(&page_count, obj->base.size >> PAGE_SHIFT))
+		return -E2BIG;
+
 	/*
 	 * Assert that the object is not currently in any GPU domain. As it
 	 * wasn't in the GTT, there shouldn't be any way it could have been in
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 50a02d850139..cdcb3ee0c433 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -815,6 +815,10 @@  static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
 {
 	struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS];
 	struct ttm_placement placement;
+	pgoff_t num_pages;
+
+	if (!safe_conversion(&num_pages, obj->base.size >> PAGE_SHIFT))
+		return -E2BIG;
 
 	GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 094f06b4ce33..25785c3a0083 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -128,13 +128,16 @@  static void i915_gem_object_userptr_drop_ref(struct drm_i915_gem_object *obj)
 
 static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 {
-	const unsigned long num_pages = obj->base.size >> PAGE_SHIFT;
 	unsigned int max_segment = i915_sg_segment_size();
 	struct sg_table *st;
 	unsigned int sg_page_sizes;
 	struct page **pvec;
+	pgoff_t num_pages; /* limited by sg_alloc_table_from_pages_segment */
 	int ret;
 
+	if (!safe_conversion(&num_pages, obj->base.size >> PAGE_SHIFT))
+		return -E2BIG;
+
 	st = kmalloc(sizeof(*st), GFP_KERNEL);
 	if (!st)
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c
index 01e54b45c5c1..795270cb4ec2 100644
--- a/drivers/gpu/drm/i915/gvt/dmabuf.c
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -42,8 +42,7 @@ 
 
 #define GEN8_DECODE_PTE(pte) (pte & GENMASK_ULL(63, 12))
 
-static int vgpu_gem_get_pages(
-		struct drm_i915_gem_object *obj)
+static int vgpu_gem_get_pages(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
 	struct intel_vgpu *vgpu;
@@ -52,7 +51,10 @@  static int vgpu_gem_get_pages(
 	int i, j, ret;
 	gen8_pte_t __iomem *gtt_entries;
 	struct intel_vgpu_fb_info *fb_info;
-	u32 page_num;
+	pgoff_t page_num;
+
+	if (!safe_conversion(&page_num, obj->base.size >> PAGE_SHIFT))
+		return -E2BIG;
 
 	fb_info = (struct intel_vgpu_fb_info *)obj->gvt_info;
 	if (drm_WARN_ON(&dev_priv->drm, !fb_info))
@@ -66,7 +68,6 @@  static int vgpu_gem_get_pages(
 	if (unlikely(!st))
 		return -ENOMEM;
 
-	page_num = obj->base.size >> PAGE_SHIFT;
 	ret = sg_alloc_table(st, page_num, GFP_KERNEL);
 	if (ret) {
 		kfree(st);
diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h b/drivers/gpu/drm/i915/i915_scatterlist.h
index 12c6a1684081..c4d4d3c84cff 100644
--- a/drivers/gpu/drm/i915/i915_scatterlist.h
+++ b/drivers/gpu/drm/i915/i915_scatterlist.h
@@ -218,4 +218,12 @@  struct i915_refct_sgt *i915_rsgt_from_mm_node(const struct drm_mm_node *node,
 struct i915_refct_sgt *i915_rsgt_from_buddy_resource(struct ttm_resource *res,
 						     u64 region_start);
 
+/* Wrap scatterlist.h to sanity check for integer truncation */
+typedef unsigned int __sg_size_t; /* see linux/scatterlist.h */
+#define sg_alloc_table(sgt, nents, gfp) \
+	overflows_type(nents, __sg_size_t) ? -E2BIG : (sg_alloc_table)(sgt, (__sg_size_t)(nents), gfp)
+
+#define sg_alloc_table_from_pages_segment(sgt, pages, npages, offset, size, max_segment, gfp) \
+	overflows_type(npages, __sg_size_t) ? -E2BIG : (sg_alloc_table_from_pages_segment)(sgt, pages, (__sg_size_t)(npages), offset, size, max_segment, gfp)
+
 #endif