Re: [PATCH] drm/i915: Possible security hole in command parsing

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

"Rebecca N. Palmer" <rebecca_palmer@zoho.com> writes:

Hi,

> i915_parse_cmds returns -EACCES on chained batches, which "tells the
> caller to abort and dispatch the workload as a non-secure batch",
> but the mechanism implementing that was broken when
> flags |= I915_DISPATCH_SECURE was moved from i915_gem_execbuffer_parse
> to i915_gem_do_execbuffer (17cabf571e50677d980e9ab2a43c5f11213003ae):
> i915_gem_execbuffer_parse returns the original batch_obj in this case,
> and i915_gem_do_execbuffer doesn't check for that.

> Is this being made secure some other way (in which case the obsolete
> comments should probably be removed), or is this a security hole?
>
> Warning: this is my first kernel patch, and has not been tested yet.
> Signed-off-by: Rebecca Palmer <rebecca_palmer@zoho.com> 
>
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1398,7 +1398,7 @@ i915_gem_do_execbuffer(struct drm_device
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct eb_vmas *eb;
> -	struct drm_i915_gem_object *batch_obj;
> +	struct drm_i915_gem_object *batch_obj, *orig_batch_obj;
>  	struct drm_i915_gem_exec_object2 shadow_exec_entry;
>  	struct intel_engine_cs *ring;
>  	struct intel_context *ctx;
> @@ -1511,7 +1511,7 @@ i915_gem_do_execbuffer(struct drm_device
>  		goto err;
>  
>  	/* take note of the batch buffer before we might reorder the lists */
> -	batch_obj = eb_get_batch(eb);
> +	orig_batch_obj = eb_get_batch(eb);
>  
>  	/* Move the objects en-masse into the GTT, evicting if necessary. */
>  	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
> @@ -1533,7 +1533,7 @@ i915_gem_do_execbuffer(struct drm_device
>  	}
>  
>  	/* Set the pending read domains for the batch buffer to COMMAND */
> -	if (batch_obj->base.pending_write_domain) {
> +	if (orig_batch_obj->base.pending_write_domain) {
>  		DRM_DEBUG("Attempting to use self-modifying batch buffer\n");
>  		ret = -EINVAL;
>  		goto err;
> @@ -1543,7 +1543,7 @@ i915_gem_do_execbuffer(struct drm_device
>  		batch_obj = i915_gem_execbuffer_parse(ring,
>  						      &shadow_exec_entry,
>  						      eb,
> -						      batch_obj,
> +						      orig_batch_obj,
>  						      args->batch_start_offset,
>  						      args->batch_len,
>  						      file->is_master);
> @@ -1559,7 +1559,7 @@ i915_gem_do_execbuffer(struct drm_device
>  		 * don't want that set when the command parser is
>  		 * enabled.
>  		 */
> -		if (USES_PPGTT(dev))

USES_PPGTT(dev) has been removed in the latest nightly, so you can
remove it here.


> +		if (USES_PPGTT(dev) && batch_obj!=orig_batch_obj)

Coding convention needs spaces around the != check.
(see scripts/checkpatch.pl).

Also please consider adding comment above parsed_obj != batch_obj
check about the parser ignoring the batch. Like
/* Skip the promotion if the parser ignored the patch */

>  			dispatch_flags |= I915_DISPATCH_SECURE;

On other gens where cmdparser is disabled, batch_obj is
left dangling as the 'if (i915_needs_cmd_parser(ring) && args->batch_len)'
branch is never taken on other than gen == 7.

I suggest that you introduce a *parsed_obj in the branch scope,
give original batch_obj to execbuffer_parse() and and do the
parsed_obj != batch_obj and batch_obj reassignment inside the
scope.

-Mika

>  		exec_start = 0;
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx