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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue,  5 Jul 2022 15:24:51 +0300
Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx> wrote:

> From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Brian Welty <brian.welty@xxxxxxxxx>
> Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> Reviewed-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>

Reviewed-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>

> ---
>  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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux