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

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

[Rebecca N. Palmer]

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))
+		if (USES_PPGTT(dev) && batch_obj!=orig_batch_obj)
 			dispatch_flags |= I915_DISPATCH_SECURE;
 
 		exec_start = 0;

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

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

[Rebecca N. Palmer]

I've now done some testing (on an i5-3230M, in Debian 8), and this patch 
doesn't *appear* to break anything: both with and without it (starting 
from linux-next 20150430 (fa94df1) + commit 245054a drm/i915: Enable cmd 
parser to do secure batch promotion for aliasing ppgtt),

-libva (said in earlier discussion to use chained batches): all basic 
tests pass except test_07 (which doesn't work under 3.16 either); 
putsurface works
-video (file playback and live camera) in vlc works
-beignet (OpenCL) test suite: all pass except builtin_powr_* 
(long-standing known issue) and builtin_tgamma (it appears that 
linux-next puts the *C*PU in denormals-flushed-to-0 floating point mode, 
which breaks this test's checking mechanism: not sure if that's a bug or 
just a difference between Debian's and your defaults, but as it happens 
both with and without the patch, it's nothing to do with this)

The one problem I did see only with the patch was that one session had 
all its windows open in the top left of the screen, un-movable, and 
missing their title bar, but this was not reproducible, so I can't tell 
if it was a result of the patch or a coincidence.

However, plain linux-next 20150430 (without 245054a) has a lot of 
problems ("GPU HANG" in the kernel log on startup but the Xfce desktop 
does come up), glxgears segfaults, beignet gives a few wrong (all-0) 
results then throws CL_OUT_OF_RESOURCES, video doesn't play; probably 
https://bugs.freedesktop.org/show_bug.cgi?id=90190), and given that all 
245054a does is enable secure batch promotion, that suggests that the 
driver no longer handles non-promoted batches properly, making this 
patch a risky move.

I tried the intel-gpu-tools tests (1.10, running in recovery mode to 
avoid loading X), but found that most (not all) of the tests reported 
"GPU HANG" in all three linux-next cases (but worked under 3.16).

Note that I will be away from email for the next few days.

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

Re: drm/i915: Possible security hole in command parsing

[Rebecca N. Palmer]

Repeating those tests[1] on linux-next 20150505 gave the same results,
but having seen the intermittent "no window title bar" problem twice
more, I now have to consider it a result of my patch[0], and hence
withdraw it.

However, that doesn't mean there isn't a security problem here,
only that we need a different way to fix it.

[0] http://lists.freedesktop.org/archives/intel-gfx/2015-April/065627.html 
[1] http://lists.freedesktop.org/archives/intel-gfx/2015-May/065705.html

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

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

[Mika Kuoppala]

"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

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

[Daniel Vetter]

On Thu, Apr 30, 2015 at 12:32:15PM +0100, Rebecca N. Palmer wrote:
> 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.

Looks really nice tbh and seems to fix a regression that the igt testsuite
caught (gem_cmd_parse/chained-batches). Thanks a lot. Mika has some minor
review comments, with those address I'll pull this in.

Thanks, Daniel

> 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))
> +		if (USES_PPGTT(dev) && batch_obj!=orig_batch_obj)
>  			dispatch_flags |= I915_DISPATCH_SECURE;
>  
>  		exec_start = 0;
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Fix possible security hole in command parsing

[Rebecca N. Palmer]

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

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@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 */
+			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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Fix possible security hole in command parsing

[Mika Kuoppala]

"Rebecca N. Palmer" <rebecca_palmer@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@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@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@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

Re: [PATCH v2] drm/i915: Fix possible security hole in command parsing

[Daniel Vetter]

On Fri, May 08, 2015 at 05:04:48PM +0300, Mika Kuoppala wrote:
> "Rebecca N. Palmer" <rebecca_palmer@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
> >
> > ---

Just an side: git drops everything _below_ the --- line from the commit
message when applying the patch. Hence comments like the above should go
below that line, and the real commit message above. Also your commit
message for v2 doesn't mention how this bug was introduced, which is
crucial information. I've stitched something together.

> > 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@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.

Done while applying.

> 
> For a first patch, stellar work!

Indeed. Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH for 4.1] drm/i915: Don't clear exec_start if batch was not copied

[Rebecca N. Palmer]

i915_gem_execbuffer_parse returns the original batch_obj on batches
it can't check (currently, chained batches).  Don't clear offset
or set I915_DISPATCH_SECURE in this case.

Fixes 17cabf571e50677d980e9ab2a43c5f11213003ae.

Signed-off-by: Rebecca Palmer <rebecca_palmer@zoho.com>
---
> > 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.
> [pushed without comment on this]

That makes this a bug in mainline as well, though I don't know of
any actual problems it causes.

(The security hole exists there too, but only with the declared-unsafe
parameter i915.enable_ppgtt=2, hence the change of title)

This fix was tested on 3e0283a (tip of Linus' tree), in the same way
as before (libva tests, vlc video, glxgears, beignet tests).

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a3190e79..5ff8a64 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1548,33 +1548,39 @@ 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)) {
+			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.
-		 *
-		 * FIXME: with aliasing ppgtt, buffers that should only
-		 * be in ggtt still end up in the aliasing ppgtt. remove
-		 * this check when that is fixed.
-		 */
-		if (USES_FULL_PPGTT(dev))
-			dispatch_flags |= I915_DISPATCH_SECURE;
-
-		exec_start = 0;
+		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.
+			 *
+			 * FIXME: with aliasing ppgtt, buffers that should only
+			 * be in ggtt still end up in the aliasing ppgtt.
+			 * remove USES_FULL_PPGTT check when that is fixed.
+			 */
+			if (USES_FULL_PPGTT(dev))
+				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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: drm/i915: Possible security hole in command parsing

[Kees Cook]

[Since this has been publicly reported, I'm moving security@kernel.org to Bcc]

On Tue, May 5, 2015 at 2:39 PM, Rebecca N. Palmer
<rebecca_palmer@zoho.com> wrote:
> Repeating those tests[1] on linux-next 20150505 gave the same results,
> but having seen the intermittent "no window title bar" problem twice
> more, I now have to consider it a result of my patch[0], and hence
> withdraw it.
>
> However, that doesn't mean there isn't a security problem here,
> only that we need a different way to fix it.
>
> [0] http://lists.freedesktop.org/archives/intel-gfx/2015-April/065627.html
> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-May/065705.html

Has this issue been resolved? (I have no idea what "secure" means in
this graphics context.)

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: drm/i915: Possible security hole in command parsing

[Rebecca N. Palmer]

In next, it was fixed by 
http://cgit.freedesktop.org/drm-intel/commit/drivers/gpu/drm/i915/i915_gem_execbuffer.c?h=for-linux-next&id=c7c7372edc4ebc173ad359aeb5752e9ce09f2741

In 4.1, the problem only exists with the marked-as-unsafe parameter 
i915.enable_ppgtt=2.  (It could be fixed there with 
http://lists.freedesktop.org/archives/intel-gfx/2015-May/066259.html , 
but doing so at this late stage would be risky.)

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