diff mbox series

drm/i915/gem: Support parsing of oversize batches

Message ID 20201015115954.871-1-chris@chris-wilson.co.uk
State New, archived
Headers show
Series drm/i915/gem: Support parsing of oversize batches | expand

Commit Message

Chris Wilson Oct. 15, 2020, 11:59 a.m. UTC
Matthew Auld noted that on more recent systems (such as the parser for
gen9) we may have objects that are larger than expected by the GEM uAPI
(i.e. greater than u32). These objects would have incorrect implicit
batch lengths, causing the parser to reject them for being incomplete,
or worse.

Based on a patch by Matthew Auld.

Reported-by: Matthew Auld <matthew.auld@intel.com>
Fixes: 435e8fc059db ("drm/i915: Allow parsing of unsized batches")
Testcase: igt/gem_exec_params/larger-than-life-batch
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Matthew Auld Oct. 15, 2020, 3:13 p.m. UTC | #1
On 15/10/2020 12:59, Chris Wilson wrote:
> Matthew Auld noted that on more recent systems (such as the parser for
> gen9) we may have objects that are larger than expected by the GEM uAPI
> (i.e. greater than u32). These objects would have incorrect implicit
> batch lengths, causing the parser to reject them for being incomplete,
> or worse.
> 
> Based on a patch by Matthew Auld.
> 
> Reported-by: Matthew Auld <matthew.auld@intel.com>
> Fixes: 435e8fc059db ("drm/i915: Allow parsing of unsized batches")
> Testcase: igt/gem_exec_params/larger-than-life-batch
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 4b09bcd70cf4..44b4558d5e86 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -287,8 +287,8 @@ struct i915_execbuffer {
>   	u64 invalid_flags; /** Set of execobj.flags that are invalid */
>   	u32 context_flags; /** Set of execobj.flags to insert from the ctx */
>   
> +	u64 batch_len; /** Length of batch within object */
>   	u32 batch_start_offset; /** Location within object of batch */
> -	u32 batch_len; /** Length of batch within object */
>   	u32 batch_flags; /** Flags composed for emit_bb_start() */
>   	struct intel_gt_buffer_pool_node *batch_pool; /** pool node for batch buffer */
>   
> @@ -871,6 +871,10 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
>   
>   	if (eb->batch_len == 0)
>   		eb->batch_len = eb->batch->vma->size - eb->batch_start_offset;
> +	if (eb->batch_len == 0) {
> +		drm_dbg(&i915->drm, "Invalid batch length\n");
> +		return -EINVAL;
> +	}

This one should be impossible, or at least we should have hit the 
range_overflows check first?

Anyway, looks good to me,
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Chris Wilson Oct. 15, 2020, 3:22 p.m. UTC | #2
Quoting Matthew Auld (2020-10-15 16:13:07)
> On 15/10/2020 12:59, Chris Wilson wrote:
> > Matthew Auld noted that on more recent systems (such as the parser for
> > gen9) we may have objects that are larger than expected by the GEM uAPI
> > (i.e. greater than u32). These objects would have incorrect implicit
> > batch lengths, causing the parser to reject them for being incomplete,
> > or worse.
> > 
> > Based on a patch by Matthew Auld.
> > 
> > Reported-by: Matthew Auld <matthew.auld@intel.com>
> > Fixes: 435e8fc059db ("drm/i915: Allow parsing of unsized batches")
> > Testcase: igt/gem_exec_params/larger-than-life-batch
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index 4b09bcd70cf4..44b4558d5e86 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -287,8 +287,8 @@ struct i915_execbuffer {
> >       u64 invalid_flags; /** Set of execobj.flags that are invalid */
> >       u32 context_flags; /** Set of execobj.flags to insert from the ctx */
> >   
> > +     u64 batch_len; /** Length of batch within object */
> >       u32 batch_start_offset; /** Location within object of batch */
> > -     u32 batch_len; /** Length of batch within object */
> >       u32 batch_flags; /** Flags composed for emit_bb_start() */
> >       struct intel_gt_buffer_pool_node *batch_pool; /** pool node for batch buffer */
> >   
> > @@ -871,6 +871,10 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> >   
> >       if (eb->batch_len == 0)
> >               eb->batch_len = eb->batch->vma->size - eb->batch_start_offset;
> > +     if (eb->batch_len == 0) {
> > +             drm_dbg(&i915->drm, "Invalid batch length\n");
> > +             return -EINVAL;
> > +     }
> 
> This one should be impossible, or at least we should have hit the 
> range_overflows check first?

It should be impossible, yes. I erred on the side of prudence; one little
check for the security conscious as the ramifications of it going wrong
are nasty.

Odd. I still feel in this instance, a check is better than a bug-on. I
must be unwell.
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 4b09bcd70cf4..44b4558d5e86 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -287,8 +287,8 @@  struct i915_execbuffer {
 	u64 invalid_flags; /** Set of execobj.flags that are invalid */
 	u32 context_flags; /** Set of execobj.flags to insert from the ctx */
 
+	u64 batch_len; /** Length of batch within object */
 	u32 batch_start_offset; /** Location within object of batch */
-	u32 batch_len; /** Length of batch within object */
 	u32 batch_flags; /** Flags composed for emit_bb_start() */
 	struct intel_gt_buffer_pool_node *batch_pool; /** pool node for batch buffer */
 
@@ -871,6 +871,10 @@  static int eb_lookup_vmas(struct i915_execbuffer *eb)
 
 	if (eb->batch_len == 0)
 		eb->batch_len = eb->batch->vma->size - eb->batch_start_offset;
+	if (eb->batch_len == 0) {
+		drm_dbg(&i915->drm, "Invalid batch length\n");
+		return -EINVAL;
+	}
 
 	return 0;
 
@@ -2424,7 +2428,7 @@  static int eb_parse(struct i915_execbuffer *eb)
 	struct drm_i915_private *i915 = eb->i915;
 	struct intel_gt_buffer_pool_node *pool = eb->batch_pool;
 	struct i915_vma *shadow, *trampoline, *batch;
-	unsigned int len;
+	unsigned long len = eb->batch_len;
 	int err;
 
 	if (!eb_use_cmdparser(eb)) {
@@ -2435,7 +2439,6 @@  static int eb_parse(struct i915_execbuffer *eb)
 		goto secure_batch;
 	}
 
-	len = eb->batch_len;
 	if (!CMDPARSER_USES_GGTT(eb->i915)) {
 		/*
 		 * ppGTT backed shadow buffers must be mapped RO, to prevent
@@ -2449,6 +2452,8 @@  static int eb_parse(struct i915_execbuffer *eb)
 	} else {
 		len += I915_CMD_PARSER_TRAMPOLINE_SIZE;
 	}
+	if (len < eb->batch_len)
+		return -EINVAL;
 
 	if (!pool) {
 		pool = intel_gt_get_buffer_pool(eb->engine->gt, len);