From 9b74f7348f214b1f99819f0d0da4a1cbabb1e740 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Wed, 22 Sep 2010 19:10:44 +0100 Subject: drm/i915: Fix 945GM regression in e259befd A minor typo caused a single fence register to be incorrectly programmed, resulting in occassional tiling corruption. Reported-and-tested-by: Hans de Bruin Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=18962 Signed-off-by: Chris Wilson Cc: stable@kernel.org --- drivers/gpu/drm/i915/i915_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/gpu/drm/i915/i915_gem.c') diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index cf4ffbee1c0..bced9b25c71 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2400,7 +2400,7 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj) I915_WRITE64(FENCE_REG_965_0 + (obj_priv->fence_reg * 8), 0); break; case 3: - if (obj_priv->fence_reg > 8) + if (obj_priv->fence_reg >= 8) fence_reg = FENCE_REG_945_8 + (obj_priv->fence_reg - 8) * 4; else case 2: -- cgit v1.2.3-70-g09d2 From 29d08b3efddca628b0360411ab2b85f7b1723f48 Mon Sep 17 00:00:00 2001 From: Dave Airlie Date: Mon, 27 Sep 2010 16:17:17 +1000 Subject: drm/gem: handlecount isn't really a kref so don't make it one. There were lots of places being inconsistent since handle count looked like a kref but it really wasn't. Fix this my just making handle count an atomic on the object, and have it increase the normal object kref. Now i915/radeon/nouveau drivers can drop the normal reference on userspace object creation, and have the handle hold it. This patch fixes a memory leak or corruption on unload, because the driver had no way of knowing if a handle had been actually added for this object, and the fbcon object needed to know this to clean itself up properly. Reviewed-by: Chris Wilson Signed-off-by: Dave Airlie --- drivers/gpu/drm/drm_gem.c | 8 ++------ drivers/gpu/drm/drm_info.c | 2 +- drivers/gpu/drm/i915/i915_gem.c | 6 ++---- drivers/gpu/drm/i915/intel_fb.c | 4 +++- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 1 + drivers/gpu/drm/nouveau/nouveau_gem.c | 6 ++---- drivers/gpu/drm/nouveau/nouveau_notifier.c | 1 + drivers/gpu/drm/radeon/radeon_display.c | 3 ++- drivers/gpu/drm/radeon/radeon_fb.c | 14 ++++---------- drivers/gpu/drm/radeon/radeon_gem.c | 4 ++-- include/drm/drmP.h | 18 +++++++++++++----- 11 files changed, 33 insertions(+), 34 deletions(-) (limited to 'drivers/gpu/drm/i915/i915_gem.c') diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 6fe2cd298c1..f7e61be8430 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -148,7 +148,7 @@ int drm_gem_object_init(struct drm_device *dev, return -ENOMEM; kref_init(&obj->refcount); - kref_init(&obj->handlecount); + atomic_set(&obj->handle_count, 0); obj->size = size; atomic_inc(&dev->object_count); @@ -496,12 +496,8 @@ static void drm_gem_object_ref_bug(struct kref *list_kref) * called before drm_gem_object_free or we'll be touching * freed memory */ -void -drm_gem_object_handle_free(struct kref *kref) +void drm_gem_object_handle_free(struct drm_gem_object *obj) { - struct drm_gem_object *obj = container_of(kref, - struct drm_gem_object, - handlecount); struct drm_device *dev = obj->dev; /* Remove any name for this object */ diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index 2ef2c782724..974e970ce3f 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -255,7 +255,7 @@ int drm_gem_one_name_info(int id, void *ptr, void *data) seq_printf(m, "%6d %8zd %7d %8d\n", obj->name, obj->size, - atomic_read(&obj->handlecount.refcount), + atomic_read(&obj->handle_count), atomic_read(&obj->refcount.refcount)); return 0; } diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index cf4ffbee1c0..4cdf74264ee 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -136,14 +136,12 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data, return -ENOMEM; ret = drm_gem_handle_create(file_priv, obj, &handle); + /* drop reference from allocate - handle holds it now */ + drm_gem_object_unreference_unlocked(obj); if (ret) { - drm_gem_object_unreference_unlocked(obj); return ret; } - /* Sink the floating reference from kref_init(handlecount) */ - drm_gem_object_handle_unreference_unlocked(obj); - args->handle = handle; return 0; } diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c index 7bdc96256bf..56ad9df2ccb 100644 --- a/drivers/gpu/drm/i915/intel_fb.c +++ b/drivers/gpu/drm/i915/intel_fb.c @@ -237,8 +237,10 @@ int intel_fbdev_destroy(struct drm_device *dev, drm_fb_helper_fini(&ifbdev->helper); drm_framebuffer_cleanup(&ifb->base); - if (ifb->obj) + if (ifb->obj) { + drm_gem_object_handle_unreference(ifb->obj); drm_gem_object_unreference(ifb->obj); + } return 0; } diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index dbd30b2e43f..d2047713dc5 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -352,6 +352,7 @@ nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *nfbdev) if (nouveau_fb->nvbo) { nouveau_bo_unmap(nouveau_fb->nvbo); + drm_gem_object_handle_unreference_unlocked(nouveau_fb->nvbo->gem); drm_gem_object_unreference_unlocked(nouveau_fb->nvbo->gem); nouveau_fb->nvbo = NULL; } diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c index ead7b8fc53f..19620a6709f 100644 --- a/drivers/gpu/drm/nouveau/nouveau_gem.c +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c @@ -167,11 +167,9 @@ nouveau_gem_ioctl_new(struct drm_device *dev, void *data, goto out; ret = drm_gem_handle_create(file_priv, nvbo->gem, &req->info.handle); + /* drop reference from allocate - handle holds it now */ + drm_gem_object_unreference_unlocked(nvbo->gem); out: - drm_gem_object_handle_unreference_unlocked(nvbo->gem); - - if (ret) - drm_gem_object_unreference_unlocked(nvbo->gem); return ret; } diff --git a/drivers/gpu/drm/nouveau/nouveau_notifier.c b/drivers/gpu/drm/nouveau/nouveau_notifier.c index 3ec181ff50c..3c9964a8fba 100644 --- a/drivers/gpu/drm/nouveau/nouveau_notifier.c +++ b/drivers/gpu/drm/nouveau/nouveau_notifier.c @@ -79,6 +79,7 @@ nouveau_notifier_takedown_channel(struct nouveau_channel *chan) mutex_lock(&dev->struct_mutex); nouveau_bo_unpin(chan->notifier_bo); mutex_unlock(&dev->struct_mutex); + drm_gem_object_handle_unreference_unlocked(chan->notifier_bo->gem); drm_gem_object_unreference_unlocked(chan->notifier_bo->gem); drm_mm_takedown(&chan->notifier_heap); } diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index 7422f274615..b92d2f2fcbe 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -843,8 +843,9 @@ static void radeon_user_framebuffer_destroy(struct drm_framebuffer *fb) { struct radeon_framebuffer *radeon_fb = to_radeon_framebuffer(fb); - if (radeon_fb->obj) + if (radeon_fb->obj) { drm_gem_object_unreference_unlocked(radeon_fb->obj); + } drm_framebuffer_cleanup(fb); kfree(radeon_fb); } diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c index c74a8b20d94..9cdf6a35bc2 100644 --- a/drivers/gpu/drm/radeon/radeon_fb.c +++ b/drivers/gpu/drm/radeon/radeon_fb.c @@ -94,8 +94,10 @@ static void radeonfb_destroy_pinned_object(struct drm_gem_object *gobj) ret = radeon_bo_reserve(rbo, false); if (likely(ret == 0)) { radeon_bo_kunmap(rbo); + radeon_bo_unpin(rbo); radeon_bo_unreserve(rbo); } + drm_gem_object_handle_unreference(gobj); drm_gem_object_unreference_unlocked(gobj); } @@ -325,8 +327,6 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfb { struct fb_info *info; struct radeon_framebuffer *rfb = &rfbdev->rfb; - struct radeon_bo *rbo; - int r; if (rfbdev->helper.fbdev) { info = rfbdev->helper.fbdev; @@ -338,14 +338,8 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfb } if (rfb->obj) { - rbo = rfb->obj->driver_private; - r = radeon_bo_reserve(rbo, false); - if (likely(r == 0)) { - radeon_bo_kunmap(rbo); - radeon_bo_unpin(rbo); - radeon_bo_unreserve(rbo); - } - drm_gem_object_unreference_unlocked(rfb->obj); + radeonfb_destroy_pinned_object(rfb->obj); + rfb->obj = NULL; } drm_fb_helper_fini(&rfbdev->helper); drm_framebuffer_cleanup(&rfb->base); diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index c578f265b24..d1e595d9172 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -201,11 +201,11 @@ int radeon_gem_create_ioctl(struct drm_device *dev, void *data, return r; } r = drm_gem_handle_create(filp, gobj, &handle); + /* drop reference from allocate - handle holds it now */ + drm_gem_object_unreference_unlocked(gobj); if (r) { - drm_gem_object_unreference_unlocked(gobj); return r; } - drm_gem_object_handle_unreference_unlocked(gobj); args->handle = handle; return 0; } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 774e1d49509..07e4726a4ee 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -612,7 +612,7 @@ struct drm_gem_object { struct kref refcount; /** Handle count of this object. Each handle also holds a reference */ - struct kref handlecount; + atomic_t handle_count; /* number of handles on this object */ /** Related drm device */ struct drm_device *dev; @@ -1461,7 +1461,7 @@ struct drm_gem_object *drm_gem_object_alloc(struct drm_device *dev, size_t size); int drm_gem_object_init(struct drm_device *dev, struct drm_gem_object *obj, size_t size); -void drm_gem_object_handle_free(struct kref *kref); +void drm_gem_object_handle_free(struct drm_gem_object *obj); void drm_gem_vm_open(struct vm_area_struct *vma); void drm_gem_vm_close(struct vm_area_struct *vma); int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma); @@ -1496,7 +1496,7 @@ static inline void drm_gem_object_handle_reference(struct drm_gem_object *obj) { drm_gem_object_reference(obj); - kref_get(&obj->handlecount); + atomic_inc(&obj->handle_count); } static inline void @@ -1505,12 +1505,15 @@ drm_gem_object_handle_unreference(struct drm_gem_object *obj) if (obj == NULL) return; + if (atomic_read(&obj->handle_count) == 0) + return; /* * Must bump handle count first as this may be the last * ref, in which case the object would disappear before we * checked for a name */ - kref_put(&obj->handlecount, drm_gem_object_handle_free); + if (atomic_dec_and_test(&obj->handle_count)) + drm_gem_object_handle_free(obj); drm_gem_object_unreference(obj); } @@ -1520,12 +1523,17 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) if (obj == NULL) return; + if (atomic_read(&obj->handle_count) == 0) + return; + /* * Must bump handle count first as this may be the last * ref, in which case the object would disappear before we * checked for a name */ - kref_put(&obj->handlecount, drm_gem_object_handle_free); + + if (atomic_dec_and_test(&obj->handle_count)) + drm_gem_object_handle_free(obj); drm_gem_object_unreference_unlocked(obj); } -- cgit v1.2.3-70-g09d2 From 929f49bf225b1b6cd04d0a7b9c0f7377d9131220 Mon Sep 17 00:00:00 2001 From: Julia Lawall Date: Sat, 2 Oct 2010 15:59:17 +0200 Subject: drivers/gpu/drm/i915/i915_gem.c: Add missing error handling code Extend the error handling code with operations found in other nearby error handling code A simplified version of the sematic match that finds this problem is as follows: (http://coccinelle.lip6.fr/) // @r exists@ @r@ statement S1,S2,S3; constant C1,C2,C3; @@ *if (...) {... S1 return -C1;} ... *if (...) {... when != S1 return -C2;} ... *if (...) {... S1 return -C3;} // Signed-off-by: Julia Lawall Signed-off-by: Chris Wilson Cc: stable@kernel.org --- drivers/gpu/drm/i915/i915_gem.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/gpu/drm/i915/i915_gem.c') diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index bced9b25c71..cfe597865f5 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3258,6 +3258,8 @@ i915_gem_object_pin_and_relocate(struct drm_gem_object *obj, (int) reloc->offset, reloc->read_domains, reloc->write_domain); + drm_gem_object_unreference(target_obj); + i915_gem_object_unpin(obj); return -EINVAL; } if (reloc->write_domain & I915_GEM_DOMAIN_CPU || -- cgit v1.2.3-70-g09d2 From ce9d419dbecc292cc3e06e8b1d6d123d3fa813a4 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Sun, 26 Sep 2010 20:50:05 +0100 Subject: drm/i915: Sanity check pread/pwrite Move the access control up from the fast paths, which are no longer universally taken first, up into the caller. This then duplicates some sanity checking along the slow paths, but is much simpler. Tracked as CVE-2010-2962. Reported-by: Kees Cook Signed-off-by: Chris Wilson Cc: stable@kernel.org --- drivers/gpu/drm/i915/i915_gem.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) (limited to 'drivers/gpu/drm/i915/i915_gem.c') diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index cfe597865f5..7749e78a730 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -477,8 +477,15 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, */ if (args->offset > obj->size || args->size > obj->size || args->offset + args->size > obj->size) { - drm_gem_object_unreference_unlocked(obj); - return -EINVAL; + ret = -EINVAL; + goto err; + } + + if (!access_ok(VERIFY_WRITE, + (char __user *)(uintptr_t)args->data_ptr, + args->size)) { + ret = -EFAULT; + goto err; } if (i915_gem_object_needs_bit17_swizzle(obj)) { @@ -490,8 +497,8 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, file_priv); } +err: drm_gem_object_unreference_unlocked(obj); - return ret; } @@ -580,8 +587,6 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev, struct drm_gem_object *obj, user_data = (char __user *) (uintptr_t) args->data_ptr; remain = args->size; - if (!access_ok(VERIFY_READ, user_data, remain)) - return -EFAULT; mutex_lock(&dev->struct_mutex); @@ -940,8 +945,15 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, */ if (args->offset > obj->size || args->size > obj->size || args->offset + args->size > obj->size) { - drm_gem_object_unreference_unlocked(obj); - return -EINVAL; + ret = -EINVAL; + goto err; + } + + if (!access_ok(VERIFY_READ, + (char __user *)(uintptr_t)args->data_ptr, + args->size)) { + ret = -EFAULT; + goto err; } /* We can only do the GTT pwrite on untiled buffers, as otherwise @@ -975,8 +987,8 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, DRM_INFO("pwrite failed %d\n", ret); #endif +err: drm_gem_object_unreference_unlocked(obj); - return ret; } -- cgit v1.2.3-70-g09d2 From 7dcd2499deab8f10011713c40bc2f309c9b65077 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Sun, 26 Sep 2010 20:21:44 +0100 Subject: drm/i915: Rephrase pwrite bounds checking to avoid any potential overflow ... and do the same for pread. Signed-off-by: Chris Wilson Cc: stable@kernel.org --- drivers/gpu/drm/i915/i915_gem.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) (limited to 'drivers/gpu/drm/i915/i915_gem.c') diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 7749e78a730..aa959c4a183 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -471,12 +471,8 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, return -ENOENT; obj_priv = to_intel_bo(obj); - /* Bounds check source. - * - * XXX: This could use review for overflow issues... - */ - if (args->offset > obj->size || args->size > obj->size || - args->offset + args->size > obj->size) { + /* Bounds check source. */ + if (args->offset > obj->size || args->size > obj->size - args->offset) { ret = -EINVAL; goto err; } @@ -939,12 +935,8 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, return -ENOENT; obj_priv = to_intel_bo(obj); - /* Bounds check destination. - * - * XXX: This could use review for overflow issues... - */ - if (args->offset > obj->size || args->size > obj->size || - args->offset + args->size > obj->size) { + /* Bounds check destination. */ + if (args->offset > obj->size || args->size > obj->size - args->offset) { ret = -EINVAL; goto err; } -- cgit v1.2.3-70-g09d2