From: Jani Nikula <jani.nikula@intel.com>
To: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: thomas.hellstrom@linux.intel.com,
intel-gfx@lists.freedesktop.org, chris@chris-wilson.co.uk,
matthew.auld@intel.com, andrzej.hajda@intel.com,
mchehab@kernel.org, nirmoy.das@intel.com
Subject: Re: [Intel-gfx] [PATCH v14 3/7] drm/i915: Check for integer truncation on scatterlist creation
Date: Thu, 03 Nov 2022 12:51:02 +0200 [thread overview]
Message-ID: <87v8nwb9bd.fsf@intel.com> (raw)
In-Reply-To: <5262267d-0d31-4936-57e4-0cf5ad80a605@intel.com>
On Thu, 03 Nov 2022, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote:
> On 11/2/22 4:53 PM, Gwan-gyeong Mun 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 the control of userspace, we have to be prudent and catch the
>> conversion errors.
>>
>> To catch the implicit truncation we check before calling scattterlist
>> creation Apis. we use overflows_type check and report E2BIG if the
>> overflows may raise. When caller does not return errno, use WARN_ON to
>> report a problem.
>>
>> 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.
>>
>> v2: Move added i915_utils's macro into drm_util header (Jani N)
>> v5: Fix macros to be enclosed in parentheses for complex values
>> Fix too long line warning
>> v8: Replace safe_conversion() with check_assign() (Kees)
>> v14: Remove shadowing macros of scatterlist creation api and fix to
>> explicitly overflow check where the scatterlist creation APIs are
>> called. (Jani)
>>
> Hi Jani,
>
> This version has removed shadowing macros of scatterlist creation api as
> per last comment of you.
>
> Can I please have your ack or review comment?
I don't get the parts that have WARN_ON() but no action, they just keep
going.
Other than that,
Acked-by: Jani Nikula <jani.nikula@intel.com>
>
> Br,
>
> G.G.
>
>> 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>
>> Co-developed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
>> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_internal.c | 7 +++++--
>> 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 | 9 ++++++---
>> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 4 ++++
>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 6 +++++-
>> drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c | 6 +++++-
>> drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 8 ++++++++
>> drivers/gpu/drm/i915/gvt/dmabuf.c | 10 ++++++----
>> drivers/gpu/drm/i915/i915_scatterlist.c | 5 +++++
>> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 ++++
>> drivers/gpu/drm/i915/selftests/scatterlist.c | 4 ++++
>> 12 files changed, 56 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
>> index 629acb403a2c..b0e6b464bf59 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
>> @@ -36,11 +36,15 @@ 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;
>> + unsigned int npages; /* restricted by sg_alloc_table */
>> int max_order = MAX_ORDER;
>> unsigned int max_segment;
>> gfp_t gfp;
>>
>> + if (overflows_type(obj->base.size >> PAGE_SHIFT, npages))
>> + return -E2BIG;
>> +
>> + npages = obj->base.size >> PAGE_SHIFT;
>> max_segment = i915_sg_segment_size(i915->drm.dev) >> PAGE_SHIFT;
>> max_order = min(max_order, get_order(max_segment));
>>
>> @@ -56,7 +60,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 55817d287676..8cd8d2041c5a 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 11125c32dd35..fdd9d151ac1f 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> @@ -60,7 +60,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>> struct address_space *mapping,
>> unsigned int max_segment)
>> {
>> - const unsigned long page_count = size / PAGE_SIZE;
>> + unsigned int page_count; /* restricted by sg_alloc_table */
>> unsigned long i;
>> struct scatterlist *sg;
>> struct page *page;
>> @@ -68,6 +68,10 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>> gfp_t noreclaim;
>> int ret;
>>
>> + if (overflows_type(size / PAGE_SIZE, page_count))
>> + return -E2BIG;
>> +
>> + page_count = size / PAGE_SIZE;
>> /*
>> * If there's no chance of allocating enough pages for the whole
>> * object, bail early.
>> @@ -193,7 +197,6 @@ 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(i915->drm.dev);
>> struct sg_table *st;
>> struct sgt_iter sgt_iter;
>> @@ -236,7 +239,7 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj)
>> } else {
>> dev_warn(i915->drm.dev,
>> "Failed to DMA remap %lu pages\n",
>> - page_count);
>> + obj->base.size >> PAGE_SHIFT);
>> goto err_pages;
>> }
>> }
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 721b716942bb..2cd7a17c720d 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -829,6 +829,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;
>>
>> + /* restricted by sg_alloc_table */
>> + if (overflows_type(obj->base.size >> PAGE_SHIFT, unsigned int))
>> + return -E2BIG;
>> +
>> GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS);
>>
>> /* Move to the requested placement. */
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> index 1b1a22716722..893c03f4a794 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> @@ -128,13 +128,17 @@ 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(obj->base.dev->dev);
>> struct sg_table *st;
>> unsigned int sg_page_sizes;
>> struct page **pvec;
>> + unsigned int num_pages; /* limited by sg_alloc_table_from_pages_segment */
>> int ret;
>>
>> + if (overflows_type(obj->base.size >> PAGE_SHIFT, num_pages))
>> + return -E2BIG;
>> +
>> + num_pages = obj->base.size >> PAGE_SHIFT;
>> st = kmalloc(sizeof(*st), GFP_KERNEL);
>> if (!st)
>> return -ENOMEM;
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
>> index f963b8e1e37b..bb1e8f1657a6 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
>> @@ -29,11 +29,15 @@ static int huge_get_pages(struct drm_i915_gem_object *obj)
>> {
>> #define GFP (GFP_KERNEL | __GFP_NOWARN | __GFP_RETRY_MAYFAIL)
>> const unsigned long nreal = obj->scratch / PAGE_SIZE;
>> - const unsigned long npages = obj->base.size / PAGE_SIZE;
>> + unsigned int npages; /* restricted by sg_alloc_table */
>> struct scatterlist *sg, *src, *end;
>> struct sg_table *pages;
>> unsigned long n;
>>
>> + if (overflows_type(obj->base.size / PAGE_SIZE, npages))
>> + return -E2BIG;
>> +
>> + npages = obj->base.size / PAGE_SIZE;
>> pages = kmalloc(sizeof(*pages), GFP);
>> if (!pages)
>> return -ENOMEM;
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>> index 0cb99e75b0bc..1120a7960d47 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>> @@ -84,6 +84,10 @@ static int get_huge_pages(struct drm_i915_gem_object *obj)
>> unsigned int sg_page_sizes;
>> u64 rem;
>>
>> + /* restricted by sg_alloc_table */
>> + if (overflows_type(obj->base.size >> PAGE_SHIFT, unsigned int))
>> + return -E2BIG;
>> +
>> st = kmalloc(sizeof(*st), GFP);
>> if (!st)
>> return -ENOMEM;
>> @@ -213,6 +217,10 @@ static int fake_get_huge_pages(struct drm_i915_gem_object *obj)
>> unsigned int sg_page_sizes;
>> u64 rem;
>>
>> + /* restricted by sg_alloc_table */
>> + if (overflows_type(obj->base.size >> PAGE_SHIFT, unsigned int))
>> + return -E2BIG;
>> +
>> st = kmalloc(sizeof(*st), GFP);
>> if (!st)
>> return -ENOMEM;
>> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c
>> index 01e54b45c5c1..e61cf3beeeba 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,8 +51,12 @@ 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;
>> + unsigned int page_num; /* limited by sg_alloc_table */
>>
>> + if (overflows_type(obj->base.size >> PAGE_SHIFT, page_num))
>> + return -E2BIG;
>> +
>> + page_num = obj->base.size >> PAGE_SHIFT;
>> fb_info = (struct intel_vgpu_fb_info *)obj->gvt_info;
>> if (drm_WARN_ON(&dev_priv->drm, !fb_info))
>> return -ENODEV;
>> @@ -66,7 +69,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.c b/drivers/gpu/drm/i915/i915_scatterlist.c
>> index 114e5e39aa72..c9dc3a917d66 100644
>> --- a/drivers/gpu/drm/i915/i915_scatterlist.c
>> +++ b/drivers/gpu/drm/i915/i915_scatterlist.c
>> @@ -96,6 +96,9 @@ struct i915_refct_sgt *i915_rsgt_from_mm_node(const struct drm_mm_node *node,
>>
>> i915_refct_sgt_init(rsgt, node->size << PAGE_SHIFT);
>> st = &rsgt->table;
>> + /* restricted by sg_alloc_table */
>> + WARN_ON(overflows_type(DIV_ROUND_UP_ULL(node->size, segment_pages),
>> + unsigned int));
>> if (sg_alloc_table(st, DIV_ROUND_UP_ULL(node->size, segment_pages),
>> GFP_KERNEL)) {
>> i915_refct_sgt_put(rsgt);
>> @@ -177,6 +180,8 @@ struct i915_refct_sgt *i915_rsgt_from_buddy_resource(struct ttm_resource *res,
>>
>> i915_refct_sgt_init(rsgt, size);
>> st = &rsgt->table;
>> + /* restricted by sg_alloc_table */
>> + WARN_ON(overflows_type(PFN_UP(res->size), unsigned int));
>> if (sg_alloc_table(st, PFN_UP(res->size), GFP_KERNEL)) {
>> i915_refct_sgt_put(rsgt);
>> return ERR_PTR(-ENOMEM);
>> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
>> index 27c733b00976..097be1e89dba 100644
>> --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
>> @@ -69,6 +69,10 @@ static int fake_get_pages(struct drm_i915_gem_object *obj)
>> return -ENOMEM;
>>
>> rem = round_up(obj->base.size, BIT(31)) >> 31;
>> + /* restricted by sg_alloc_table */
>> + if (overflows_type(rem, unsigned int))
>> + return -E2BIG;
>> +
>> if (sg_alloc_table(pages, rem, GFP)) {
>> kfree(pages);
>> return -ENOMEM;
>> diff --git a/drivers/gpu/drm/i915/selftests/scatterlist.c b/drivers/gpu/drm/i915/selftests/scatterlist.c
>> index d599186d5b71..805c4bfb85fe 100644
>> --- a/drivers/gpu/drm/i915/selftests/scatterlist.c
>> +++ b/drivers/gpu/drm/i915/selftests/scatterlist.c
>> @@ -220,6 +220,10 @@ static int alloc_table(struct pfn_table *pt,
>> struct scatterlist *sg;
>> unsigned long n, pfn;
>>
>> + /* restricted by sg_alloc_table */
>> + if (overflows_type(max, unsigned int))
>> + return -E2BIG;
>> +
>> if (sg_alloc_table(&pt->st, max,
>> GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN))
>> return alloc_error;
--
Jani Nikula, Intel Open Source Graphics Center
next prev parent reply other threads:[~2022-11-03 10:51 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-02 14:53 [Intel-gfx] [PATCH v14 0/7] Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation Gwan-gyeong Mun
2022-11-02 14:53 ` [Intel-gfx] [PATCH v14 1/7] overflow: Introduce overflows_type() and castable_to_type() Gwan-gyeong Mun
2022-11-03 9:48 ` Gwan-gyeong Mun
2022-11-02 14:53 ` [Intel-gfx] [PATCH v14 2/7] drm/i915/gem: Typecheck page lookups Gwan-gyeong Mun
2022-11-02 14:53 ` [Intel-gfx] [PATCH v14 3/7] drm/i915: Check for integer truncation on scatterlist creation Gwan-gyeong Mun
2022-11-03 10:30 ` Gwan-gyeong Mun
2022-11-03 10:51 ` Jani Nikula [this message]
2022-11-02 14:53 ` [Intel-gfx] [PATCH v14 4/7] drm/i915: Check for integer truncation on the configuration of ttm place Gwan-gyeong Mun
2022-11-02 14:54 ` [Intel-gfx] [PATCH v14 5/7] drm/i915: Check if the size is too big while creating shmem file Gwan-gyeong Mun
2022-11-02 14:54 ` [Intel-gfx] [PATCH v14 6/7] drm/i915: Use error code as -E2BIG when the size of gem ttm object is too large Gwan-gyeong Mun
2022-11-02 14:54 ` [Intel-gfx] [PATCH v14 7/7] drm/i915: Remove truncation warning for large objects Gwan-gyeong Mun
2022-11-02 16:21 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation Patchwork
2022-11-02 16:21 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2022-11-02 16:43 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2022-11-02 20:56 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2022-11-03 6:46 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation (rev2) Patchwork
2022-11-03 7:07 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2022-11-03 10:22 ` Gwan-gyeong Mun
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87v8nwb9bd.fsf@intel.com \
--to=jani.nikula@intel.com \
--cc=andrzej.hajda@intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=gwan-gyeong.mun@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=matthew.auld@intel.com \
--cc=mchehab@kernel.org \
--cc=nirmoy.das@intel.com \
--cc=thomas.hellstrom@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox