Error in inner loop in validate_cmds_sorted / out of bounds issue

Error in inner loop in validate_cmds_sorted / out of bounds issue

[Hanno Böck]

[-- Attachment #1.1: Type: text/plain, Size: 1006 bytes --]

Hi,

I was trying to track down an out of bounds read issue in the intel drm
driver that got reported by kasan.

It happens in the function validate_cmds_sorted (i915_cmd_parser.c),
where there are two nested loops, this is the relevant code part:
	for (i = 0; i < cmd_table_count; i++) {
		const struct drm_i915_cmd_table *table = &cmd_tables[i];
		u32 previous = 0;
		int j;

		for (j = 0; j < table->count; j++) {
			const struct drm_i915_cmd_descriptor *desc =
				&table->table[i];


Now that &table->table[i] should probably really be &table->table[j],
because that's the counter variable of the inner loop. Otherwise it
doesn't make any sense (the inner loop would just repeat doing the same
thing multiple times).
However if I try to change [i] to [j] here my system doesn't boot any
more, I just get a black screen. So I assume this bug is somehow hiding
another more severe bug.


cu,
-- 
Hanno Böck
http://hboeck.de/

mail/jabber: hanno@hboeck.de
GPG: BBB51E42

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: Error in inner loop in validate_cmds_sorted / out of bounds issue

[Chris Wilson]

On Sat, Jul 25, 2015 at 06:56:20PM -0700, Hanno Böck wrote:
> Hi,
> 
> I was trying to track down an out of bounds read issue in the intel drm
> driver that got reported by kasan.
> 
> It happens in the function validate_cmds_sorted (i915_cmd_parser.c),
> where there are two nested loops, this is the relevant code part:
> 	for (i = 0; i < cmd_table_count; i++) {
> 		const struct drm_i915_cmd_table *table = &cmd_tables[i];
> 		u32 previous = 0;
> 		int j;
> 
> 		for (j = 0; j < table->count; j++) {
> 			const struct drm_i915_cmd_descriptor *desc =
> 				&table->table[i];
> 
> 
> Now that &table->table[i] should probably really be &table->table[j],
> because that's the counter variable of the inner loop. Otherwise it
> doesn't make any sense (the inner loop would just repeat doing the same
> thing multiple times).
> However if I try to change [i] to [j] here my system doesn't boot any
> more, I just get a black screen. So I assume this bug is somehow hiding
> another more severe bug.

The tables aren't sorted, that is worth fixing.

This should get you booting with minimal fuss if you care to track down
the error.

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 430571b..688e814 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -672,6 +672,13 @@ static void fini_hash_table(struct intel_engine_cs *ring)
        }
 }
 
+#define DRM_ERROR_ON(cond, fmt, ...) ({                                        \
+       bool __cond = !!(cond);                                         \
+       if (unlikely(__cond))                                           \
+               drm_err("assertion failed, %s: " fmt, #cond, ##__VA_ARGS__); \
+       unlikely(__cond);                                               \
+})
+
 /**
  * i915_cmd_parser_init_ring() - set cmd parser related fields for a ringbuffer
  * @ring: the ringbuffer to initialize
@@ -751,11 +758,16 @@ int i915_cmd_parser_init_ring(struct intel_engine_cs *ring)
        default:
                DRM_ERROR("CMD: cmd_parser_init with unknown ring: %d\n",
                          ring->id);
-               BUG();
+               return -ENODEV;
        }
 
-       BUG_ON(!validate_cmds_sorted(ring, cmd_tables, cmd_table_count));
-       BUG_ON(!validate_regs_sorted(ring));
+       if (DRM_ERROR_ON(!validate_cmds_sorted(ring, cmd_tables, cmd_table_count),
+                        "command parser table is not sorted - required for bisetion searching\n"))
+               return -ENODEV;
+
+       if (DRM_ERROR_ON(!validate_regs_sorted(ring),
+                        "register lists are not sorted - required for bisection searching\n"))
+               return -ENODEV;
 
        WARN_ON(!hash_empty(ring->cmd_hash));
 

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Error in inner loop in validate_cmds_sorted / out of bounds issue

[Hanno Böck]

[-- Attachment #1.1.1: Type: text/plain, Size: 348 bytes --]

On Mon, 27 Jul 2015 09:59:45 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> The tables aren't sorted, that is worth fixing.

Attached patch should do that and fix the loop. Now it boots without
errors.

Does that look okay? If so please apply.

-- 
Hanno Böck
http://hboeck.de/

mail/jabber: hanno@hboeck.de
GPG: BBB51E42

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: linux-i915-sort-cmds.diff --]
[-- Type: text/x-patch, Size: 1273 bytes --]

Signed-off-by: Hanno Boeck <hanno@hboeck.de>

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 306d9e4..95aeb70 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -151,8 +151,8 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
 	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
 	CMD(  MI_PREDICATE,                     SMI,    F,  1,      S  ),
 	CMD(  MI_TOPOLOGY_FILTER,               SMI,    F,  1,      S  ),
-	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
 	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
+	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
 	CMD(  MI_SET_CONTEXT,                   SMI,   !F,  0xFF,   R  ),
 	CMD(  MI_URB_CLEAR,                     SMI,   !F,  0xFF,   S  ),
 	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3F,   B,
@@ -564,7 +564,7 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring,
 
 		for (j = 0; j < table->count; j++) {
 			const struct drm_i915_cmd_descriptor *desc =
-				&table->table[i];
+				&table->table[j];
 			u32 curr = desc->cmd.value & desc->cmd.mask;
 
 			if (curr < previous) {

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: Error in inner loop in validate_cmds_sorted / out of bounds issue

[Chris Wilson]

On Mon, Jul 27, 2015 at 09:15:32PM -0700, Hanno Böck wrote:
> On Mon, 27 Jul 2015 09:59:45 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > The tables aren't sorted, that is worth fixing.
> 
> Attached patch should do that and fix the loop. Now it boots without
> errors.
> 
> Does that look okay? If so please apply.

Indeed, nice catch. Could you please read
Documentation/SubmittingPatches and apply your Signed-off-by and then we
can accept this patch under your authorship.

Preferrably this is two patches, (a) fix the tables, (b) fix the
validator. That way we can delay enabling the validator if we need to
fix the tables for others.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Error in inner loop in validate_cmds_sorted / out of bounds issue

[Daniel Vetter]

On Tue, Jul 28, 2015 at 08:45:51AM +0100, Chris Wilson wrote:
> On Mon, Jul 27, 2015 at 09:15:32PM -0700, Hanno Böck wrote:
> > On Mon, 27 Jul 2015 09:59:45 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > The tables aren't sorted, that is worth fixing.
> > 
> > Attached patch should do that and fix the loop. Now it boots without
> > errors.
> > 
> > Does that look okay? If so please apply.
> 
> Indeed, nice catch. Could you please read
> Documentation/SubmittingPatches and apply your Signed-off-by and then we
> can accept this patch under your authorship.
> 
> Preferrably this is two patches, (a) fix the tables, (b) fix the
> validator. That way we can delay enabling the validator if we need to
> fix the tables for others.

Also can you please add signed-off-by lines to your patch when
resubmitting? See Documentation/SubmittingPatches for all the details.

Thanks, 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: Error in inner loop in validate_cmds_sorted / out of bounds issue

[Hanno Böck]

[-- Attachment #1.1.1: Type: text/plain, Size: 1158 bytes --]

Hi,

On Tue, 28 Jul 2015 10:14:51 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> > Indeed, nice catch. Could you please read
> > Documentation/SubmittingPatches and apply your Signed-off-by and
> > then we can accept this patch under your authorship.
> > 
> > Preferrably this is two patches, (a) fix the tables, (b) fix the
> > validator. That way we can delay enabling the validator if we need
> > to fix the tables for others.

I think I have checked all tables, not just the ones used on my gpu,
they should be fine. But I've splittet the patch.

> Also can you please add signed-off-by lines to your patch when
> resubmitting? See Documentation/SubmittingPatches for all the details.

The patch already had a "Signed-off-by" line.

The checkpatch script complains that it doesn't like the formatting of
the CMD command. However I won't change that in this patch, as this is
how the whole file is formatted. If this is wanted I can submit a patch
changing the formatting afterwards, but I think this is an unrelated
change.

Please apply.

-- 
Hanno Böck
http://hboeck.de/

mail/jabber: hanno@hboeck.de
GPG: BBB51E42

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: 01-i915-fix-cmd-order.patch --]
[-- Type: text/x-patch, Size: 993 bytes --]

Properly sort cmd tables.

Signed-off-by: Hanno Boeck <hanno@hboeck.de>
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 306d9e4..5fdd8c8 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -151,8 +151,8 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
 	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
 	CMD(  MI_PREDICATE,                     SMI,    F,  1,      S  ),
 	CMD(  MI_TOPOLOGY_FILTER,               SMI,    F,  1,      S  ),
-	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
 	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
+	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
 	CMD(  MI_SET_CONTEXT,                   SMI,   !F,  0xFF,   R  ),
 	CMD(  MI_URB_CLEAR,                     SMI,   !F,  0xFF,   S  ),
 	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3F,   B,

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.3: 02-i915-fix-cmd-check-loop.patch --]
[-- Type: text/x-patch, Size: 603 bytes --]

Fix loop checking cmd tables.

Signed-off-by: Hanno Boeck <hanno@hboeck.de>
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 306d9e4..3a53bf3 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -564,7 +564,7 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring,
 
 		for (j = 0; j < table->count; j++) {
 			const struct drm_i915_cmd_descriptor *desc =
-				&table->table[i];
+				&table->table[j];
 			u32 curr = desc->cmd.value & desc->cmd.mask;
 
 			if (curr < previous) {

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: Error in inner loop in validate_cmds_sorted / out of bounds issue

[Chris Wilson]

On Tue, Jul 28, 2015 at 11:14:19AM -0700, Hanno Böck wrote:
> Hi,
> 
> On Tue, 28 Jul 2015 10:14:51 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > > Indeed, nice catch. Could you please read
> > > Documentation/SubmittingPatches and apply your Signed-off-by and
> > > then we can accept this patch under your authorship.
> > > 
> > > Preferrably this is two patches, (a) fix the tables, (b) fix the
> > > validator. That way we can delay enabling the validator if we need
> > > to fix the tables for others.
> 
> I think I have checked all tables, not just the ones used on my gpu,
> they should be fine. But I've splittet the patch.
> 
> > Also can you please add signed-off-by lines to your patch when
> > resubmitting? See Documentation/SubmittingPatches for all the details.
> 
> The patch already had a "Signed-off-by" line.
> 
> The checkpatch script complains that it doesn't like the formatting of
> the CMD command. However I won't change that in this patch, as this is
> how the whole file is formatted. If this is wanted I can submit a patch
> changing the formatting afterwards, but I think this is an unrelated
> change.
> 
> Please apply.
> 
> -- 
> Hanno Böck
> http://hboeck.de/
> 
> mail/jabber: hanno@hboeck.de
> GPG: BBB51E42

> Properly sort cmd tables.

drm/i915: Properly sort MI coomand table

In the future, we may want to speed up command/register searching using
a bisection and so we require them to be in ascending order respectively
by command value or register address. However, this was not true for one
pair in the MI table; make it so.
 
> Signed-off-by: Hanno Boeck <hanno@hboeck.de>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 306d9e4..5fdd8c8 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -151,8 +151,8 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
>  	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
>  	CMD(  MI_PREDICATE,                     SMI,    F,  1,      S  ),
>  	CMD(  MI_TOPOLOGY_FILTER,               SMI,    F,  1,      S  ),
> -	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
>  	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
> +	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
>  	CMD(  MI_SET_CONTEXT,                   SMI,   !F,  0xFF,   R  ),
>  	CMD(  MI_URB_CLEAR,                     SMI,   !F,  0xFF,   S  ),
>  	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3F,   B,

> Fix loop checking cmd tables.

drm/i915: Fix command parser table validator

As we may like to use a bisection search on the tables in future, we
need them to be ordered. For convenience we expect the compiled tables
to be order and check on initialisation. However, the validator used the
wrong iterators failed to spot the misordered MI tables and instead
walked off into the unknown (as spotted by kasan).

> Signed-off-by: Hanno Boeck <hanno@hboeck.de>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 306d9e4..3a53bf3 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -564,7 +564,7 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring,
>  
>  		for (j = 0; j < table->count; j++) {
>  			const struct drm_i915_cmd_descriptor *desc =
> -				&table->table[i];
> +				&table->table[j];
>  			u32 curr = desc->cmd.value & desc->cmd.mask;
>  
>  			if (curr < previous) {

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Error in inner loop in validate_cmds_sorted / out of bounds issue

[Daniel Vetter]

On Tue, Jul 28, 2015 at 08:36:18PM +0100, Chris Wilson wrote:
> On Tue, Jul 28, 2015 at 11:14:19AM -0700, Hanno Böck wrote:
> > Hi,
> > 
> > On Tue, 28 Jul 2015 10:14:51 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > > Indeed, nice catch. Could you please read
> > > > Documentation/SubmittingPatches and apply your Signed-off-by and
> > > > then we can accept this patch under your authorship.
> > > > 
> > > > Preferrably this is two patches, (a) fix the tables, (b) fix the
> > > > validator. That way we can delay enabling the validator if we need
> > > > to fix the tables for others.
> > 
> > I think I have checked all tables, not just the ones used on my gpu,
> > they should be fine. But I've splittet the patch.
> > 
> > > Also can you please add signed-off-by lines to your patch when
> > > resubmitting? See Documentation/SubmittingPatches for all the details.
> > 
> > The patch already had a "Signed-off-by" line.
> > 
> > The checkpatch script complains that it doesn't like the formatting of
> > the CMD command. However I won't change that in this patch, as this is
> > how the whole file is formatted. If this is wanted I can submit a patch
> > changing the formatting afterwards, but I think this is an unrelated
> > change.
> > 
> > Please apply.
> > 
> > -- 
> > Hanno Böck
> > http://hboeck.de/
> > 
> > mail/jabber: hanno@hboeck.de
> > GPG: BBB51E42
> 
> > Properly sort cmd tables.
> 
> drm/i915: Properly sort MI coomand table
> 
> In the future, we may want to speed up command/register searching using
> a bisection and so we require them to be in ascending order respectively
> by command value or register address. However, this was not true for one
> pair in the MI table; make it so.
>  
> > Signed-off-by: Hanno Boeck <hanno@hboeck.de>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index 306d9e4..5fdd8c8 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -151,8 +151,8 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
> >  	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
> >  	CMD(  MI_PREDICATE,                     SMI,    F,  1,      S  ),
> >  	CMD(  MI_TOPOLOGY_FILTER,               SMI,    F,  1,      S  ),
> > -	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
> >  	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
> > +	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
> >  	CMD(  MI_SET_CONTEXT,                   SMI,   !F,  0xFF,   R  ),
> >  	CMD(  MI_URB_CLEAR,                     SMI,   !F,  0xFF,   S  ),
> >  	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3F,   B,
> 
> > Fix loop checking cmd tables.
> 
> drm/i915: Fix command parser table validator
> 
> As we may like to use a bisection search on the tables in future, we
> need them to be ordered. For convenience we expect the compiled tables
> to be order and check on initialisation. However, the validator used the
> wrong iterators failed to spot the misordered MI tables and instead
> walked off into the unknown (as spotted by kasan).
> 
> > Signed-off-by: Hanno Boeck <hanno@hboeck.de>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Ok I hand-assembled the patches since it's your first, but please followo
Documentation/Submitting patches next time around, i.e. 1 mail per patch,
no attachments, diff included at the bottom. Otherwise the tools can't
just pick up patches on my side. git send-email will do all that
formatting for you correctly, so that's what I recommend to use.

Anyway pachtes applied to drm-intel-next-queued, thanks a lot for doing
them.
-Daniel

> 
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index 306d9e4..3a53bf3 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -564,7 +564,7 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring,
> >  
> >  		for (j = 0; j < table->count; j++) {
> >  			const struct drm_i915_cmd_descriptor *desc =
> > -				&table->table[i];
> > +				&table->table[j];
> >  			u32 curr = desc->cmd.value & desc->cmd.mask;
> >  
> >  			if (curr < previous) {
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

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