| Message ID | 20220714090807.2340818-5-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 |
On Thu, 14 Jul 2022 12:08:04 +0300 Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote: > There is an impedance mismatch between the first/last valid page > frame number of ttm place in unsigned and our memory/page accounting in > unsigned long. > 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 as we switch from unsigned long to > unsigned, we use overflows_type check and report E2BIG or overflow_type > prior to the operation. > > v3: Not to change execution inside a macro. (Mauro) > Add safe_conversion_gem_bug_on() macro and remove temporal > SAFE_CONVERSION() macro. > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Reviewed-by: Nirmoy Das <nirmoy.das@intel.com> LGTM. Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> > --- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 6 +++--- > drivers/gpu/drm/i915/i915_gem.h | 4 ++++ > drivers/gpu/drm/i915/intel_region_ttm.c | 20 +++++++++++++++++--- > 3 files changed, 24 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 9f2be1892b6c..88f2887627dc 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -140,14 +140,14 @@ i915_ttm_place_from_region(const struct intel_memory_region *mr, > if (flags & I915_BO_ALLOC_CONTIGUOUS) > place->flags |= TTM_PL_FLAG_CONTIGUOUS; > if (offset != I915_BO_INVALID_OFFSET) { > - place->fpfn = offset >> PAGE_SHIFT; > - place->lpfn = place->fpfn + (size >> PAGE_SHIFT); > + safe_conversion_gem_bug_on(&place->fpfn, offset >> PAGE_SHIFT); > + safe_conversion_gem_bug_on(&place->lpfn, place->fpfn + (size >> PAGE_SHIFT)); > } else if (mr->io_size && mr->io_size < mr->total) { > if (flags & I915_BO_ALLOC_GPU_ONLY) { > place->flags |= TTM_PL_FLAG_TOPDOWN; > } else { > place->fpfn = 0; > - place->lpfn = mr->io_size >> PAGE_SHIFT; > + safe_conversion_gem_bug_on(&place->lpfn, mr->io_size >> PAGE_SHIFT); > } > } > } > diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h > index 68d8d52bd541..6b673607abee 100644 > --- a/drivers/gpu/drm/i915/i915_gem.h > +++ b/drivers/gpu/drm/i915/i915_gem.h > @@ -83,5 +83,9 @@ struct drm_i915_private; > #endif > > #define I915_GEM_IDLE_TIMEOUT (HZ / 5) > +#define safe_conversion_gem_bug_on(ptr, value) ({ \ > + safe_conversion(ptr, value) ? 1 \ > + : (({ GEM_BUG_ON(overflows_type(value, *ptr)); }), 0); \ > +}) > > #endif /* __I915_GEM_H__ */ > diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c > index 575d67bc6ffe..f0d143948725 100644 > --- a/drivers/gpu/drm/i915/intel_region_ttm.c > +++ b/drivers/gpu/drm/i915/intel_region_ttm.c > @@ -209,14 +209,26 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem, > if (flags & I915_BO_ALLOC_CONTIGUOUS) > place.flags |= TTM_PL_FLAG_CONTIGUOUS; > if (offset != I915_BO_INVALID_OFFSET) { > - place.fpfn = offset >> PAGE_SHIFT; > - place.lpfn = place.fpfn + (size >> PAGE_SHIFT); > + if (!safe_conversion_gem_bug_on(&place.fpfn, > + offset >> PAGE_SHIFT)) { > + ret = -E2BIG; > + goto out; > + } > + if (!safe_conversion_gem_bug_on(&place.lpfn, > + place.fpfn + (size >> PAGE_SHIFT))) { > + ret = -E2BIG; > + goto out; > + } > } else if (mem->io_size && mem->io_size < mem->total) { > if (flags & I915_BO_ALLOC_GPU_ONLY) { > place.flags |= TTM_PL_FLAG_TOPDOWN; > } else { > place.fpfn = 0; > - place.lpfn = mem->io_size >> PAGE_SHIFT; > + if (!safe_conversion_gem_bug_on(&place.lpfn, > + mem->io_size >> PAGE_SHIFT)) { > + ret = -E2BIG; > + goto out; > + } > } > } > > @@ -224,6 +236,8 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem, > mock_bo.bdev = &mem->i915->bdev; > > ret = man->func->alloc(man, &mock_bo, &place, &res); > + > +out: > if (ret == -ENOSPC) > ret = -ENXIO; > if (!ret)
Hi Gwan-gyeong,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
url: https://github.com/intel-lab-lkp/linux/commits/Gwan-gyeong-Mun/Fixes-integer-overflow-or-integer-truncation-issues-in-page-lookups-ttm-place-configuration-and-scatterlist-creation/20220714-171019
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: x86_64-randconfig-a011
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/04722b3900e636ffd9041c86cf58815f2a81a8b2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Gwan-gyeong-Mun/Fixes-integer-overflow-or-integer-truncation-issues-in-page-lookups-ttm-place-configuration-and-scatterlist-creation/20220714-171019
git checkout 04722b3900e636ffd9041c86cf58815f2a81a8b2
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/i915/gt/intel_engine_types.h:20,
from drivers/gpu/drm/i915/gt/intel_context_types.h:18,
from drivers/gpu/drm/i915/gem/i915_gem_context_types.h:20,
from drivers/gpu/drm/i915/i915_request.h:34,
from drivers/gpu/drm/i915/i915_active.h:13,
from drivers/gpu/drm/i915/gem/i915_gem_object_types.h:16,
from drivers/gpu/drm/i915/display/intel_frontbuffer.h:30,
from drivers/gpu/drm/i915/i915_drv.h:47,
from drivers/gpu/drm/i915/gem/i915_gem_ttm.c:12:
drivers/gpu/drm/i915/gem/i915_gem_ttm.c: In function 'i915_ttm_place_from_region':
>> drivers/gpu/drm/i915/i915_gem.h:88:66: warning: left-hand operand of comma expression has no effect [-Wunused-value]
88 | : (({ GEM_BUG_ON(overflows_type(value, *ptr)); }), 0); \
| ^
drivers/gpu/drm/i915/gem/i915_gem_ttm.c:143:17: note: in expansion of macro 'safe_conversion_gem_bug_on'
143 | safe_conversion_gem_bug_on(&place->fpfn, offset >> PAGE_SHIFT);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/i915/i915_gem.h:88:66: warning: left-hand operand of comma expression has no effect [-Wunused-value]
88 | : (({ GEM_BUG_ON(overflows_type(value, *ptr)); }), 0); \
| ^
drivers/gpu/drm/i915/gem/i915_gem_ttm.c:144:17: note: in expansion of macro 'safe_conversion_gem_bug_on'
144 | safe_conversion_gem_bug_on(&place->lpfn, place->fpfn + (size >> PAGE_SHIFT));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/i915/i915_gem.h:88:66: warning: left-hand operand of comma expression has no effect [-Wunused-value]
88 | : (({ GEM_BUG_ON(overflows_type(value, *ptr)); }), 0); \
| ^
drivers/gpu/drm/i915/gem/i915_gem_ttm.c:150:25: note: in expansion of macro 'safe_conversion_gem_bug_on'
150 | safe_conversion_gem_bug_on(&place->lpfn, mr->io_size >> PAGE_SHIFT);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
vim +88 drivers/gpu/drm/i915/i915_gem.h
84
85 #define I915_GEM_IDLE_TIMEOUT (HZ / 5)
86 #define safe_conversion_gem_bug_on(ptr, value) ({ \
87 safe_conversion(ptr, value) ? 1 \
> 88 : (({ GEM_BUG_ON(overflows_type(value, *ptr)); }), 0); \
89 })
90
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 9f2be1892b6c..88f2887627dc 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -140,14 +140,14 @@ i915_ttm_place_from_region(const struct intel_memory_region *mr, if (flags & I915_BO_ALLOC_CONTIGUOUS) place->flags |= TTM_PL_FLAG_CONTIGUOUS; if (offset != I915_BO_INVALID_OFFSET) { - place->fpfn = offset >> PAGE_SHIFT; - place->lpfn = place->fpfn + (size >> PAGE_SHIFT); + safe_conversion_gem_bug_on(&place->fpfn, offset >> PAGE_SHIFT); + safe_conversion_gem_bug_on(&place->lpfn, place->fpfn + (size >> PAGE_SHIFT)); } else if (mr->io_size && mr->io_size < mr->total) { if (flags & I915_BO_ALLOC_GPU_ONLY) { place->flags |= TTM_PL_FLAG_TOPDOWN; } else { place->fpfn = 0; - place->lpfn = mr->io_size >> PAGE_SHIFT; + safe_conversion_gem_bug_on(&place->lpfn, mr->io_size >> PAGE_SHIFT); } } } diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h index 68d8d52bd541..6b673607abee 100644 --- a/drivers/gpu/drm/i915/i915_gem.h +++ b/drivers/gpu/drm/i915/i915_gem.h @@ -83,5 +83,9 @@ struct drm_i915_private; #endif #define I915_GEM_IDLE_TIMEOUT (HZ / 5) +#define safe_conversion_gem_bug_on(ptr, value) ({ \ + safe_conversion(ptr, value) ? 1 \ + : (({ GEM_BUG_ON(overflows_type(value, *ptr)); }), 0); \ +}) #endif /* __I915_GEM_H__ */ diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c index 575d67bc6ffe..f0d143948725 100644 --- a/drivers/gpu/drm/i915/intel_region_ttm.c +++ b/drivers/gpu/drm/i915/intel_region_ttm.c @@ -209,14 +209,26 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem, if (flags & I915_BO_ALLOC_CONTIGUOUS) place.flags |= TTM_PL_FLAG_CONTIGUOUS; if (offset != I915_BO_INVALID_OFFSET) { - place.fpfn = offset >> PAGE_SHIFT; - place.lpfn = place.fpfn + (size >> PAGE_SHIFT); + if (!safe_conversion_gem_bug_on(&place.fpfn, + offset >> PAGE_SHIFT)) { + ret = -E2BIG; + goto out; + } + if (!safe_conversion_gem_bug_on(&place.lpfn, + place.fpfn + (size >> PAGE_SHIFT))) { + ret = -E2BIG; + goto out; + } } else if (mem->io_size && mem->io_size < mem->total) { if (flags & I915_BO_ALLOC_GPU_ONLY) { place.flags |= TTM_PL_FLAG_TOPDOWN; } else { place.fpfn = 0; - place.lpfn = mem->io_size >> PAGE_SHIFT; + if (!safe_conversion_gem_bug_on(&place.lpfn, + mem->io_size >> PAGE_SHIFT)) { + ret = -E2BIG; + goto out; + } } } @@ -224,6 +236,8 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem, mock_bo.bdev = &mem->i915->bdev; ret = man->func->alloc(man, &mock_bo, &place, &res); + +out: if (ret == -ENOSPC) ret = -ENXIO; if (!ret)
There is an impedance mismatch between the first/last valid page frame number of ttm place in unsigned and our memory/page accounting in unsigned long. 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 as we switch from unsigned long to unsigned, we use overflows_type check and report E2BIG or overflow_type prior to the operation. v3: Not to change execution inside a macro. (Mauro) Add safe_conversion_gem_bug_on() macro and remove temporal SAFE_CONVERSION() macro. Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> 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_ttm.c | 6 +++--- drivers/gpu/drm/i915/i915_gem.h | 4 ++++ drivers/gpu/drm/i915/intel_region_ttm.c | 20 +++++++++++++++++--- 3 files changed, 24 insertions(+), 6 deletions(-)