All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo
@ 2015-11-20 15:11 ` Chris Wilson
  0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2015-11-20 15:11 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson, stable

The offset within and the length of the command sequence to execute are
supplied by the user with respect to the batch buffer. We should be
validating that region is wholly contained within the batch buffer;
make it so.

Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a4c243cec4aa..e38284c1b89f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1462,6 +1462,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	/* take note of the batch buffer before we might reorder the lists */
 	batch_obj = eb_get_batch(eb);
 
+	if (args->batch_len > batch_obj->base.size ||
+	    args->batch_start_offset > batch_obj->base.size - args->batch_len) {
+		DRM_DEBUG("Attempting to execute commands from beyond the bounds of the batch object\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
 	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, ctx, &need_relocs);
-- 
2.6.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo
@ 2015-11-20 15:11 ` Chris Wilson
  0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2015-11-20 15:11 UTC (permalink / raw)
  To: intel-gfx; +Cc: stable

The offset within and the length of the command sequence to execute are
supplied by the user with respect to the batch buffer. We should be
validating that region is wholly contained within the batch buffer;
make it so.

Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a4c243cec4aa..e38284c1b89f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1462,6 +1462,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	/* take note of the batch buffer before we might reorder the lists */
 	batch_obj = eb_get_batch(eb);
 
+	if (args->batch_len > batch_obj->base.size ||
+	    args->batch_start_offset > batch_obj->base.size - args->batch_len) {
+		DRM_DEBUG("Attempting to execute commands from beyond the bounds of the batch object\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
 	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, ctx, &need_relocs);
-- 
2.6.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo
  2015-11-20 15:11 ` Chris Wilson
@ 2015-11-20 15:38   ` Ville Syrjälä
  -1 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2015-11-20 15:38 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, stable

On Fri, Nov 20, 2015 at 03:11:04PM +0000, Chris Wilson wrote:
> The offset within and the length of the command sequence to execute are
> supplied by the user with respect to the batch buffer. We should be
> validating that region is wholly contained within the batch buffer;
> make it so.
> 
> Reported-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a4c243cec4aa..e38284c1b89f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1462,6 +1462,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	/* take note of the batch buffer before we might reorder the lists */
>  	batch_obj = eb_get_batch(eb);
>  
> +	if (args->batch_len > batch_obj->base.size ||
> +	    args->batch_start_offset > batch_obj->base.size - args->batch_len) {

lgtm. No possibility of overflow doing it that way.

Reviewed-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>

> +		DRM_DEBUG("Attempting to execute commands from beyond the bounds of the batch object\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
>  	/* Move the objects en-masse into the GTT, evicting if necessary. */
>  	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
>  	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, ctx, &need_relocs);
> -- 
> 2.6.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrj�l�
Intel OTC

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo
@ 2015-11-20 15:38   ` Ville Syrjälä
  0 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2015-11-20 15:38 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, stable

On Fri, Nov 20, 2015 at 03:11:04PM +0000, Chris Wilson wrote:
> The offset within and the length of the command sequence to execute are
> supplied by the user with respect to the batch buffer. We should be
> validating that region is wholly contained within the batch buffer;
> make it so.
> 
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a4c243cec4aa..e38284c1b89f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1462,6 +1462,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	/* take note of the batch buffer before we might reorder the lists */
>  	batch_obj = eb_get_batch(eb);
>  
> +	if (args->batch_len > batch_obj->base.size ||
> +	    args->batch_start_offset > batch_obj->base.size - args->batch_len) {

lgtm. No possibility of overflow doing it that way.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		DRM_DEBUG("Attempting to execute commands from beyond the bounds of the batch object\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
>  	/* Move the objects en-masse into the GTT, evicting if necessary. */
>  	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
>  	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, ctx, &need_relocs);
> -- 
> 2.6.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo
  2015-11-20 15:38   ` Ville Syrjälä
  (?)
@ 2016-04-28  8:51   ` Jani Nikula
  2016-04-28  8:54       ` Jani Nikula
  -1 siblings, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2016-04-28  8:51 UTC (permalink / raw)
  To: Ville Syrjälä, Chris Wilson; +Cc: intel-gfx, stable

On Fri, 20 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Nov 20, 2015 at 03:11:04PM +0000, Chris Wilson wrote:
>> The offset within and the length of the command sequence to execute are
>> supplied by the user with respect to the batch buffer. We should be
>> validating that region is wholly contained within the batch buffer;
>> make it so.
>> 
>> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index a4c243cec4aa..e38284c1b89f 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -1462,6 +1462,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>  	/* take note of the batch buffer before we might reorder the lists */
>>  	batch_obj = eb_get_batch(eb);
>>  
>> +	if (args->batch_len > batch_obj->base.size ||
>> +	    args->batch_start_offset > batch_obj->base.size - args->batch_len) {
>
> lgtm. No possibility of overflow doing it that way.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>> +		DRM_DEBUG("Attempting to execute commands from beyond the bounds of the batch object\n");
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>>  	/* Move the objects en-masse into the GTT, evicting if necessary. */
>>  	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
>>  	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, ctx, &need_relocs);
>> -- 
>> 2.6.2
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo
  2016-04-28  8:51   ` Jani Nikula
@ 2016-04-28  8:54       ` Jani Nikula
  0 siblings, 0 replies; 10+ messages in thread
From: Jani Nikula @ 2016-04-28  8:54 UTC (permalink / raw)
  To: Ville Syrjälä, Chris Wilson; +Cc: intel-gfx, stable

On Thu, 28 Apr 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 20 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Fri, Nov 20, 2015 at 03:11:04PM +0000, Chris Wilson wrote:
>>> The offset within and the length of the command sequence to execute are
>>> supplied by the user with respect to the batch buffer. We should be
>>> validating that region is wholly contained within the batch buffer;
>>> make it so.
>>> 
>>> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>> 
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> index a4c243cec4aa..e38284c1b89f 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> @@ -1462,6 +1462,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>>  	/* take note of the batch buffer before we might reorder the lists */
>>>  	batch_obj = eb_get_batch(eb);
>>>  
>>> +	if (args->batch_len > batch_obj->base.size ||
>>> +	    args->batch_start_offset > batch_obj->base.size - args->batch_len) {
>>
>> lgtm. No possibility of overflow doing it that way.
>>
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I don't know what I fat fingered with the previous mail, but I just
stumbled upon this patch and noticed it never made it. Is this still
valid?

BR,
Jani.


>>
>>> +		DRM_DEBUG("Attempting to execute commands from beyond the bounds of the batch object\n");
>>> +		ret = -EINVAL;
>>> +		goto err;
>>> +	}
>>> +
>>>  	/* Move the objects en-masse into the GTT, evicting if necessary. */
>>>  	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
>>>  	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, ctx, &need_relocs);
>>> -- 
>>> 2.6.2
>>> 
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo
@ 2016-04-28  8:54       ` Jani Nikula
  0 siblings, 0 replies; 10+ messages in thread
From: Jani Nikula @ 2016-04-28  8:54 UTC (permalink / raw)
  To: Ville Syrjälä, Chris Wilson; +Cc: intel-gfx, stable

On Thu, 28 Apr 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 20 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Fri, Nov 20, 2015 at 03:11:04PM +0000, Chris Wilson wrote:
>>> The offset within and the length of the command sequence to execute are
>>> supplied by the user with respect to the batch buffer. We should be
>>> validating that region is wholly contained within the batch buffer;
>>> make it so.
>>> 
>>> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>> 
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> index a4c243cec4aa..e38284c1b89f 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> @@ -1462,6 +1462,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>>  	/* take note of the batch buffer before we might reorder the lists */
>>>  	batch_obj = eb_get_batch(eb);
>>>  
>>> +	if (args->batch_len > batch_obj->base.size ||
>>> +	    args->batch_start_offset > batch_obj->base.size - args->batch_len) {
>>
>> lgtm. No possibility of overflow doing it that way.
>>
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I don't know what I fat fingered with the previous mail, but I just
stumbled upon this patch and noticed it never made it. Is this still
valid?

BR,
Jani.


>>
>>> +		DRM_DEBUG("Attempting to execute commands from beyond the bounds of the batch object\n");
>>> +		ret = -EINVAL;
>>> +		goto err;
>>> +	}
>>> +
>>>  	/* Move the objects en-masse into the GTT, evicting if necessary. */
>>>  	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
>>>  	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, ctx, &need_relocs);
>>> -- 
>>> 2.6.2
>>> 
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo
  2016-04-28  8:54       ` Jani Nikula
@ 2016-04-28  9:02         ` Chris Wilson
  -1 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2016-04-28  9:02 UTC (permalink / raw)
  To: Jani Nikula; +Cc: Ville Syrjälä, intel-gfx, stable

On Thu, Apr 28, 2016 at 11:54:04AM +0300, Jani Nikula wrote:
> On Thu, 28 Apr 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Fri, 20 Nov 2015, Ville Syrj�l� <ville.syrjala@linux.intel.com> wrote:
> >> On Fri, Nov 20, 2015 at 03:11:04PM +0000, Chris Wilson wrote:
> >>> The offset within and the length of the command sequence to execute are
> >>> supplied by the user with respect to the batch buffer. We should be
> >>> validating that region is wholly contained within the batch buffer;
> >>> make it so.
> >>> 
> >>> Reported-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: stable@vger.kernel.org
> >>> ---
> >>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
> >>>  1 file changed, 7 insertions(+)
> >>> 
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>> index a4c243cec4aa..e38284c1b89f 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>> @@ -1462,6 +1462,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >>>  	/* take note of the batch buffer before we might reorder the lists */
> >>>  	batch_obj = eb_get_batch(eb);
> >>>  
> >>> +	if (args->batch_len > batch_obj->base.size ||
> >>> +	    args->batch_start_offset > batch_obj->base.size - args->batch_len) {
> >>
> >> lgtm. No possibility of overflow doing it that way.
> >>
> >> Reviewed-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> I don't know what I fat fingered with the previous mail, but I just
> stumbled upon this patch and noticed it never made it. Is this still
> valid?

Yup, I'd completely forgotten about this patch and it we don't have the
safeguard in the kernel yet.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo
@ 2016-04-28  9:02         ` Chris Wilson
  0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2016-04-28  9:02 UTC (permalink / raw)
  To: Jani Nikula; +Cc: Ville Syrjälä, intel-gfx, stable

On Thu, Apr 28, 2016 at 11:54:04AM +0300, Jani Nikula wrote:
> On Thu, 28 Apr 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Fri, 20 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> On Fri, Nov 20, 2015 at 03:11:04PM +0000, Chris Wilson wrote:
> >>> The offset within and the length of the command sequence to execute are
> >>> supplied by the user with respect to the batch buffer. We should be
> >>> validating that region is wholly contained within the batch buffer;
> >>> make it so.
> >>> 
> >>> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: stable@vger.kernel.org
> >>> ---
> >>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
> >>>  1 file changed, 7 insertions(+)
> >>> 
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>> index a4c243cec4aa..e38284c1b89f 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>> @@ -1462,6 +1462,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >>>  	/* take note of the batch buffer before we might reorder the lists */
> >>>  	batch_obj = eb_get_batch(eb);
> >>>  
> >>> +	if (args->batch_len > batch_obj->base.size ||
> >>> +	    args->batch_start_offset > batch_obj->base.size - args->batch_len) {
> >>
> >> lgtm. No possibility of overflow doing it that way.
> >>
> >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I don't know what I fat fingered with the previous mail, but I just
> stumbled upon this patch and noticed it never made it. Is this still
> valid?

Yup, I'd completely forgotten about this patch and it we don't have the
safeguard in the kernel yet.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo
  2016-04-28  9:02         ` Chris Wilson
  (?)
@ 2016-04-28 11:14         ` Dave Gordon
  -1 siblings, 0 replies; 10+ messages in thread
From: Dave Gordon @ 2016-04-28 11:14 UTC (permalink / raw)
  To: intel-gfx, Chris Wilson

On 28/04/16 10:02, Chris Wilson wrote:
> On Thu, Apr 28, 2016 at 11:54:04AM +0300, Jani Nikula wrote:
>> On Thu, 28 Apr 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>> On Fri, 20 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>>>> On Fri, Nov 20, 2015 at 03:11:04PM +0000, Chris Wilson wrote:
>>>>> The offset within and the length of the command sequence to execute are
>>>>> supplied by the user with respect to the batch buffer. We should be
>>>>> validating that region is wholly contained within the batch buffer;
>>>>> make it so.
>>>>>
>>>>> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: stable@vger.kernel.org
>>>>> ---
>>>>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
>>>>>   1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>> index a4c243cec4aa..e38284c1b89f 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>> @@ -1462,6 +1462,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>>>>   	/* take note of the batch buffer before we might reorder the lists */
>>>>>   	batch_obj = eb_get_batch(eb);
>>>>>
>>>>> +	if (args->batch_len > batch_obj->base.size ||
>>>>> +	    args->batch_start_offset > batch_obj->base.size - args->batch_len) {
>>>>
>>>> lgtm. No possibility of overflow doing it that way.
>>>>
>>>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> I don't know what I fat fingered with the previous mail, but I just
>> stumbled upon this patch and noticed it never made it. Is this still
>> valid?
>
> Yup, I'd completely forgotten about this patch and it we don't have the
> safeguard in the kernel yet.
> -Chris
>

Hmm .. this will allow (len == 0) as long as start is in the range 
[0..size). But later, i915_gem_ringbuffer_submission() will interpret 
length 0 as meaning "sizeof batch_bo", which would be out-of-bounds if 
start != 0. Relevant code is:

         exec_len   = args->batch_len;
         exec_start = params->batch_obj_vm_offset +
                      params->args_batch_start_offset;

         if (exec_len == 0)
                 exec_len = params->batch_obj->base.size;

Should we permit len == 0 iff start == 0? Or take it to mean "from start 
to sizeof bo", and maybe put the replacement in the new check code 
rather than later, in submission()?

Of course batch length is not actually used on later hardware, and so 
the execlists version doesn't make this substitution, since any hardware 
that can do execlists only uses the BB-START form without a length. 
Nonetheless we might still want to validate and interpret this in a 
uniform manner ...

.Dave.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-04-28 11:14 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-20 15:11 [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo Chris Wilson
2015-11-20 15:11 ` Chris Wilson
2015-11-20 15:38 ` [Intel-gfx] " Ville Syrjälä
2015-11-20 15:38   ` Ville Syrjälä
2016-04-28  8:51   ` Jani Nikula
2016-04-28  8:54     ` Jani Nikula
2016-04-28  8:54       ` Jani Nikula
2016-04-28  9:02       ` [Intel-gfx] " Chris Wilson
2016-04-28  9:02         ` Chris Wilson
2016-04-28 11:14         ` Dave Gordon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.