3.16.83-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: Shuah Khan shuahkh@osg.samsung.com
commit 6f0dd24a084a17f9984dd49dffbf7055bf123993 upstream.
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 shuahkh@osg.samsung.com Suggested-by: Sakari Ailus sakari.ailus@linux.intel.com Reported-by: Mauro Carvalho Chehab mchehab@osg.samsung.com Tested-by: Mauro Carvalho Chehab mchehab@osg.samsung.com Signed-off-by: Mauro Carvalho Chehab mchehab@s-opensource.com [bwh: Backported to 3.16: adjut filename, context] Signed-off-by: Ben Hutchings ben@decadent.org.uk --- --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -407,6 +407,7 @@ int __must_check __media_device_register if (ret < 0) { /* devnode free is handled in media_devnode_*() */ mdev->devnode = NULL; + media_devnode_unregister_prepare(devnode); media_devnode_unregister(devnode); return ret; } @@ -425,16 +426,16 @@ void media_device_unregister(struct medi struct media_entity *entity; struct media_entity *next;
+ /* Clear the devnode register bit to avoid races with media dev open */ + media_devnode_unregister_prepare(mdev->devnode); + list_for_each_entry_safe(entity, next, &mdev->entities, list) media_device_unregister_entity(entity);
- /* 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);
--- a/drivers/media/media-devnode.c +++ b/drivers/media/media-devnode.c @@ -302,6 +302,17 @@ cdev_add_error: return ret; }
+void media_devnode_unregister_prepare(struct media_devnode *devnode) +{ + /* Check if devnode was ever registered at all */ + if (!media_devnode_is_registered(devnode)) + return; + + mutex_lock(&media_devnode_lock); + clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags); + mutex_unlock(&media_devnode_lock); +} + /** * media_devnode_unregister - unregister a media device node * @devnode: the device node to unregister @@ -309,17 +320,11 @@ cdev_add_error: * 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) { - /* Check if devnode was ever registered at all */ - if (!media_devnode_is_registered(devnode)) - return; - 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); --- a/include/media/media-devnode.h +++ b/include/media/media-devnode.h @@ -89,6 +89,20 @@ struct media_devnode { 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); + void media_devnode_unregister(struct media_devnode *devnode);
static inline struct media_devnode *media_devnode_data(struct file *filp)