[media] media: Refactor copying IOCTL arguments from and to user space
authorSakari Ailus <sakari.ailus@linux.intel.com>
Tue, 3 May 2016 21:42:13 +0000 (18:42 -0300)
committerMauro Carvalho Chehab <mchehab@s-opensource.com>
Thu, 22 Sep 2016 13:25:55 +0000 (10:25 -0300)
Refactor copying the IOCTL argument structs from the user space and back,
in order to reduce code copied around and make the implementation more
robust.

As a result, the copying is done while not holding the graph mutex.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
drivers/media/media-device.c

index 4ac912b60f8518273e313d8dd35814bb73ff59f4..c44406ed1bb1f596ca50fde49178cbb095e5a00f 100644 (file)
@@ -59,27 +59,24 @@ static int media_device_close(struct file *filp)
 }
 
 static int media_device_get_info(struct media_device *dev,
-                                struct media_device_info __user *__info)
+                                struct media_device_info *info)
 {
-       struct media_device_info info;
-
-       memset(&info, 0, sizeof(info));
+       memset(info, 0, sizeof(*info));
 
        if (dev->driver_name[0])
-               strlcpy(info.driver, dev->driver_name, sizeof(info.driver));
+               strlcpy(info->driver, dev->driver_name, sizeof(info->driver));
        else
-               strlcpy(info.driver, dev->dev->driver->name, sizeof(info.driver));
+               strlcpy(info->driver, dev->dev->driver->name,
+                       sizeof(info->driver));
 
-       strlcpy(info.model, dev->model, sizeof(info.model));
-       strlcpy(info.serial, dev->serial, sizeof(info.serial));
-       strlcpy(info.bus_info, dev->bus_info, sizeof(info.bus_info));
+       strlcpy(info->model, dev->model, sizeof(info->model));
+       strlcpy(info->serial, dev->serial, sizeof(info->serial));
+       strlcpy(info->bus_info, dev->bus_info, sizeof(info->bus_info));
 
-       info.media_version = MEDIA_API_VERSION;
-       info.hw_revision = dev->hw_revision;
-       info.driver_version = dev->driver_version;
+       info->media_version = MEDIA_API_VERSION;
+       info->hw_revision = dev->hw_revision;
+       info->driver_version = dev->driver_version;
 
-       if (copy_to_user(__info, &info, sizeof(*__info)))
-               return -EFAULT;
        return 0;
 }
 
