From a087ce704b802becbb4b0f2a20f2cb3f6911802e Mon Sep 17 00:00:00 2001 From: Mauro Carvalho Chehab Date: Wed, 27 Apr 2016 19:28:26 -0300 Subject: [media] media-device: dynamically allocate struct media_devnode struct media_devnode is currently embedded at struct media_device. While this works fine during normal usage, it leads to a race condition during devnode unregister. the problem is that drivers assume that, after calling media_device_unregister(), the struct that contains media_device can be freed. This is not true, as it can't be freed until userspace closes all opened /dev/media devnodes. In other words, if the media devnode is still open, and media_device gets freed, any call to an ioctl will make the core to try to access struct media_device, with will cause an use-after-free and even GPF. Fix this by dynamically allocating the struct media_devnode and only freeing it when it is safe. Signed-off-by: Mauro Carvalho Chehab Signed-off-by: Mauro Carvalho Chehab --- drivers/media/media-device.c | 44 ++++++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 14 deletions(-) (limited to 'drivers/media/media-device.c') diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index a1cd50f331f1..e61fa66d95df 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -423,7 +423,7 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { struct media_devnode *devnode = media_devnode_data(filp); - struct media_device *dev = to_media_device(devnode); + struct media_device *dev = devnode->media_dev; long ret; mutex_lock(&dev->graph_mutex); @@ -495,7 +495,7 @@ static long media_device_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { struct media_devnode *devnode = media_devnode_data(filp); - struct media_device *dev = to_media_device(devnode); + struct media_device *dev = devnode->media_dev; long ret; switch (cmd) { @@ -531,7 +531,8 @@ static const struct media_file_operations media_device_fops = { static ssize_t show_model(struct device *cd, struct device_attribute *attr, char *buf) { - struct media_device *mdev = to_media_device(to_media_devnode(cd)); + struct media_devnode *devnode = to_media_devnode(cd); + struct media_device *mdev = devnode->media_dev; return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model); } @@ -704,23 +705,34 @@ EXPORT_SYMBOL_GPL(media_device_cleanup); int __must_check __media_device_register(struct media_device *mdev, struct module *owner) { + struct media_devnode *devnode; int ret; + devnode = kzalloc(sizeof(*devnode), GFP_KERNEL); + if (!devnode) + return -ENOMEM; + /* Register the device node. */ - mdev->devnode.fops = &media_device_fops; - mdev->devnode.parent = mdev->dev; - mdev->devnode.release = media_device_release; + mdev->devnode = devnode; + devnode->fops = &media_device_fops; + devnode->parent = mdev->dev; + devnode->release = media_device_release; /* Set version 0 to indicate user-space that the graph is static */ mdev->topology_version = 0; - ret = media_devnode_register(&mdev->devnode, owner); - if (ret < 0) + ret = media_devnode_register(mdev, devnode, owner); + if (ret < 0) { + mdev->devnode = NULL; + kfree(devnode); return ret; + } - ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); + ret = device_create_file(&devnode->dev, &dev_attr_model); if (ret < 0) { - media_devnode_unregister(&mdev->devnode); + mdev->devnode = NULL; + media_devnode_unregister(devnode); + kfree(devnode); return ret; } @@ -771,7 +783,7 @@ void media_device_unregister(struct media_device *mdev) mutex_lock(&mdev->graph_mutex); /* Check if mdev was ever registered at all */ - if (!media_devnode_is_registered(&mdev->devnode)) { + if (!media_devnode_is_registered(mdev->devnode)) { mutex_unlock(&mdev->graph_mutex); return; } @@ -794,9 +806,13 @@ void media_device_unregister(struct media_device *mdev) mutex_unlock(&mdev->graph_mutex); - device_remove_file(&mdev->devnode.dev, &dev_attr_model); - dev_dbg(mdev->dev, "Media device unregistering\n"); - media_devnode_unregister(&mdev->devnode); + dev_dbg(mdev->dev, "Media device unregistered\n"); + + /* Check if mdev devnode was registered */ + if (media_devnode_is_registered(mdev->devnode)) { + device_remove_file(&mdev->devnode->dev, &dev_attr_model); + media_devnode_unregister(mdev->devnode); + } } EXPORT_SYMBOL_GPL(media_device_unregister); -- cgit v1.2.3-59-g8ed1b From 5b28dde51d0ccc54cee70756e1800d70bed7114a Mon Sep 17 00:00:00 2001 From: Shuah Khan Date: Wed, 4 May 2016 16:48:28 -0300 Subject: [media] media: fix use-after-free in cdev_put() when app exits after driver unbind When driver unbinds while media_ioctl is in progress, cdev_put() fails with when app exits after driver unbinds. Add devnode struct device kobj as the cdev parent kobject. cdev_add() gets a reference to it and releases it in cdev_del() ensuring that the devnode is not deallocated as long as the application has the device file open. media_devnode_register() initializes the struct device kobj before calling cdev_add(). media_devnode_unregister() does cdev_del() and then deletes the device. devnode is released when the last reference to the struct device is gone. This problem is found on uvcvideo, em28xx, and au0828 drivers and fix has been tested on all three. kernel: [ 193.599736] BUG: KASAN: use-after-free in cdev_put+0x4e/0x50 kernel: [ 193.599745] Read of size 8 by task media_device_te/1851 kernel: [ 193.599792] INFO: Allocated in __media_device_register+0x54 kernel: [ 193.599951] INFO: Freed in media_devnode_release+0xa4/0xc0 kernel: [ 193.601083] Call Trace: kernel: [ 193.601093] [] dump_stack+0x67/0x94 kernel: [ 193.601102] [] print_trailer+0x112/0x1a0 kernel: [ 193.601111] [] object_err+0x34/0x40 kernel: [ 193.601119] [] kasan_report_error+0x224/0x530 kernel: [ 193.601128] [] ? kzfree+0x2d/0x40 kernel: [ 193.601137] [] ? kfree+0x1d2/0x1f0 kernel: [ 193.601154] [] ? cdev_put+0x4e/0x50 kernel: [ 193.601162] [] cdev_put+0x4e/0x50 kernel: [ 193.601170] [] __fput+0x52b/0x6c0 kernel: [ 193.601179] [] ? switch_task_namespaces+0x2a kernel: [ 193.601188] [] ____fput+0xe/0x10 kernel: [ 193.601196] [] task_work_run+0x133/0x1f0 kernel: [ 193.601204] [] ? switch_task_namespaces+0x5e kernel: [ 193.601213] [] do_exit+0x72c/0x2c20 kernel: [ 193.601224] [] ? release_task+0x1250/0x1250 - - - kernel: [ 193.601360] [] ? exit_to_usermode_loop+0xe7 kernel: [ 193.601368] [] exit_to_usermode_loop+0x120 kernel: [ 193.601376] [] syscall_return_slowpath+0x16a kernel: [ 193.601386] [] entry_SYSCALL_64_fastpath+0xa6 Signed-off-by: Shuah Khan Tested-by: Mauro Carvalho Chehab Signed-off-by: Mauro Carvalho Chehab --- drivers/media/media-device.c | 6 ++++-- drivers/media/media-devnode.c | 48 ++++++++++++++++++++++++++----------------- 2 files changed, 33 insertions(+), 21 deletions(-) (limited to 'drivers/media/media-device.c') diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index e61fa66d95df..33a99524216a 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -723,16 +723,16 @@ int __must_check __media_device_register(struct media_device *mdev, ret = media_devnode_register(mdev, devnode, owner); if (ret < 0) { + /* devnode free is handled in media_devnode_*() */ mdev->devnode = NULL; - kfree(devnode); return ret; } ret = device_create_file(&devnode->dev, &dev_attr_model); if (ret < 0) { + /* devnode free is handled in media_devnode_*() */ mdev->devnode = NULL; media_devnode_unregister(devnode); - kfree(devnode); return ret; } @@ -812,6 +812,8 @@ void media_device_unregister(struct media_device *mdev) if (media_devnode_is_registered(mdev->devnode)) { device_remove_file(&mdev->devnode->dev, &dev_attr_model); media_devnode_unregister(mdev->devnode); + /* devnode free is handled in media_devnode_*() */ + mdev->devnode = NULL; } } EXPORT_SYMBOL_GPL(media_device_unregister); diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c index ecdc02d6ed83..5b605ff38daf 100644 --- a/drivers/media/media-devnode.c +++ b/drivers/media/media-devnode.c @@ -63,13 +63,8 @@ static void media_devnode_release(struct device *cd) struct media_devnode *devnode = to_media_devnode(cd); mutex_lock(&media_devnode_lock); - - /* Delete the cdev on this minor as well */ - cdev_del(&devnode->cdev); - /* Mark device node number as free */ clear_bit(devnode->minor, media_devnode_nums); - mutex_unlock(&media_devnode_lock); /* Release media_devnode and perform other cleanups as needed. */ @@ -77,6 +72,7 @@ static void media_devnode_release(struct device *cd) devnode->release(devnode); kfree(devnode); + pr_debug("%s: Media Devnode Deallocated\n", __func__); } static struct bus_type media_bus_type = { @@ -205,6 +201,8 @@ static int media_release(struct inode *inode, struct file *filp) /* decrease the refcount unconditionally since the release() return value is ignored. */ put_device(&devnode->dev); + + pr_debug("%s: Media Release\n", __func__); return 0; } @@ -235,6 +233,7 @@ int __must_check media_devnode_register(struct media_device *mdev, if (minor == MEDIA_NUM_DEVICES) { mutex_unlock(&media_devnode_lock); pr_err("could not get a free minor\n"); + kfree(devnode); return -ENFILE; } @@ -244,27 +243,31 @@ int __must_check media_devnode_register(struct media_device *mdev, devnode->minor = minor; devnode->media_dev = mdev; + /* Part 1: Initialize dev now to use dev.kobj for cdev.kobj.parent */ + devnode->dev.bus = &media_bus_type; + devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor); + devnode->dev.release = media_devnode_release; + if (devnode->parent) + devnode->dev.parent = devnode->parent; + dev_set_name(&devnode->dev, "media%d", devnode->minor); + device_initialize(&devnode->dev); + /* Part 2: Initialize and register the character device */ cdev_init(&devnode->cdev, &media_devnode_fops); devnode->cdev.owner = owner; + devnode->cdev.kobj.parent = &devnode->dev.kobj; ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1); if (ret < 0) { pr_err("%s: cdev_add failed\n", __func__); - goto error; + goto cdev_add_error; } - /* Part 3: Register the media device */ - devnode->dev.bus = &media_bus_type; - devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor); - devnode->dev.release = media_devnode_release; - if (devnode->parent) - devnode->dev.parent = devnode->parent; - dev_set_name(&devnode->dev, "media%d", devnode->minor); - ret = device_register(&devnode->dev); + /* Part 3: Add the media device */ + ret = device_add(&devnode->dev); if (ret < 0) { - pr_err("%s: device_register failed\n", __func__); - goto error; + pr_err("%s: device_add failed\n", __func__); + goto device_add_error; } /* Part 4: Activate this minor. The char device can now be used. */ @@ -272,12 +275,15 @@ int __must_check media_devnode_register(struct media_device *mdev, return 0; -error: - mutex_lock(&media_devnode_lock); +device_add_error: cdev_del(&devnode->cdev); +cdev_add_error: + mutex_lock(&media_devnode_lock); clear_bit(devnode->minor, media_devnode_nums); + devnode->media_dev = NULL; mutex_unlock(&media_devnode_lock); + put_device(&devnode->dev); return ret; } @@ -289,8 +295,12 @@ void media_devnode_unregister(struct media_devnode *devnode) mutex_lock(&media_devnode_lock); clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags); + /* Delete the cdev on this minor as well */ + cdev_del(&devnode->cdev); mutex_unlock(&media_devnode_lock); - device_unregister(&devnode->dev); + device_del(&devnode->dev); + devnode->media_dev = NULL; + put_device(&devnode->dev); } /* -- cgit v1.2.3-59-g8ed1b From 6f0dd24a084a17f9984dd49dffbf7055bf123993 Mon Sep 17 00:00:00 2001 From: Shuah Khan Date: Fri, 10 Jun 2016 14:37:23 -0300 Subject: [media] media: fix media devnode ioctl/syscall and unregister race Media devnode open/ioctl could be in progress when media device unregister is initiated. System calls and ioctls check media device registered status at the beginning, however, there is a window where unregister could be in progress without changing the media devnode status to unregistered. process 1 process 2 fd = open(/dev/media0) media_devnode_is_registered() (returns true here) media_device_unregister() (unregister is in progress and devnode isn't unregistered yet) ... ioctl(fd, ...) __media_ioctl() media_devnode_is_registered() (returns true here) ... media_devnode_unregister() ... (driver releases the media device memory) media_device_ioctl() (By this point devnode->media_dev does not point to allocated memory. use-after free in in mutex_lock_nested) BUG: KASAN: use-after-free in mutex_lock_nested+0x79c/0x800 at addr ffff8801ebe914f0 Fix it by clearing register bit when unregister starts to avoid the race. process 1 process 2 fd = open(/dev/media0) media_devnode_is_registered() (could return true here) media_device_unregister() (clear the register bit, then start unregister.) ... ioctl(fd, ...) __media_ioctl() media_devnode_is_registered() (return false here, ioctl returns I/O error, and will not access media device memory) ... media_devnode_unregister() ... (driver releases the media device memory) Signed-off-by: Shuah Khan Suggested-by: Sakari Ailus Reported-by: Mauro Carvalho Chehab Tested-by: Mauro Carvalho Chehab Signed-off-by: Mauro Carvalho Chehab --- drivers/media/media-device.c | 15 ++++++++------- drivers/media/media-devnode.c | 8 +++++++- include/media/media-devnode.h | 16 ++++++++++++++-- 3 files changed, 29 insertions(+), 10 deletions(-) (limited to 'drivers/media/media-device.c') diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index 33a99524216a..1795abeda658 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -732,6 +732,7 @@ int __must_check __media_device_register(struct media_device *mdev, if (ret < 0) { /* devnode free is handled in media_devnode_*() */ mdev->devnode = NULL; + media_devnode_unregister_prepare(devnode); media_devnode_unregister(devnode); return ret; } @@ -788,6 +789,9 @@ void media_device_unregister(struct media_device *mdev) return; } + /* Clear the devnode register bit to avoid races with media dev open */ + media_devnode_unregister_prepare(mdev->devnode); + /* Remove all entities from the media device */ list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) __media_device_unregister_entity(entity); @@ -808,13 +812,10 @@ void media_device_unregister(struct media_device *mdev) dev_dbg(mdev->dev, "Media device unregistered\n"); - /* Check if mdev devnode was registered */ - if (media_devnode_is_registered(mdev->devnode)) { - device_remove_file(&mdev->devnode->dev, &dev_attr_model); - media_devnode_unregister(mdev->devnode); - /* devnode free is handled in media_devnode_*() */ - mdev->devnode = NULL; - } + device_remove_file(&mdev->devnode->dev, &dev_attr_model); + media_devnode_unregister(mdev->devnode); + /* devnode free is handled in media_devnode_*() */ + mdev->devnode = NULL; } EXPORT_SYMBOL_GPL(media_device_unregister); diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c index 5b605ff38daf..f2772ba6f611 100644 --- a/drivers/media/media-devnode.c +++ b/drivers/media/media-devnode.c @@ -287,7 +287,7 @@ cdev_add_error: return ret; } -void media_devnode_unregister(struct media_devnode *devnode) +void media_devnode_unregister_prepare(struct media_devnode *devnode) { /* Check if devnode was ever registered at all */ if (!media_devnode_is_registered(devnode)) @@ -295,6 +295,12 @@ void media_devnode_unregister(struct media_devnode *devnode) mutex_lock(&media_devnode_lock); clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags); + mutex_unlock(&media_devnode_lock); +} + +void media_devnode_unregister(struct media_devnode *devnode) +{ + mutex_lock(&media_devnode_lock); /* Delete the cdev on this minor as well */ cdev_del(&devnode->cdev); mutex_unlock(&media_devnode_lock); diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h index 5bb3b0e86d73..f0b7dd79fb92 100644 --- a/include/media/media-devnode.h +++ b/include/media/media-devnode.h @@ -125,6 +125,19 @@ int __must_check media_devnode_register(struct media_device *mdev, struct media_devnode *devnode, struct module *owner); +/** + * media_devnode_unregister_prepare - clear the media device node register bit + * @devnode: the device node to prepare for unregister + * + * This clears the passed device register bit. Future open calls will be met + * with errors. Should be called before media_devnode_unregister() to avoid + * races with unregister and device file open calls. + * + * This function can safely be called if the device node has never been + * registered or has already been unregistered. + */ +void media_devnode_unregister_prepare(struct media_devnode *devnode); + /** * media_devnode_unregister - unregister a media device node * @devnode: the device node to unregister @@ -132,8 +145,7 @@ int __must_check media_devnode_register(struct media_device *mdev, * This unregisters the passed device. Future open calls will be met with * errors. * - * This function can safely be called if the device node has never been - * registered or has already been unregistered. + * Should be called after media_devnode_unregister_prepare() */ void media_devnode_unregister(struct media_devnode *devnode); -- cgit v1.2.3-59-g8ed1b