[Intel-gfx] [PATCH] drm/i915/execbuf: don't allow zero batch_len

[Chris Wilson]

Quoting Matthew Auld (2020-10-13 12:18:39)
> As per the ABI batch_len is u32, however if the batch_len is left unset,
> then the kernel will just assume batch_len is the size of the whole
> batch object, however since the vma->size is u64, while the batch_len is
> just u32 we can end up with batch_len = 0 if we are given too large batch
> object(e.g 1ULL << 32), which doesn't look the intended behaviour and
> probably leads to explosions on some HW.
> 
> Testcase: igt/gem_exec_params/larger-than-life-batch
> Fixes: 0b5372727be3 ("drm/i915/cmdparser: Use cached vmappings")

Nah. That's setting exec_len used for dispatch, not for parsing, which
is still using 

i915_gem_execbuffer_parse(engine, &shadow_exec_entry,
			  params->batch->obj,
			  eb,
			  args->batch_start_offset,
			  args->batch_len,
			  drm_is_current_master(file));
(and args->batch_len is straight from userspace and passed onwards)

It's right up until 435e8fc059db ("drm/i915: Allow parsing of unsized batches")
where we are using the user value of batch_len for allocating the shadow
object and parsing.

Fixes: 435e8fc059db ("drm/i915: Allow parsing of unsized batches")

> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 4b09bcd70cf4..80c738c72e6e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -869,8 +869,13 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
>                 return -EINVAL;
>         }
>  
> -       if (eb->batch_len == 0)
> +       if (eb->batch_len == 0) {
>                 eb->batch_len = eb->batch->vma->size - eb->batch_start_offset;

if (overflows_type(eb->batch->vma->size - eb->batch_start_offset, eb->batch_len))

It should not have caused the cmdparser any trouble though, it should
have been quite happy to copy nothing and reject the batch for reaching
the end too early (with a very slim chance of a stale
MI_BATCH_BUFFER_END to the rescue).

intel_gt_get_buffer_pool() looks suspect given a size of 0, it will
either give the largest object it has cached or break upon
creating/allocating internal pages.

In terms of HW fail, only gen2 used the parameter and it has a very
limited batch/GTT size.
-Chris