[PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo

[PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo

[Chris Wilson]

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

Re: [Intel-gfx] [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo

[Ville Syrjälä]

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

Re: [Intel-gfx] [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo

[Jani Nikula]

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

Re: [Intel-gfx] [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo

[Jani Nikula]

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

Re: [Intel-gfx] [PATCH] drm/i915: Validate execbuffer start/length arguments against the target bo

[Chris Wilson]

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