Quoting Tvrtko Ursulin (2024-05-21 13:12:01)
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-lkp/202405151008.6ddd1aaf-oliver.sang@xxxxxxxxx
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Kent Overstreet <kent.overstreet@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
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
>