@@ -101,29 +98,25 @@ static struct media_entity *find_entity(struct media_device *mdev, u32 id)
 }
 
 static long media_device_enum_entities(struct media_device *mdev,
-                                      struct media_entity_desc __user *uent)
+                                      struct media_entity_desc *entd)
 {
        struct media_entity *ent;
-       struct media_entity_desc u_ent;
-
-       memset(&u_ent, 0, sizeof(u_ent));
-       if (copy_from_user(&u_ent.id, &uent->id, sizeof(u_ent.id)))
-               return -EFAULT;
-
-       ent = find_entity(mdev, u_ent.id);
 
+       ent = find_entity(mdev, entd->id);
        if (ent == NULL)
                return -EINVAL;
 
-       u_ent.id = media_entity_id(ent);
+       memset(entd, 0, sizeof(*entd));
+
+       entd->id = media_entity_id(ent);
        if (ent->name)
-               strlcpy(u_ent.name, ent->name, sizeof(u_ent.name));
-       u_ent.type = ent->function;
-       u_ent.revision = 0;             /* Unused */
-       u_ent.flags = ent->flags;
-       u_ent.group_id = 0;             /* Unused */
-       u_ent.pads = ent->num_pads;
-       u_ent.links = ent->num_links - ent->num_backlinks;
+               strlcpy(entd->name, ent->name, sizeof(entd->name));
+       entd->type = ent->function;
+       entd->revision = 0;             /* Unused */
+       entd->flags = ent->flags;
+       entd->group_id = 0;             /* Unused */
+       entd->pads = ent->num_pads;
+       entd->links = ent->num_links - ent->num_backlinks;
 
        /*
         * Workaround for a bug at media-ctl <= v1.10 that makes it to
@@ -139,14 +132,13 @@ static long media_device_enum_entities(struct media_device *mdev,
        if (ent->function < MEDIA_ENT_F_OLD_BASE ||
            ent->function > MEDIA_ENT_T_DEVNODE_UNKNOWN) {
                if (is_media_entity_v4l2_subdev(ent))
-                       u_ent.type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
+                       entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
                else if (ent->function != MEDIA_ENT_F_IO_V4L)
-                       u_ent.type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
+                       entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
        }
 
-       memcpy(&u_ent.raw, &ent->info, sizeof(ent->info));
-       if (copy_to_user(uent, &u_ent, sizeof(u_ent)))
-               return -EFAULT;
+       memcpy(&entd->raw, &ent->info, sizeof(ent->info));
+
        return 0;
 }
 
@@ -158,8 +150,8 @@ static void media_device_kpad_to_upad(const struct media_pad *kpad,
        upad->flags = kpad->flags;
 }
 
-static long __media_device_enum_links(struct media_device *mdev,
-                                     struct media_links_enum *links)
+static long media_device_enum_links(struct media_device *mdev,
+                                   struct media_links_enum *links)
 {
        struct media_entity *entity;
 
@@ -206,64 +198,35 @@ static long __media_device_enum_links(struct media_device *mdev,
        return 0;
 }
 
-static long media_device_enum_links(struct media_device *mdev,
-                                   struct media_links_enum __user *ulinks)
-{
-       struct media_links_enum links;
-       int rval;
-
-       if (copy_from_user(&links, ulinks, sizeof(links)))
-               return -EFAULT;
-
-       rval = __media_device_enum_links(mdev, &links);
-       if (rval < 0)
-               return rval;
-
-       if (copy_to_user(ulinks, &links, sizeof(*ulinks)))
-               return -EFAULT;
-
-       return 0;
-}
-
 static long media_device_setup_link(struct media_device *mdev,
-                                   struct media_link_desc __user *_ulink)
+                                   struct media_link_desc *linkd)
 {
        struct media_link *link = NULL;
-       struct media_link_desc ulink;
        struct media_entity *source;
        struct media_entity *sink;
-       int ret;
-
-       if (copy_from_user(&ulink, _ulink, sizeof(ulink)))
-               return -EFAULT;
 
        /* Find the source and sink entities and link.
         */
-       source = find_entity(mdev, ulink.source.entity);
-       sink = find_entity(mdev, ulink.sink.entity);
+       source = find_entity(mdev, linkd->source.entity);
+       sink = find_entity(mdev, linkd->sink.entity);
 
        if (source == NULL || sink == NULL)
                return -EINVAL;
 
-       if (ulink.source.index >= source->num_pads ||
-           ulink.sink.index >= sink->num_pads)
+       if (linkd->source.index >= source->num_pads ||
+           linkd->sink.index >= sink->num_pads)
                return -EINVAL;
 
-       link = media_entity_find_link(&source->pads[ulink.source.index],
-                                     &sink->pads[ulink.sink.index]);
+       link = media_entity_find_link(&source->pads[linkd->source.index],
+                                     &sink->pads[linkd->sink.index]);
        if (link == NULL)
                return -EINVAL;
 
        /* Setup the link on both entities. */
-       ret = __media_entity_setup_link(link, ulink.flags);
-
-       if (copy_to_user(_ulink, &ulink, sizeof(ulink)))
-               return -EFAULT;
-
-       return ret;
+       return __media_entity_setup_link(link, linkd->flags);
 }
 
