Linux kernel DRM Intel i915 Multiple IOCTL Integer Overflows

A few days ago I was checking the ChangeLog of 3.3.5 release of the Linux kernel. As you can see the issues were reported by Xi Wang and the exact code for the first vulnreability is located at drivers/gpu/drm/i915/i915_gem_execbuffer.c and below you can see the code snippet.

static int
i915_gem_do_execbuffer(struct drm_device *dev, void *data,
                       struct drm_file *file,
                       struct drm_i915_gem_execbuffer2 *args,
                       struct drm_i915_gem_exec_object2 *exec)
{
        drm_i915_private_t *dev_priv = dev->dev_private;
        struct list_head objects;
        struct eb_objects *eb;
        struct drm_i915_gem_object *batch_obj;
        struct drm_clip_rect *cliprects = NULL;
        struct intel_ring_buffer *ring;
        u32 exec_start, exec_len;
        u32 seqno;
        u32 mask;
        int ret, mode, i;
   ...
        if (args->num_cliprects != 0) {
                if (ring != &dev_priv->ring[RCS]) {
                        DRM_ERROR("clip rectangles are only valid with the render ring\n");
                        return -EINVAL;
                }

                cliprects = kmalloc(args->num_cliprects * sizeof(*cliprects),
                                    GFP_KERNEL);
                if (cliprects == NULL) {
                        ret = -ENOMEM;
                        goto pre_mutex_err;
                }

                if (copy_from_user(cliprects,
                                     (struct drm_clip_rect __user *)(uintptr_t)
                                     args->cliprects_ptr,
                                     sizeof(*cliprects)*args->num_cliprects)) {
                        ret = -EFAULT;
                        goto pre_mutex_err;
                }
        }
   ...
        kfree(cliprects);
        return ret;
}

Clearly, the above kmalloc() could result in an integer overflow on 32-bit systems if the user controlled ‘args->num_cliprects’ (controlled through IOCTL) is large enough. Here you can also see how ‘drm_i915_gem_execbuffer2’ structure is defined in include/drm/i915_drm.h header file.

struct drm_i915_gem_execbuffer2 {
        /**
         * List of gem_exec_object2 structs
         */
        __u64 buffers_ptr;
        __u32 buffer_count;

        /** Offset in the batchbuffer to start execution from. */
        __u32 batch_start_offset;
        /** Bytes used in batchbuffer from batch_start_offset */
        __u32 batch_len;
        __u32 DR1;
        __u32 DR4;
        __u32 num_cliprects;
        /** This is a struct drm_clip_rect *cliprects */
        __u64 cliprects_ptr;
#define I915_EXEC_RING_MASK              (7<<0)
#define I915_EXEC_DEFAULT                (0<<0)
#define I915_EXEC_RENDER                 (1<<0)
#define I915_EXEC_BSD                    (2<<0)
#define I915_EXEC_BLT                    (3<<0)

/* Used for switching the constants addressing mode on gen4+ RENDER ring.
 * Gen6+ only supports relative addressing to dynamic state (default) and
 * absolute addressing.
 *
 * These flags are ignored for the BSD and BLT rings.
 */
#define I915_EXEC_CONSTANTS_MASK        (3<<6)
#define I915_EXEC_CONSTANTS_REL_GENERAL (0<<6) /* default */
#define I915_EXEC_CONSTANTS_ABSOLUTE    (1<<6)
#define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
        __u64 flags;
        __u64 rsvd1;
        __u64 rsvd2;
};

The fix was to add the missing checks as shown below.

 		}
 
+		if (args->num_cliprects > UINT_MAX / sizeof(*cliprects)) {
+			DRM_DEBUG("execbuf with %u cliprects\n",
+				  args->num_cliprects);
+			return -EINVAL;
+		}
 		cliprects = kmalloc(args->num_cliprects * sizeof(*cliprects),
 				    GFP_KERNEL);

The second vulnerability is on the same file and it was also reported by Xi Wang. Here is the equivalent code snippet.

int
i915_gem_execbuffer2(struct drm_device *dev, void *data,
                     struct drm_file *file)
{
        struct drm_i915_gem_execbuffer2 *args = data;
        struct drm_i915_gem_exec_object2 *exec2_list = NULL;
        int ret;

        if (args->buffer_count < 1) {
                DRM_ERROR("execbuf2 with %d buffers\n", args->buffer_count);
                return -EINVAL;
        }

        exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count,
                             GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
   ...
        ret = copy_from_user(exec2_list,
                             (struct drm_i915_relocation_entry __user *)
                             (uintptr_t) args->buffers_ptr,
                             sizeof(*exec2_list) * args->buffer_count);
   ...
        ret = i915_gem_do_execbuffer(dev, data, file, args, exec2_list);
        if (!ret) {
                /* Copy the new buffer offsets back to the user's exec list. */
                ret = copy_to_user((struct drm_i915_relocation_entry __user *)
                                   (uintptr_t) args->buffers_ptr,
                                   exec2_list,
                                   sizeof(*exec2_list) * args->buffer_count);
                if (ret) {
                        ret = -EFAULT;
                        DRM_ERROR("failed to copy %d exec entries "
                                  "back to user (%d)\n",
                                  args->buffer_count, ret);
                }
        }

        drm_free_large(exec2_list);
        return ret;
}

Here we have an identical possible integer overflow on the kmalloc() call that uses the user controlled ‘args->buffer_count’ (once again controlled through IOCTL). The fix was to add the missing checks.

 	int ret;
 
-	if (args->buffer_count < 1) {
+	if (args->buffer_count < 1 ||
+	    args->buffer_count > UINT_MAX / sizeof(*exec2_list)) {
 		DRM_ERROR("execbuf2 with %d buffers\n", args->buffer_count);
 		return -EINVAL;

It is interesting that I was not able to find any CVE or security advisory for these vulnerabilities apart from the Linux kernel’s ChangeLog.