Error in inner loop in validate_cmds_sorted / out of bounds issue
diff mbox

Message ID 20150727085945.GA7300@nuc-i3427.alporthouse.com
State New
Headers show

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 mbox

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