-static long __media_device_get_topology(struct media_device *mdev,
+static long media_device_get_topology(struct media_device *mdev,
                                      struct media_v2_topology *topo)
 {
        struct media_entity *entity;
@@ -400,35 +363,41 @@ static long __media_device_get_topology(struct media_device *mdev,
        return ret;
 }
 
-static long media_device_get_topology(struct media_device *mdev,
-                                     struct media_v2_topology __user *utopo)
+static long copy_arg_from_user(void *karg, void __user *uarg, unsigned int cmd)
 {
-       struct media_v2_topology ktopo;
-       int ret;
-
-       if (copy_from_user(&ktopo, utopo, sizeof(ktopo)))
+       /* All media IOCTLs are _IOWR() */
+       if (copy_from_user(karg, uarg, _IOC_SIZE(cmd)))
                return -EFAULT;
 
-       ret = __media_device_get_topology(mdev, &ktopo);
-       if (ret < 0)
-               return ret;
+       return 0;
+}
 
-       if (copy_to_user(utopo, &ktopo, sizeof(*utopo)))
+static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
+{
+       /* All media IOCTLs are _IOWR() */
+       if (copy_to_user(uarg, karg, _IOC_SIZE(cmd)))
                return -EFAULT;
 
        return 0;
 }
 
-#define MEDIA_IOC(__cmd, func)                                         \
-       [_IOC_NR(MEDIA_IOC_##__cmd)] = {                                \
-               .cmd = MEDIA_IOC_##__cmd,                               \
-               .fn = (long (*)(struct media_device *, void __user *))func,    \
+#define MEDIA_IOC_ARG(__cmd, func, from_user, to_user) \
+       [_IOC_NR(MEDIA_IOC_##__cmd)] = {                \
+               .cmd = MEDIA_IOC_##__cmd,               \
+               .fn = (long (*)(struct media_device *, void *))func,    \
+               .arg_from_user = from_user,             \
+               .arg_to_user = to_user,                 \
        }
 
+#define MEDIA_IOC(__cmd, func)                                         \
+       MEDIA_IOC_ARG(__cmd, func, copy_arg_from_user, copy_arg_to_user)
+
 /* the table is indexed by _IOC_NR(cmd) */
 struct media_ioctl_info {
        unsigned int cmd;
-       long (*fn)(struct media_device *dev, void __user *arg);
+       long (*fn)(struct media_device *dev, void *arg);
+       long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
+       long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
 };
 
 static const struct media_ioctl_info ioctl_info[] = {
@@ -440,11 +409,13 @@ static const struct media_ioctl_info ioctl_info[] = {
 };
 
 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_device *dev = devnode->media_dev;
        const struct media_ioctl_info *info;
+       void __user *arg = (void __user *)__arg;
+       char __karg[256], *karg = __karg;
        long ret;
 
        if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info)
@@ -453,10 +424,29 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
 
        info = &ioctl_info[_IOC_NR(cmd)];
 
+       if (_IOC_SIZE(info->cmd) > sizeof(__karg)) {
+               karg = kmalloc(_IOC_SIZE(info->cmd), GFP_KERNEL);
+               if (!karg)
+                       return -ENOMEM;
+       }
+
+       if (info->arg_from_user) {
+               ret = info->arg_from_user(karg, arg, cmd);
+               if (ret)
+                       goto out_free;
+       }
+
        mutex_lock(&dev->graph_mutex);
-       ret = info->fn(dev, (void __user *)arg);
+       ret = info->fn(dev, karg);
        mutex_unlock(&dev->graph_mutex);
 
+       if (!ret && info->arg_to_user)
+               ret = info->arg_to_user(arg, karg, cmd);
+
+out_free:
+       if (karg != __karg)
+               kfree(karg);
+
        return ret;
 }
 
@@ -485,7 +475,7 @@ static long media_device_enum_links32(struct media_device *mdev,
        links.pads = compat_ptr(pads_ptr);
        links.links = compat_ptr(links_ptr);
 
-       return __media_device_enum_links(mdev, &links);
+       return media_device_enum_links(mdev, &links);
 }
 
 #define MEDIA_IOC_ENUM_LINKS32         _IOWR('|', 0x02, struct media_links_enum32)