| Message ID | 20240521101201.18978-1-tursulin@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: 2 GiB of relocations ought to be enough for anybody* | expand |
On Tue, May 21, 2024 at 11:12:01AM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > Kernel test robot reports i915 can hit a warn in kvmalloc_node which has > a purpose of dissalowing crazy size kernel allocations. This was added in > 7661809d493b ("mm: don't allow oversized kvmalloc() calls"): > > /* Don't even allow crazy sizes */ > if (WARN_ON_ONCE(size > INT_MAX)) > return NULL; > > This would be kind of okay since i915 at one point dropped the need for > making a shadow copy of the relocation list, but then it got re-added in > fd1500fcd442 ("Revert "drm/i915/gem: Drop relocation slowpath".") a year > after Linus added the above warning. > > It is plausible that the issue was not seen until now because to trigger > gem_exec_reloc test requires a combination of an relatively older > generation hardware but with at least 8GiB of RAM installed. Probably even > more depending on runtime checks. > > Lets cap what we allow userspace to pass in using the matching limit. > There should be no issue for real userspace since we are talking about > "crazy" number of relocations which have no practical purpose. > > *) Well IGT tests might get upset but they can be easily adjusted. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> Thanks for fixing this! Reviewed-by: Kees Cook <keescook@chromium.org>
Quoting Tvrtko Ursulin (2024-05-21 13:12:01) > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > Kernel test robot reports i915 can hit a warn in kvmalloc_node which has > a purpose of dissalowing crazy size kernel allocations. This was added in > 7661809d493b ("mm: don't allow oversized kvmalloc() calls"): > > /* Don't even allow crazy sizes */ > if (WARN_ON_ONCE(size > INT_MAX)) > return NULL; > > This would be kind of okay since i915 at one point dropped the need for > making a shadow copy of the relocation list, but then it got re-added in > fd1500fcd442 ("Revert "drm/i915/gem: Drop relocation slowpath".") a year > after Linus added the above warning. > > It is plausible that the issue was not seen until now because to trigger > gem_exec_reloc test requires a combination of an relatively older > generation hardware but with at least 8GiB of RAM installed. Probably even > more depending on runtime checks. > > Lets cap what we allow userspace to pass in using the matching limit. > There should be no issue for real userspace since we are talking about > "crazy" number of relocations which have no practical purpose. > > *) Well IGT tests might get upset but they can be easily adjusted. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > Reported-by: kernel test robot <oliver.sang@intel.com> > Closes: https://lore.kernel.org/oe-lkp/202405151008.6ddd1aaf-oliver.sang@intel.com > Cc: Kees Cook <keescook@chromium.org> > Cc: Kent Overstreet <kent.overstreet@linux.dev> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index d3a771afb083..4b34bf4fde77 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -1533,7 +1533,7 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev) > u64_to_user_ptr(entry->relocs_ptr); > unsigned long remain = entry->relocation_count; > > - if (unlikely(remain > N_RELOC(ULONG_MAX))) > + if (unlikely(remain > N_RELOC(INT_MAX))) > return -EINVAL; Yeah, nobody will realistically need that many relocations. Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas > > /* > @@ -1641,7 +1641,7 @@ static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) > if (size == 0) > return 0; > > - if (size > N_RELOC(ULONG_MAX)) > + if (size > N_RELOC(INT_MAX)) > return -EINVAL; > > addr = u64_to_user_ptr(entry->relocs_ptr); > -- > 2.44.0 >
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index d3a771afb083..4b34bf4fde77 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1533,7 +1533,7 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev) u64_to_user_ptr(entry->relocs_ptr); unsigned long remain = entry->relocation_count; - if (unlikely(remain > N_RELOC(ULONG_MAX))) + if (unlikely(remain > N_RELOC(INT_MAX))) return -EINVAL; /* @@ -1641,7 +1641,7 @@ static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) if (size == 0) return 0; - if (size > N_RELOC(ULONG_MAX)) + if (size > N_RELOC(INT_MAX)) return -EINVAL; addr = u64_to_user_ptr(entry->relocs_ptr);