lkml.org 
[lkml]   [2020]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.16 242/245] [media] media-device: dynamically allocate struct media_devnode
    3.16.83-rc1 review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

    commit a087ce704b802becbb4b0f2a20f2cb3f6911802e upstream.

    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 <mchehab@osg.samsung.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
    [bwh: Backported to 3.16:
    - Drop change in au0828
    - Include <linux/slab.h> in media-device.c
    - Adjust context]
    Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
    ---
    --- a/drivers/media/media-device.c
    +++ b/drivers/media/media-device.c
    @@ -24,6 +24,7 @@
    #include <linux/export.h>
    #include <linux/ioctl.h>
    #include <linux/media.h>
    +#include <linux/slab.h>
    #include <linux/types.h>

    #include <media/media-device.h>
    @@ -236,7 +237,7 @@ static long media_device_ioctl(struct fi
    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) {
    @@ -305,7 +306,7 @@ static long media_device_compat_ioctl(st
    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) {
    @@ -346,7 +347,8 @@ static const struct media_file_operation
    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);
    }
    @@ -374,6 +376,7 @@ static void media_device_release(struct
    int __must_check __media_device_register(struct media_device *mdev,
    struct module *owner)
    {
    + struct media_devnode *devnode;
    int ret;

    if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0))
    @@ -384,17 +387,27 @@ int __must_check __media_device_register
    spin_lock_init(&mdev->lock);
    mutex_init(&mdev->graph_mutex);

    + 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;
    - ret = media_devnode_register(&mdev->devnode, owner);
    - if (ret < 0)
    + mdev->devnode = devnode;
    + devnode->fops = &media_device_fops;
    + devnode->parent = mdev->dev;
    + devnode->release = media_device_release;
    + 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;
    }

    @@ -415,8 +428,11 @@ void media_device_unregister(struct medi
    list_for_each_entry_safe(entity, next, &mdev->entities, list)
    media_device_unregister_entity(entity);

    - device_remove_file(&mdev->devnode.dev, &dev_attr_model);
    - 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);

    --- a/drivers/media/media-devnode.c
    +++ b/drivers/media/media-devnode.c
    @@ -44,6 +44,7 @@
    #include <linux/uaccess.h>

    #include <media/media-devnode.h>
    +#include <media/media-device.h>

    #define MEDIA_NUM_DEVICES 256
    #define MEDIA_NAME "media"
    @@ -74,6 +75,8 @@ static void media_devnode_release(struct
    /* Release media_devnode and perform other cleanups as needed. */
    if (devnode->release)
    devnode->release(devnode);
    +
    + kfree(devnode);
    }

    static struct bus_type media_bus_type = {
    @@ -221,6 +224,7 @@ static const struct file_operations medi

    /**
    * 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
    *
    * The registration code assigns minor numbers and registers the new device node
    @@ -233,7 +237,8 @@ static const struct file_operations medi
    * the media_devnode structure is *not* called, so the caller is responsible for
    * 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)
    {
    int minor;
    @@ -252,6 +257,7 @@ int __must_check media_devnode_register(
    mutex_unlock(&media_devnode_lock);

    devnode->minor = minor;
    + devnode->media_dev = mdev;

    /* Part 2: Initialize and register the character device */
    cdev_init(&devnode->cdev, &media_devnode_fops);
    --- a/include/media/media-device.h
    +++ b/include/media/media-device.h
    @@ -60,7 +60,7 @@ struct device;
    struct media_device {
    /* dev->driver_data points to this struct. */
    struct device *dev;
    - struct media_devnode devnode;
    + struct media_devnode *devnode;

    char model[32];
    char serial[40];
    @@ -84,9 +84,6 @@ struct media_device {
    #define MEDIA_DEV_NOTIFY_PRE_LINK_CH 0
    #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)
    -
    int __must_check __media_device_register(struct media_device *mdev,
    struct module *owner);
    #define media_device_register(mdev) __media_device_register(mdev, THIS_MODULE)
    --- a/drivers/media/usb/uvc/uvc_driver.c
    +++ b/drivers/media/usb/uvc/uvc_driver.c
    @@ -1640,7 +1640,7 @@ static void uvc_delete(struct uvc_device
    if (dev->vdev.dev)
    v4l2_device_unregister(&dev->vdev);
    #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);
    #endif

    --- a/include/media/media-devnode.h
    +++ b/include/media/media-devnode.h
    @@ -33,6 +33,8 @@
    #include <linux/device.h>
    #include <linux/cdev.h>

    +struct media_device;
    +
    /*
    * 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
    @@ -63,6 +65,8 @@ struct media_file_operations {
    * before registering the node.
    */
    struct media_devnode {
    + struct media_device *media_dev;
    +
    /* device ops */
    const struct media_file_operations *fops;

    @@ -82,7 +86,8 @@ struct media_devnode {
    /* dev to media_devnode */
    #define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)

    -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);
    void media_devnode_unregister(struct media_devnode *devnode);

    @@ -93,6 +98,9 @@ static inline struct media_devnode *medi

    static inline int media_devnode_is_registered(struct media_devnode *devnode)
    {
    + if (!devnode)
    + return false;
    +
    return test_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
    }

    \
     
     \ /
      Last update: 2020-04-24 01:10    [W:2.343 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site