Error in inner loop in validate_cmds_sorted / out of bounds issue

Commit Message

Chris Wilson July 27, 2015, 8:59 a.m. UTC

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.

Patch

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