Commit a087ce70 authored by Mauro Carvalho Chehab's avatar Mauro Carvalho Chehab Committed by Mauro Carvalho Chehab

[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: default avatarMauro Carvalho Chehab <mchehab@osg.samsung.com>
Signed-off-by: default avatarMauro Carvalho Chehab <mchehab@s-opensource.com>
parent 163f1e93
...@@ -423,7 +423,7 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd, ...@@ -423,7 +423,7 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg) unsigned long arg)
{ {
struct media_devnode *devnode = media_devnode_data(filp); 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; long ret;
mutex_lock(&dev->graph_mutex); mutex_lock(&dev->graph_mutex);
...@@ -495,7 +495,7 @@ static long media_device_compat_ioctl(struct file *filp, unsigned int cmd, ...@@ -495,7 +495,7 @@ static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg) unsigned long arg)
{ {
struct media_devnode *devnode = media_devnode_data(filp); 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; long ret;
switch (cmd) { switch (cmd) {
...@@ -531,7 +531,8 @@ static const struct media_file_operations media_device_fops = { ...@@ -531,7 +531,8 @@ static const struct media_file_operations media_device_fops = {
static ssize_t show_model(struct device *cd, static ssize_t show_model(struct device *cd,
struct device_attribute *attr, char *buf) 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); return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
} }
...@@ -704,23 +705,34 @@ EXPORT_SYMBOL_GPL(media_device_cleanup); ...@@ -704,23 +705,34 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
int __must_check __media_device_register(struct media_device *mdev, int __must_check __media_device_register(struct media_device *mdev,
struct module *owner) struct module *owner)
{ {
struct media_devnode *devnode;
int ret; int ret;
devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
if (!devnode)
return -ENOMEM;
/* Register the device node. */ /* Register the device node. */
mdev->devnode.fops = &media_device_fops; mdev->devnode = devnode;
mdev->devnode.parent = mdev->dev; devnode->fops = &media_device_fops;
mdev->devnode.release = media_device_release; devnode->parent = mdev->dev;
devnode->release = media_device_release;
/* Set version 0 to indicate user-space that the graph is static */ /* Set version 0 to indicate user-space that the graph is static */
mdev->topology_version = 0; mdev->topology_version = 0;
ret = media_devnode_register(&mdev->devnode, owner); ret = media_devnode_register(mdev, devnode, owner);
if (ret < 0) if (ret < 0) {
mdev->devnode = NULL;
kfree(devnode);
return ret; return ret;
}
ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); ret = device_create_file(&devnode->dev, &dev_attr_model);
if (ret < 0) { if (ret < 0) {
media_devnode_unregister(&mdev->devnode); mdev->devnode = NULL;
media_devnode_unregister(devnode);
kfree(devnode);
return ret; return ret;
} }
...@@ -771,7 +783,7 @@ void media_device_unregister(struct media_device *mdev) ...@@ -771,7 +783,7 @@ void media_device_unregister(struct media_device *mdev)
mutex_lock(&mdev->graph_mutex); mutex_lock(&mdev->graph_mutex);
/* Check if mdev was ever registered at all */ /* 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); mutex_unlock(&mdev->graph_mutex);
return; return;
} }
...@@ -794,9 +806,13 @@ void media_device_unregister(struct media_device *mdev) ...@@ -794,9 +806,13 @@ void media_device_unregister(struct media_device *mdev)
mutex_unlock(&mdev->graph_mutex); mutex_unlock(&mdev->graph_mutex);
device_remove_file(&mdev->devnode.dev, &dev_attr_model); dev_dbg(mdev->dev, "Media device unregistered\n");
dev_dbg(mdev->dev, "Media device unregistering\n");
media_devnode_unregister(&mdev->devnode); /* 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); EXPORT_SYMBOL_GPL(media_device_unregister);
......
...@@ -44,6 +44,7 @@ ...@@ -44,6 +44,7 @@
#include <linux/uaccess.h> #include <linux/uaccess.h>
#include <media/media-devnode.h> #include <media/media-devnode.h>
#include <media/media-device.h>
#define MEDIA_NUM_DEVICES 256 #define MEDIA_NUM_DEVICES 256
#define MEDIA_NAME "media" #define MEDIA_NAME "media"
...@@ -74,6 +75,8 @@ static void media_devnode_release(struct device *cd) ...@@ -74,6 +75,8 @@ static void media_devnode_release(struct device *cd)
/* Release media_devnode and perform other cleanups as needed. */ /* Release media_devnode and perform other cleanups as needed. */
if (devnode->release) if (devnode->release)
devnode->release(devnode); devnode->release(devnode);
kfree(devnode);
} }
static struct bus_type media_bus_type = { static struct bus_type media_bus_type = {
...@@ -219,7 +222,8 @@ static const struct file_operations media_devnode_fops = { ...@@ -219,7 +222,8 @@ static const struct file_operations media_devnode_fops = {
.llseek = no_llseek, .llseek = no_llseek,
}; };
int __must_check media_devnode_register(struct media_devnode *devnode, int __must_check media_devnode_register(struct media_device *mdev,
struct media_devnode *devnode,
struct module *owner) struct module *owner)
{ {
int minor; int minor;
...@@ -238,6 +242,7 @@ int __must_check media_devnode_register(struct media_devnode *devnode, ...@@ -238,6 +242,7 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
mutex_unlock(&media_devnode_lock); mutex_unlock(&media_devnode_lock);
devnode->minor = minor; devnode->minor = minor;
devnode->media_dev = mdev;
/* Part 2: Initialize and register the character device */ /* Part 2: Initialize and register the character device */
cdev_init(&devnode->cdev, &media_devnode_fops); cdev_init(&devnode->cdev, &media_devnode_fops);
......
...@@ -142,7 +142,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) ...@@ -142,7 +142,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
struct media_device *mdev = dev->media_dev; struct media_device *mdev = dev->media_dev;
struct media_entity_notify *notify, *nextp; struct media_entity_notify *notify, *nextp;
if (!mdev || !media_devnode_is_registered(&mdev->devnode)) if (!mdev || !media_devnode_is_registered(mdev->devnode))
return; return;
/* Remove au0828 entity_notify callbacks */ /* Remove au0828 entity_notify callbacks */
...@@ -482,7 +482,7 @@ static int au0828_media_device_register(struct au0828_dev *dev, ...@@ -482,7 +482,7 @@ static int au0828_media_device_register(struct au0828_dev *dev,
if (!dev->media_dev) if (!dev->media_dev)
return 0; return 0;
if (!media_devnode_is_registered(&dev->media_dev->devnode)) { if (!media_devnode_is_registered(dev->media_dev->devnode)) {
/* register media device */ /* register media device */
ret = media_device_register(dev->media_dev); ret = media_device_register(dev->media_dev);
......
...@@ -1674,7 +1674,7 @@ static void uvc_delete(struct uvc_device *dev) ...@@ -1674,7 +1674,7 @@ static void uvc_delete(struct uvc_device *dev)
if (dev->vdev.dev) if (dev->vdev.dev)
v4l2_device_unregister(&dev->vdev); v4l2_device_unregister(&dev->vdev);
#ifdef CONFIG_MEDIA_CONTROLLER #ifdef CONFIG_MEDIA_CONTROLLER
if (media_devnode_is_registered(&dev->mdev.devnode)) if (media_devnode_is_registered(dev->mdev.devnode))
media_device_unregister(&dev->mdev); media_device_unregister(&dev->mdev);
media_device_cleanup(&dev->mdev); media_device_cleanup(&dev->mdev);
#endif #endif
......
...@@ -347,7 +347,7 @@ struct media_entity_notify { ...@@ -347,7 +347,7 @@ struct media_entity_notify {
struct media_device { struct media_device {
/* dev->driver_data points to this struct. */ /* dev->driver_data points to this struct. */
struct device *dev; struct device *dev;
struct media_devnode devnode; struct media_devnode *devnode;
char model[32]; char model[32];
char driver_name[32]; char driver_name[32];
...@@ -393,9 +393,6 @@ struct usb_device; ...@@ -393,9 +393,6 @@ struct usb_device;
#define MEDIA_DEV_NOTIFY_PRE_LINK_CH 0 #define MEDIA_DEV_NOTIFY_PRE_LINK_CH 0
#define MEDIA_DEV_NOTIFY_POST_LINK_CH 1 #define MEDIA_DEV_NOTIFY_POST_LINK_CH 1
/* media_devnode to media_device */
#define to_media_device(node) container_of(node, struct media_device, devnode)
/** /**
* media_entity_enum_init - Initialise an entity enumeration * media_entity_enum_init - Initialise an entity enumeration
* *
......
...@@ -33,6 +33,8 @@ ...@@ -33,6 +33,8 @@
#include <linux/device.h> #include <linux/device.h>
#include <linux/cdev.h> #include <linux/cdev.h>
struct media_device;
/* /*
* Flag to mark the media_devnode struct as registered. Drivers must not touch * Flag to mark the media_devnode struct as registered. Drivers must not touch
* this flag directly, it will be set and cleared by media_devnode_register and * this flag directly, it will be set and cleared by media_devnode_register and
...@@ -81,6 +83,8 @@ struct media_file_operations { ...@@ -81,6 +83,8 @@ struct media_file_operations {
* before registering the node. * before registering the node.
*/ */
struct media_devnode { struct media_devnode {
struct media_device *media_dev;
/* device ops */ /* device ops */
const struct media_file_operations *fops; const struct media_file_operations *fops;
...@@ -103,6 +107,7 @@ struct media_devnode { ...@@ -103,6 +107,7 @@ struct media_devnode {
/** /**
* media_devnode_register - register a media device node * media_devnode_register - register a media device node
* *
* @media_dev: struct media_device we want to register a device node
* @devnode: media device node structure we want to register * @devnode: media device node structure we want to register
* @owner: should be filled with %THIS_MODULE * @owner: should be filled with %THIS_MODULE
* *
...@@ -116,7 +121,8 @@ struct media_devnode { ...@@ -116,7 +121,8 @@ struct media_devnode {
* the media_devnode structure is *not* called, so the caller is responsible for * the media_devnode structure is *not* called, so the caller is responsible for
* freeing any data. * freeing any data.
*/ */
int __must_check media_devnode_register(struct media_devnode *devnode, int __must_check media_devnode_register(struct media_device *mdev,
struct media_devnode *devnode,
struct module *owner); struct module *owner);
/** /**
...@@ -146,9 +152,14 @@ static inline struct media_devnode *media_devnode_data(struct file *filp) ...@@ -146,9 +152,14 @@ static inline struct media_devnode *media_devnode_data(struct file *filp)
* false otherwise. * false otherwise.
* *
* @devnode: pointer to struct &media_devnode. * @devnode: pointer to struct &media_devnode.
*
* Note: If mdev is NULL, it also returns false.
*/ */
static inline int media_devnode_is_registered(struct media_devnode *devnode) static inline int media_devnode_is_registered(struct media_devnode *devnode)
{ {
if (!devnode)
return false;
return test_bit(MEDIA_FLAG_REGISTERED, &devnode->flags); return test_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment