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
