| Message ID | 20220603093830.1529520-1-gwan-gyeong.mun@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation | expand |
On Fri, 03 Jun 2022, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote: > This patch series fixes integer overflow or integer truncation issues in > page lookups, ttm place configuration and scatterlist creation, etc. > We need to check that we avoid integer overflows when looking up a page, > and so fix all the instances where we have mistakenly used a plain integer > instead of a more suitable long. So when are we going to start moving the helpers, both existing and the ones being added here, from i915_utils.h to proper kernel headers? We just keep adding more and more. This needs to stop. BR, Jani.
On 6/3/22 4:19 PM, Jani Nikula wrote: > On Fri, 03 Jun 2022, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote: >> This patch series fixes integer overflow or integer truncation issues in >> page lookups, ttm place configuration and scatterlist creation, etc. >> We need to check that we avoid integer overflows when looking up a page, >> and so fix all the instances where we have mistakenly used a plain integer >> instead of a more suitable long. > > So when are we going to start moving the helpers, both existing and the > ones being added here, from i915_utils.h to proper kernel headers? We > just keep adding more and more. This needs to stop. > > BR, > Jani. > Thanks for your comments. The following two macros have been added to i915_utils.h in this patch series. #define exactly_pgoff_t(n) exact_type(pgoff_t, n) The reason this macro was added is that there was a possibility that an overflow could occur in the code that is passed as a function parameter in the form of "offset >> PAGE_SHIFT" for the offset variable of unsigned int type in the code. Therefore this macro added so that we can check this part at the code level. After checking all of this type of overflow problem in the i915 driver code, it is thought that it will be possible to remove it through refactoring in the future. So, in my opinion, there is no need to move this macro to include/linux/types.h or other header files. #define safe_conversion(ptr, value) ({ \ typeof(value) __v = (value); \ typeof(ptr) __ptr = (ptr); \ overflows_type(__v, *__ptr) ? 0 : (*__ptr = (typeof(*__ptr))__v), 1; \ }) The above macro depends on overflows_type(), and overflows_type() is defined in i915_utils.h in the same way. To move the safe_conversion() macro to a commonly available header, the overflows_type() macro must also be moved, and if the name or type of this macro is changed, all other code using overflows_type() will be affected. In this patch, safe_conversion() is added to i915_utils.h, and it seems to be prepared and moved to the next patch. If you absolutely need to move the safe_conversion() macro to a common header in this patch series, please let me know and I will update this patch series. Br, G.G.
This patch series fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation, etc. We need to check that we avoid integer overflows when looking up a page, and so fix all the instances where we have mistakenly used a plain integer instead of a more suitable long. And 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 as we switch from unsigned long into the scatterlist's unsigned int, we use our 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. And ttm place also has the same problem with scatterlist creation, and we fix the integer truncation problem with the way approached by scatterlist creation. And It corrects the error code to return -E2BIG when creating gem objects using ttm or shmem, if the size is too large in each case. Testcase: igt@gem_create@create-massive Testcase: igt@gem_userptr_blits@input-checking Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4991 Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5411 Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Matthew Auld <matthew.auld@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Chris Wilson (3): drm/i915/gem: Typecheck page lookups drm/i915: Check for integer truncation on scatterlist creation drm/i915: Remove truncation warning for large objects Gwan-gyeong Mun (3): drm/i915: Check for integer truncation on the configuration of ttm place drm/i915: Check if the size is too big while creating shmem file drm/i915: Use error code as -E2BIG when the size of gem ttm object is too large drivers/gpu/drm/i915/gem/i915_gem_internal.c | 6 +- drivers/gpu/drm/i915/gem/i915_gem_object.c | 7 +- drivers/gpu/drm/i915/gem/i915_gem_object.h | 77 +++++++++++-------- drivers/gpu/drm/i915/gem/i915_gem_pages.c | 25 +++--- drivers/gpu/drm/i915/gem/i915_gem_phys.c | 4 + drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 14 +++- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 29 ++++++- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 5 +- .../drm/i915/gem/selftests/i915_gem_context.c | 12 +-- .../drm/i915/gem/selftests/i915_gem_mman.c | 8 +- .../drm/i915/gem/selftests/i915_gem_object.c | 8 +- drivers/gpu/drm/i915/gvt/dmabuf.c | 9 ++- drivers/gpu/drm/i915/i915_gem.c | 18 ++++- drivers/gpu/drm/i915/i915_scatterlist.h | 8 ++ drivers/gpu/drm/i915/i915_utils.h | 18 +++++ drivers/gpu/drm/i915/i915_vma.c | 8 +- drivers/gpu/drm/i915/intel_region_ttm.c | 16 +++- 17 files changed, 189 insertions(+), 83 deletions(-)