From patchwork Wed May 4 11:20:53 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sakari Ailus X-Patchwork-Id: 9012381 Return-Path: X-Original-To: patchwork-linux-media@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 14CF9BF29F for ; Wed, 4 May 2016 11:24:17 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id CE5062038F for ; Wed, 4 May 2016 11:24:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3C9AF2039E for ; Wed, 4 May 2016 11:24:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752366AbcEDLYL (ORCPT ); Wed, 4 May 2016 07:24:11 -0400 Received: from mga03.intel.com ([134.134.136.65]:61674 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752227AbcEDLYI (ORCPT ); Wed, 4 May 2016 07:24:08 -0400 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP; 04 May 2016 04:24:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,576,1455004800"; d="scan'208";a="968450029" Received: from paasikivi.fi.intel.com ([10.237.72.42]) by orsmga002.jf.intel.com with ESMTP; 04 May 2016 04:24:04 -0700 Received: from nauris.fi.intel.com (nauris.localdomain [192.168.240.2]) by paasikivi.fi.intel.com (Postfix) with ESMTP id 45AA020B94; Wed, 4 May 2016 14:24:03 +0300 (EEST) Received: by nauris.fi.intel.com (Postfix, from userid 1000) id ECFF8201F4; Wed, 4 May 2016 14:21:05 +0300 (EEST) From: Sakari Ailus To: linux-media@vger.kernel.org Cc: laurent.pinchart@ideasonboard.com, hverkuil@xs4all.nl, mchehab@osg.samsung.com Subject: [PATCH v2 3/5] media: Refactor copying IOCTL arguments from and to user space Date: Wed, 4 May 2016 14:20:53 +0300 Message-Id: <1462360855-23354-4-git-send-email-sakari.ailus@linux.intel.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1462360855-23354-1-git-send-email-sakari.ailus@linux.intel.com> References: <1462360855-23354-1-git-send-email-sakari.ailus@linux.intel.com> Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Spam-Status: No, score=-9.0 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- drivers/media/media-device.c | 214 ++++++++++++++++++++++--------------------- 1 file changed, 110 insertions(+), 104 deletions(-) diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index 9b5a88d..39fe07f 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -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,50 @@ 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, \ +#ifdef CONFIG_COMPAT +/* Only compat IOCTLs need this right now. */ +static long copy_arg_to_user_nop(void __user *uarg, void *karg, + unsigned int cmd) +{ + return 0; +} +#endif + +#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 inline long is_valid_ioctl(const struct media_ioctl_info *info, @@ -438,13 +416,29 @@ static inline long is_valid_ioctl(const struct media_ioctl_info *info, || info[_IOC_NR(cmd)].cmd != cmd) ? -ENOIOCTLCMD : 0; } +static unsigned int media_ioctl_max_arg_size( + const struct media_ioctl_info *info, unsigned int len, + unsigned int *max_size) +{ + if (*max_size) + return *max_size; + + for (; len; len--, info++) + *max_size = max(_IOC_SIZE(info->cmd), *max_size); + + return *max_size; +} + static long __media_device_ioctl( struct file *filp, unsigned int cmd, void __user *arg, - const struct media_ioctl_info *info_array, unsigned int info_array_len) + const struct media_ioctl_info *info_array, unsigned int info_array_len, + unsigned int *max_arg_size) { struct media_devnode *devnode = media_devnode_data(filp); struct media_device *dev = to_media_device(devnode); const struct media_ioctl_info *info; + char karg[media_ioctl_max_arg_size(info_array, info_array_len, + max_arg_size)]; long ret; ret = is_valid_ioctl(info_array, info_array_len, cmd); @@ -453,11 +447,16 @@ static long __media_device_ioctl( info = &info_array[_IOC_NR(cmd)]; + info->arg_from_user(karg, arg, cmd); + mutex_lock(&dev->graph_mutex); - ret = info->fn(dev, arg); + ret = info->fn(dev, karg); mutex_unlock(&dev->graph_mutex); - return ret; + if (ret) + return ret; + + return info->arg_to_user(arg, karg, cmd); } static const struct media_ioctl_info ioctl_info[] = { @@ -471,9 +470,12 @@ static const struct media_ioctl_info ioctl_info[] = { static long media_device_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { + static unsigned int max_arg_size; + return __media_device_ioctl( filp, cmd, (void __user *)arg, - ioctl_info, ARRAY_SIZE(ioctl_info)); + ioctl_info, ARRAY_SIZE(ioctl_info), + &max_arg_size); } #ifdef CONFIG_COMPAT @@ -485,23 +487,24 @@ struct media_links_enum32 { __u32 reserved[4]; }; -static long media_device_enum_links32(struct media_device *mdev, - struct media_links_enum32 __user *ulinks) +static long from_user_enum_links32(void *karg, void __user *uarg, + unsigned int cmd) { - struct media_links_enum links; + struct media_links_enum *links = karg; + struct media_links_enum32 __user *ulinks = uarg; compat_uptr_t pads_ptr, links_ptr; - memset(&links, 0, sizeof(links)); + memset(links, 0, sizeof(*links)); - if (get_user(links.entity, &ulinks->entity) + if (get_user(links->entity, &ulinks->entity) || get_user(pads_ptr, &ulinks->pads) || get_user(links_ptr, &ulinks->links)) return -EFAULT; - links.pads = compat_ptr(pads_ptr); - links.links = compat_ptr(links_ptr); + links->pads = compat_ptr(pads_ptr); + links->links = compat_ptr(links_ptr); - return __media_device_enum_links(mdev, &links); + return 0; } #define MEDIA_IOC_ENUM_LINKS32 _IOWR('|', 0x02, struct media_links_enum32) @@ -509,7 +512,7 @@ static long media_device_enum_links32(struct media_device *mdev, static const struct media_ioctl_info compat_ioctl_info[] = { MEDIA_IOC(DEVICE_INFO, media_device_get_info), MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities), - MEDIA_IOC(ENUM_LINKS32, media_device_enum_links32), + MEDIA_IOC_ARG(ENUM_LINKS32, media_device_enum_links, from_user_enum_links32, copy_arg_to_user_nop), MEDIA_IOC(SETUP_LINK, media_device_setup_link), MEDIA_IOC(G_TOPOLOGY, media_device_get_topology), }; @@ -517,9 +520,12 @@ static const struct media_ioctl_info compat_ioctl_info[] = { static long media_device_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { + static unsigned int max_arg_size; + return __media_device_ioctl( filp, cmd, (void __user *)arg, - compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info)); + compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info), + &max_arg_size); } #endif /* CONFIG_COMPAT */