[PATCH] drm/i915: 2 GiB of relocations ought to be enough for anybody*

[Joonas Lahtinen]

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