When testing with a device which uses the drm/udl driver, KASAN shows that on hot-remove we have a use-after-free:
================================================================== BUG: KASAN: use-after-free in do_raw_spin_lock+0x1c/0xd0 Read of size 4 at addr ffff888385e325fc by task kworker/2:2/47
CPU: 2 PID: 47 Comm: kworker/2:2 Tainted: G U 4.14.133 #19 Hardware name: GOOGLE Samus, BIOS Google_Samus.6300.276.0 08/17/2016 Workqueue: events drm_mode_rmfb_work_fn Call Trace: dump_stack+0x67/0x92 print_address_description+0x80/0x2d6 ? do_raw_spin_lock+0x1c/0xd0 kasan_report+0x255/0x295 do_raw_spin_lock+0x1c/0xd0 _raw_spin_lock_irqsave+0x42/0x4e ? down_timeout+0x19/0x58 down_timeout+0x19/0x58 udl_get_urb+0x3d/0x13b ? drm_helper_encoder_in_use+0xc2/0xe1 udl_crtc_dpms+0x45/0x274 __drm_helper_disable_unused_functions+0xed/0x150 drm_crtc_helper_set_config+0x22d/0xfc2 ? lock_acquire+0x1e4/0x21a ? modeset_lock+0x165/0x20e ? __mutex_trylock+0x9/0x11 ? debug_lockdep_rcu_enabled+0x2a/0x59 __drm_mode_set_config_internal+0xf3/0x240 drm_crtc_force_disable+0x68/0x83 drm_framebuffer_remove+0x10b/0x1af drm_mode_rmfb_work_fn+0x8d/0x9b process_one_work+0x42f/0x7a2 worker_thread+0x3a4/0x483 ? flush_delayed_work+0x64/0x64 kthread+0x1e7/0x1f7 ? __init_completion+0x2c/0x2c ret_from_fork+0x3a/0x50
Allocated by task 1959: save_stack+0x46/0xce kasan_kmalloc+0x99/0xa8 kmem_cache_alloc_trace+0x10d/0x133 udl_driver_load+0x59/0x7fe drm_dev_register+0x16b/0x2fd udl_usb_probe+0x4f/0xa6 usb_probe_interface+0x26a/0x31d driver_probe_device+0x1d5/0x411 bus_for_each_drv+0xbe/0xe5 __device_attach+0xdd/0x15b bus_probe_device+0x5a/0x10b device_add+0x468/0x7fb usb_set_configuration+0x978/0x9e5 generic_probe+0x45/0x77 driver_probe_device+0x1d5/0x411 bus_for_each_drv+0xbe/0xe5 __device_attach+0xdd/0x15b bus_probe_device+0x5a/0x10b device_add+0x468/0x7fb usb_new_device+0x51d/0x6a1 hub_event+0xee4/0x1639 process_one_work+0x42f/0x7a2 worker_thread+0x31c/0x483 kthread+0x1e7/0x1f7 ret_from_fork+0x3a/0x50
Freed by task 1959: save_stack+0x46/0xce kasan_slab_free+0x8a/0xac slab_free_hook+0x52/0x5c kfree+0x1a5/0x228 drm_dev_unregister+0xa6/0x16c drm_dev_unplug+0x12/0x5b usb_unbind_interface+0xc8/0x2c1 device_release_driver_internal+0x1e4/0x302 bus_remove_device+0x1b9/0x1e4 device_del+0x275/0x42d usb_disable_device+0x112/0x2cb usb_disconnect+0xef/0x28e usb_disconnect+0xe0/0x28e hub_event+0x7cc/0x1639 process_one_work+0x42f/0x7a2 worker_thread+0x31c/0x483 kthread+0x1e7/0x1f7 ret_from_fork+0x3a/0x50
The buggy address belongs to the object at ffff888385e32588 which belongs to the cache kmalloc-2048 of size 2048 The buggy address is located 116 bytes inside of 2048-byte region [ffff888385e32588, ffff888385e32d88) The buggy address belongs to the page: page:ffffea000e178c00 count:1 mapcount:0 mapping: (null) index:0x0 compound_mapcount: 0 flags: 0x8000000000008100(slab|head) raw: 8000000000008100 0000000000000000 0000000000000000 00000001000d000d raw: ffffea000ee71e20 ffffea000ee6d620 ffff88842d00d0c0 0000000000000000 page dumped because: kasan: bad access detected
Memory state around the buggy address: ffff888385e32480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc ffff888385e32500: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff888385e32580: fc fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^ ffff888385e32600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ffff888385e32680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ==================================================================
This happens 100% of the time and is resolved by the following patch upstream:
commit 6ecac85eadb9 ("drm/udl: move to embedding drm device inside udl device.")
This patch is the second in this series, and requires the first patch as a dependency. This series apples cleanly to v4.14.133.
Dave Airlie (2): drm/udl: introduce a macro to convert dev to udl. drm/udl: move to embedding drm device inside udl device.
drivers/gpu/drm/udl/udl_drv.c | 56 +++++++++++++++++++++++++++------- drivers/gpu/drm/udl/udl_drv.h | 9 +++--- drivers/gpu/drm/udl/udl_fb.c | 12 ++++---- drivers/gpu/drm/udl/udl_main.c | 35 ++++++--------------- 4 files changed, 65 insertions(+), 47 deletions(-)
From: Dave Airlie airlied@redhat.com
commit fd96e0dba19c53c2d66f2a398716bb74df8ca85e upstream.
This just makes it easier to later embed drm into udl.
[rez] Regarding the backport to v4.14.y, the only difference is due to the fact that in v4.14.y the udl_gem_mmap() function doesn't have a local 'struct udl_device' pointer so it didn't need to be converted.
Reviewed-by: Alex Deucher alexander.deucher@amd.com Signed-off-by: Dave Airlie airlied@redhat.com Link: https://patchwork.freedesktop.org/patch/msgid/20190405031715.5959-3-airlied@... Signed-off-by: Ross Zwisler zwisler@google.com --- drivers/gpu/drm/udl/udl_drv.h | 2 ++ drivers/gpu/drm/udl/udl_fb.c | 10 +++++----- drivers/gpu/drm/udl/udl_main.c | 12 ++++++------ 3 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 307455dd6526..ba0146e06b1e 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -68,6 +68,8 @@ struct udl_device { atomic_t cpu_kcycles_used; /* transpired during pixel processing */ };
+#define to_udl(x) ((x)->dev_private) + struct udl_gem_object { struct drm_gem_object base; struct page **pages; diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 491f1892b50e..1e78767df06c 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -82,7 +82,7 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, int width, int height) { struct drm_device *dev = fb->base.dev; - struct udl_device *udl = dev->dev_private; + struct udl_device *udl = to_udl(dev); int i, ret; char *cmd; cycles_t start_cycles, end_cycles; @@ -210,7 +210,7 @@ static int udl_fb_open(struct fb_info *info, int user) { struct udl_fbdev *ufbdev = info->par; struct drm_device *dev = ufbdev->ufb.base.dev; - struct udl_device *udl = dev->dev_private; + struct udl_device *udl = to_udl(dev);
/* If the USB device is gone, we don't accept new opens */ if (drm_dev_is_unplugged(udl->ddev)) @@ -441,7 +441,7 @@ static void udl_fbdev_destroy(struct drm_device *dev,
int udl_fbdev_init(struct drm_device *dev) { - struct udl_device *udl = dev->dev_private; + struct udl_device *udl = to_udl(dev); int bpp_sel = fb_bpp; struct udl_fbdev *ufbdev; int ret; @@ -480,7 +480,7 @@ int udl_fbdev_init(struct drm_device *dev)
void udl_fbdev_cleanup(struct drm_device *dev) { - struct udl_device *udl = dev->dev_private; + struct udl_device *udl = to_udl(dev); if (!udl->fbdev) return;
@@ -491,7 +491,7 @@ void udl_fbdev_cleanup(struct drm_device *dev)
void udl_fbdev_unplug(struct drm_device *dev) { - struct udl_device *udl = dev->dev_private; + struct udl_device *udl = to_udl(dev); struct udl_fbdev *ufbdev; if (!udl->fbdev) return; diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 60866b422f81..05c14c80024c 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -28,7 +28,7 @@ static int udl_parse_vendor_descriptor(struct drm_device *dev, struct usb_device *usbdev) { - struct udl_device *udl = dev->dev_private; + struct udl_device *udl = to_udl(dev); char *desc; char *buf; char *desc_end; @@ -164,7 +164,7 @@ void udl_urb_completion(struct urb *urb)
static void udl_free_urb_list(struct drm_device *dev) { - struct udl_device *udl = dev->dev_private; + struct udl_device *udl = to_udl(dev); int count = udl->urbs.count; struct list_head *node; struct urb_node *unode; @@ -198,7 +198,7 @@ static void udl_free_urb_list(struct drm_device *dev)
static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size) { - struct udl_device *udl = dev->dev_private; + struct udl_device *udl = to_udl(dev); struct urb *urb; struct urb_node *unode; char *buf; @@ -262,7 +262,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
struct urb *udl_get_urb(struct drm_device *dev) { - struct udl_device *udl = dev->dev_private; + struct udl_device *udl = to_udl(dev); int ret = 0; struct list_head *entry; struct urb_node *unode; @@ -296,7 +296,7 @@ struct urb *udl_get_urb(struct drm_device *dev)
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len) { - struct udl_device *udl = dev->dev_private; + struct udl_device *udl = to_udl(dev); int ret;
BUG_ON(len > udl->urbs.size); @@ -372,7 +372,7 @@ int udl_drop_usb(struct drm_device *dev)
void udl_driver_unload(struct drm_device *dev) { - struct udl_device *udl = dev->dev_private; + struct udl_device *udl = to_udl(dev);
if (udl->urbs.count) udl_free_urb_list(dev);
From: Dave Airlie airlied@redhat.com
commit 6ecac85eadb9d4065b9038fa3d3c66d49038e14b upstream.
This should help with some of the lifetime issues, and move us away from load/unload.
[rez] Regarding the backport to v4.14.y, the only difference is due to the fact that in v4.14.y the udl_usb_probe() function still uses drm_dev_unref() instead of drm_dev_put().
Acked-by: Alex Deucher alexander.deucher@amd.com Signed-off-by: Dave Airlie airlied@redhat.com Link: https://patchwork.freedesktop.org/patch/msgid/20190405031715.5959-4-airlied@... Signed-off-by: Ross Zwisler zwisler@google.com --- drivers/gpu/drm/udl/udl_drv.c | 56 +++++++++++++++++++++++++++------- drivers/gpu/drm/udl/udl_drv.h | 9 +++--- drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_main.c | 23 ++------------ 4 files changed, 53 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index b45ac6bc8add..b428c3da7576 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -43,10 +43,16 @@ static const struct file_operations udl_driver_fops = { .llseek = noop_llseek, };
+static void udl_driver_release(struct drm_device *dev) +{ + udl_fini(dev); + udl_modeset_cleanup(dev); + drm_dev_fini(dev); + kfree(dev); +} + static struct drm_driver driver = { .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME, - .load = udl_driver_load, - .unload = udl_driver_unload, .release = udl_driver_release,
/* gem hooks */ @@ -70,28 +76,56 @@ static struct drm_driver driver = { .patchlevel = DRIVER_PATCHLEVEL, };
+static struct udl_device *udl_driver_create(struct usb_interface *interface) +{ + struct usb_device *udev = interface_to_usbdev(interface); + struct udl_device *udl; + int r; + + udl = kzalloc(sizeof(*udl), GFP_KERNEL); + if (!udl) + return ERR_PTR(-ENOMEM); + + r = drm_dev_init(&udl->drm, &driver, &interface->dev); + if (r) { + kfree(udl); + return ERR_PTR(r); + } + + udl->udev = udev; + udl->drm.dev_private = udl; + + r = udl_init(udl); + if (r) { + drm_dev_fini(&udl->drm); + kfree(udl); + return ERR_PTR(r); + } + + usb_set_intfdata(interface, udl); + return udl; +} + static int udl_usb_probe(struct usb_interface *interface, const struct usb_device_id *id) { - struct usb_device *udev = interface_to_usbdev(interface); - struct drm_device *dev; int r; + struct udl_device *udl;
- dev = drm_dev_alloc(&driver, &interface->dev); - if (IS_ERR(dev)) - return PTR_ERR(dev); + udl = udl_driver_create(interface); + if (IS_ERR(udl)) + return PTR_ERR(udl);
- r = drm_dev_register(dev, (unsigned long)udev); + r = drm_dev_register(&udl->drm, 0); if (r) goto err_free;
- usb_set_intfdata(interface, dev); - DRM_INFO("Initialized udl on minor %d\n", dev->primary->index); + DRM_INFO("Initialized udl on minor %d\n", udl->drm.primary->index);
return 0;
err_free: - drm_dev_unref(dev); + drm_dev_unref(&udl->drm); return r; }
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index ba0146e06b1e..d5a5dcd15dd8 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -49,8 +49,8 @@ struct urb_list { struct udl_fbdev;
struct udl_device { + struct drm_device drm; struct device *dev; - struct drm_device *ddev; struct usb_device *udev; struct drm_crtc *crtc;
@@ -68,7 +68,7 @@ struct udl_device { atomic_t cpu_kcycles_used; /* transpired during pixel processing */ };
-#define to_udl(x) ((x)->dev_private) +#define to_udl(x) container_of(x, struct udl_device, drm)
struct udl_gem_object { struct drm_gem_object base; @@ -101,9 +101,8 @@ struct urb *udl_get_urb(struct drm_device *dev); int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len); void udl_urb_completion(struct urb *urb);
-int udl_driver_load(struct drm_device *dev, unsigned long flags); -void udl_driver_unload(struct drm_device *dev); -void udl_driver_release(struct drm_device *dev); +int udl_init(struct udl_device *udl); +void udl_fini(struct drm_device *dev);
int udl_fbdev_init(struct drm_device *dev); void udl_fbdev_cleanup(struct drm_device *dev); diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 1e78767df06c..f41fd0684ce4 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -213,7 +213,7 @@ static int udl_fb_open(struct fb_info *info, int user) struct udl_device *udl = to_udl(dev);
/* If the USB device is gone, we don't accept new opens */ - if (drm_dev_is_unplugged(udl->ddev)) + if (drm_dev_is_unplugged(&udl->drm)) return -ENODEV;
ufbdev->fb_count++; diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 05c14c80024c..124428f33e1e 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -311,20 +311,12 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len) return ret; }
-int udl_driver_load(struct drm_device *dev, unsigned long flags) +int udl_init(struct udl_device *udl) { - struct usb_device *udev = (void*)flags; - struct udl_device *udl; + struct drm_device *dev = &udl->drm; int ret = -ENOMEM;
DRM_DEBUG("\n"); - udl = kzalloc(sizeof(struct udl_device), GFP_KERNEL); - if (!udl) - return -ENOMEM; - - udl->udev = udev; - udl->ddev = dev; - dev->dev_private = udl;
if (!udl_parse_vendor_descriptor(dev, udl->udev)) { ret = -ENODEV; @@ -359,7 +351,6 @@ int udl_driver_load(struct drm_device *dev, unsigned long flags) err: if (udl->urbs.count) udl_free_urb_list(dev); - kfree(udl); DRM_ERROR("%d\n", ret); return ret; } @@ -370,7 +361,7 @@ int udl_drop_usb(struct drm_device *dev) return 0; }
-void udl_driver_unload(struct drm_device *dev) +void udl_fini(struct drm_device *dev) { struct udl_device *udl = to_udl(dev);
@@ -378,12 +369,4 @@ void udl_driver_unload(struct drm_device *dev) udl_free_urb_list(dev);
udl_fbdev_cleanup(dev); - kfree(udl); -} - -void udl_driver_release(struct drm_device *dev) -{ - udl_modeset_cleanup(dev); - drm_dev_fini(dev); - kfree(dev); }
On Mon, Jul 15, 2019 at 01:36:16PM -0600, Ross Zwisler wrote:
This patch is the second in this series, and requires the first patch as a dependency. This series apples cleanly to v4.14.133.
Hm, we don't need ac3b35f11a06 here? Why not? I'd love to document that with the backport.
-- Thanks, Sasha
On Mon, Jul 15, 2019 at 09:13:08PM -0400, Sasha Levin wrote:
On Mon, Jul 15, 2019 at 01:36:16PM -0600, Ross Zwisler wrote:
This patch is the second in this series, and requires the first patch as a dependency. This series apples cleanly to v4.14.133.
Hm, we don't need ac3b35f11a06 here? Why not? I'd love to document that with the backport.
Nope, we don't need that patch in the v4.14 backport.
In v4.19.y we have two functions, drm_dev_put() and drm_dev_unref(), which are aliases for one another (drm_dev_unref() just calls drm_dev_put()). drm_dev_unref() is the older of the two, and was introduced back in v4.0. drm_dev_put() was introduced in v4.15 with
9a96f55034e41 drm: introduce drm_dev_{get/put} functions
and slowly callers were moved from the old name (_unref) to the new name (_put). The patch you mentioned, ac3b35f11a06, is one such patch where we are replacing a drm_dev_unref() call with a drm_dev_put() call. This doesn't have a functional change, but was necessary so that the third patch in the v4.19.y series I sent would apply cleanly.
For the v4.14.y series, though, the drm_dev_put() function hasn't yet been defined and everyone is still using drm_dev_unref(). So, we don't need a backport of ac3b35f11a06, and I also had a small backport change in the last patch of the v4.14.y series where I had to change a drm_dev_put() call with a drm_dev_unref() call.
Just for posterity, the drm_dev_unref() calls were eventually all changed to drm_dev_put() in v5.0, and drm_dev_unref() was removed entirely. That happened with the following two patches:
808bad32ea423 drm: replace "drm_dev_unref" function with "drm_dev_put" ba1d345401476 drm: remove deprecated "drm_dev_unref" function
On Tue, Jul 16, 2019 at 10:08:28AM -0600, Ross Zwisler wrote:
On Mon, Jul 15, 2019 at 09:13:08PM -0400, Sasha Levin wrote:
On Mon, Jul 15, 2019 at 01:36:16PM -0600, Ross Zwisler wrote:
This patch is the second in this series, and requires the first patch as a dependency. This series apples cleanly to v4.14.133.
Hm, we don't need ac3b35f11a06 here? Why not? I'd love to document that with the backport.
Nope, we don't need that patch in the v4.14 backport.
In v4.19.y we have two functions, drm_dev_put() and drm_dev_unref(), which are aliases for one another (drm_dev_unref() just calls drm_dev_put()). drm_dev_unref() is the older of the two, and was introduced back in v4.0. drm_dev_put() was introduced in v4.15 with
9a96f55034e41 drm: introduce drm_dev_{get/put} functions
and slowly callers were moved from the old name (_unref) to the new name (_put). The patch you mentioned, ac3b35f11a06, is one such patch where we are replacing a drm_dev_unref() call with a drm_dev_put() call. This doesn't have a functional change, but was necessary so that the third patch in the v4.19.y series I sent would apply cleanly.
For the v4.14.y series, though, the drm_dev_put() function hasn't yet been defined and everyone is still using drm_dev_unref(). So, we don't need a backport of ac3b35f11a06, and I also had a small backport change in the last patch of the v4.14.y series where I had to change a drm_dev_put() call with a drm_dev_unref() call.
Just for posterity, the drm_dev_unref() calls were eventually all changed to drm_dev_put() in v5.0, and drm_dev_unref() was removed entirely. That happened with the following two patches:
808bad32ea423 drm: replace "drm_dev_unref" function with "drm_dev_put" ba1d345401476 drm: remove deprecated "drm_dev_unref" function
Thank you for the explanation. I've queued both this and the 4.19 patches, and added your explanation to the 4.14 patch.
-- Thanks, Sasha
linux-stable-mirror@lists.linaro.org
-
Ross Zwisler -
Ross Zwisler -
Sasha Levin