Quoting Tvrtko Ursulin (2024-05-21 13:12:01) > From: Tvrtko Ursulin <[email protected]> > > 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 <[email protected]> > Reported-by: kernel test robot <[email protected]> > Closes: > https://lore.kernel.org/oe-lkp/[email protected] > Cc: Kees Cook <[email protected]> > Cc: Kent Overstreet <[email protected]> > Cc: Joonas Lahtinen <[email protected]> > Cc: Rodrigo Vivi <[email protected]> > --- > 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 <[email protected]> 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 >
