[Intel-gfx] [PATCH v2] drm/i915: Fix possible security hole in command parsing

[Mika Kuoppala]

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

Hi,

>> where cmdparser is disabled, batch_obj is
>> left dangling
> Sorry!  Fixed now.
>

There is absolutely nothing to be sorry about.

> This version also brings exec_start = 0 inside this check, as it
> appears to be there because the copying (i915_cmd_parser.c:1054)
> removes any offset the original might have had.
>
> When tested on next-20150508 (675b3fb), it passed my checks
> (libva tests, vlc video, glxgears, beignet tests), and didn't
> show the "missing window title bar" problem [0-1] in 3 attempts,
> but given the intermittent nature of that I can't be sure.
>
> I still can't give useful i-g-t results, as it works on 3.16
> but reports "GPU HANG" for most tests on 4.0 and (both patched and
> unpatched) next (scripts/run-tests.sh at the recovery-mode
> (single-user-mode) prompt, both i-g-t 1.10 and latest git).
>
> [0] http://lists.freedesktop.org/archives/intel-gfx/2015-May/065705.html
> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-May/065906.html
>
> ---
>
> i915_gem_execbuffer_parse returns the original batch_obj on batches
> it can't check (currently, chained batches).  Don't set the secure
> bit in this case.
>
> v2 (thanks to Mika Kuoppala):
> Don't leave batch_obj unset when the parser is not run.
> Only do exec_start = 0 on parsed batches.
> Add comments.
>
> Signed-off-by: Rebecca Palmer <rebecca_palmer at zoho.com>
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 7ab63d9..2fb6dc1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1540,28 +1540,38 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	}
>  
>  	if (i915_needs_cmd_parser(ring) && args->batch_len) {
> -		batch_obj = i915_gem_execbuffer_parse(ring,
> +		struct drm_i915_gem_object *parsed_batch_obj;
> +
> +		parsed_batch_obj = i915_gem_execbuffer_parse(ring,
>  						      &shadow_exec_entry,
>  						      eb,
>  						      batch_obj,
>  						      args->batch_start_offset,
>  						      args->batch_len,
>  						      file->is_master);
> -		if (IS_ERR(batch_obj)) {
> -			ret = PTR_ERR(batch_obj);
> +		if (IS_ERR(parsed_batch_obj)) {
> +			/* Batch rejected by parser, or an error
> occurred */

This comment should be omitted as the rejection part is not
valid in here and the error part is redudant. Daniel can squash it while
applying I think.

For a first patch, stellar work!

Reviewed-by: Mika Kuoppala <mika.kuoppala at intel.com>

> +			ret = PTR_ERR(parsed_batch_obj);
>  			goto err;
>  		}
>  
> -		/*
> -		 * Set the DISPATCH_SECURE bit to remove the NON_SECURE
> -		 * bit from MI_BATCH_BUFFER_START commands issued in the
> -		 * dispatch_execbuffer implementations. We specifically
> -		 * don't want that set when the command parser is
> -		 * enabled.
> -		 */
> -		dispatch_flags |= I915_DISPATCH_SECURE;
> -
> -		exec_start = 0;
> +		/* parsed_batch_obj == batch_obj means batch not fully parsed:
> +		 * accept, but don't promote to secure */
> +
> +		if (parsed_batch_obj != batch_obj) {
> +			/*
> +			 * Batch parsed and accepted:
> +			 *
> +			 * Set the DISPATCH_SECURE bit to remove the NON_SECURE
> +			 * bit from MI_BATCH_BUFFER_START commands issued in
> +			 * the dispatch_execbuffer implementations. We
> +			 * specifically don't want that set on batches the
> +			 * command parser has accepted.
> +			 */
> +			dispatch_flags |= I915_DISPATCH_SECURE;
> +			exec_start = 0;
> +			batch_obj = parsed_batch_obj;
> +		}
>  	}
>  
>  	batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx