| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <925f444d-e5b0-7c5b-b5d0-6b66b8045c2d@gmail.com>
Date: Thu, 16 Feb 2017 11:07:16 -0800
From: Steve Longerbeam <slongerbeam@...il.com>
To: Russell King - ARM Linux <linux@...linux.org.uk>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Hans Verkuil <hverkuil@...all.nl>,
Mauro Carvalho Chehab <mchehab@...hat.com>
Cc: mark.rutland@....com, andrew-ct.chen@...iatek.com,
minghsiu.tsai@...iatek.com, sakari.ailus@...ux.intel.com,
nick@...anahar.org, songjun.wu@...rochip.com, pavel@....cz,
robert.jarzmik@...e.fr, devel@...verdev.osuosl.org,
markus.heiser@...marIT.de,
Steve Longerbeam <steve_longerbeam@...tor.com>,
shuah@...nel.org, geert@...ux-m68k.org,
linux-media@...r.kernel.org, devicetree@...r.kernel.org,
kernel@...gutronix.de, arnd@...db.de, tiffany.lin@...iatek.com,
bparrot@...com, robh+dt@...nel.org, horms+renesas@...ge.net.au,
mchehab@...nel.org, laurent.pinchart+renesas@...asonboard.com,
linux-arm-kernel@...ts.infradead.org,
niklas.soderlund+renesas@...natech.se, gregkh@...uxfoundation.org,
linux-kernel@...r.kernel.org, jean-christophe.trotin@...com,
p.zabel@...gutronix.de, fabio.estevam@....com, shawnguo@...nel.org,
sudipm.mukherjee@...il.com
Subject: Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver
On 02/16/2017 06:20 AM, Russell King - ARM Linux wrote:
> On Thu, Feb 16, 2017 at 01:09:35PM +0000, Russell King - ARM Linux wrote:
>>
>> <snip>
>> More crap.
>>
>> If the "complete" method fails (or, in fact, anything in
>> v4l2_async_test_notify() fails) then all hell breaks loose, because
>> of the total lack of clean up (and no, this isn't anything to do with
>> some stupid justification of those BUG_ON()s above.)
>>
>> v4l2_async_notifier_register() gets called, it adds the notifier to
>> the global notifier list. v4l2_async_test_notify() gets called. It
>> returns an error, which is propagated out of
>> v4l2_async_notifier_register().
>>
>> So the caller thinks that v4l2_async_notifier_register() failed, which
>> will cause imx_media_probe() to fail, causing imxmd->subdev_notifier
>> to be kfree()'d. We now have a use-after free bug.
>>
>> Second case. v4l2_async_register_subdev(). Almost exactly the same,
>> except in this case adding sd->async_list to the notifier->done list
>> may have succeeded, and failure after that, again, results in an
>> in-use list_head being kfree()'d.
>
> And here's a patch which, combined with the fixes for ipuv3, results in
> everything appearing to work properly. Feel free to tear out the bits
> for your area and turn them into proper patches.
>
> drivers/gpu/ipu-v3/ipu-common.c | 6 ---
> drivers/media/media-entity.c | 7 +--
> drivers/media/v4l2-core/v4l2-async.c | 71 +++++++++++++++++++++++--------
> drivers/staging/media/imx/imx-media-csi.c | 1 +
> drivers/staging/media/imx/imx-media-dev.c | 2 +-
> 5 files changed, 59 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c
> index 97218af4fe75..8368e6f766ee 100644
> --- a/drivers/gpu/ipu-v3/ipu-common.c
> +++ b/drivers/gpu/ipu-v3/ipu-common.c
> @@ -1238,12 +1238,6 @@ static int ipu_add_client_devices(struct ipu_soc *ipu, unsigned long ipu_base)
> platform_device_put(pdev);
> goto err_register;
> }
> -
> - /*
> - * Set of_node only after calling platform_device_add. Otherwise
> - * the platform:imx-ipuv3-crtc modalias won't be used.
> - */
> - pdev->dev.of_node = of_node;
> }
Ah, never mind my question earlier, I see now why the CSI's were likely
not recognized, probably because of this. Anyway I agree with this
change and I made the accompanying requisite change to imx-media-csi.c
and imx-media-dev.c below.
Steve
>
> return 0;
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index f9f723f5e4f0..154593a168df 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -625,9 +625,10 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
> struct media_link *link;
> struct media_link *backlink;
>
> - BUG_ON(source == NULL || sink == NULL);
> - BUG_ON(source_pad >= source->num_pads);
> - BUG_ON(sink_pad >= sink->num_pads);
> + if (WARN_ON(source == NULL || sink == NULL) ||
> + WARN_ON(source_pad >= source->num_pads) ||
> + WARN_ON(sink_pad >= sink->num_pads))
> + return -EINVAL;
>
> link = media_add_link(&source->links);
> if (link == NULL)
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 5bada202b2d3..09934fb96a8d 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -94,7 +94,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
> }
>
> static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> - struct v4l2_subdev *sd,
> + struct list_head *new, struct v4l2_subdev *sd,
> struct v4l2_async_subdev *asd)
> {
> int ret;
> @@ -107,22 +107,36 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> if (notifier->bound) {
> ret = notifier->bound(notifier, sd, asd);
> if (ret < 0)
> - return ret;
> + goto err_bind;
> }
> +
> /* Move from the global subdevice list to notifier's done */
> - list_move(&sd->async_list, ¬ifier->done);
> + list_move(&sd->async_list, new);
>
> ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> - if (ret < 0) {
> - if (notifier->unbind)
> - notifier->unbind(notifier, sd, asd);
> - return ret;
> - }
> + if (ret < 0)
> + goto err_register;
>
> - if (list_empty(¬ifier->waiting) && notifier->complete)
> - return notifier->complete(notifier);
> + if (list_empty(¬ifier->waiting) && notifier->complete) {
> + ret = notifier->complete(notifier);
> + if (ret < 0)
> + goto err_complete;
> + }
>
> return 0;
> +
> +err_complete:
> + v4l2_device_unregister_subdev(sd);
> +err_register:
> + if (notifier->unbind)
> + notifier->unbind(notifier, sd, asd);
> +err_bind:
> + sd->notifier = NULL;
> + sd->asd = NULL;
> + list_add(&asd->list, ¬ifier->waiting);
> + /* always take this off the list on error */
> + list_del(&sd->async_list);
> + return ret;
> }
>
> static void v4l2_async_cleanup(struct v4l2_subdev *sd)
> @@ -139,7 +153,8 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> {
> struct v4l2_subdev *sd, *tmp;
> struct v4l2_async_subdev *asd;
> - int i;
> + LIST_HEAD(new);
> + int ret, i;
>
> if (!notifier->num_subdevs || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> return -EINVAL;
> @@ -172,22 +187,39 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> list_add(¬ifier->list, ¬ifier_list);
>
> list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> - int ret;
> -
> asd = v4l2_async_belongs(notifier, sd);
> if (!asd)
> continue;
>
> - ret = v4l2_async_test_notify(notifier, sd, asd);
> + ret = v4l2_async_test_notify(notifier, &new, sd, asd);
> if (ret < 0) {
> - mutex_unlock(&list_lock);
> - return ret;
> + /*
> + * On failure, v4l2_async_test_notify() takes the
> + * sd off the subdev list. Add it back.
> + */
> + list_add(&sd->async_list, &subdev_list);
> + goto err_notify;
> }
> }
>
> + list_splice(&new, ¬ifier->done);
> +
> mutex_unlock(&list_lock);
>
> return 0;
> +
> +err_notify:
> + list_del(¬ifier->list);
> + list_for_each_entry_safe(sd, tmp, &new, async_list) {
> + v4l2_device_unregister_subdev(sd);
> + list_move(&sd->async_list, &subdev_list);
> + if (notifier->unbind)
> + notifier->unbind(notifier, sd, sd->asd);
> + sd->notifier = NULL;
> + sd->asd = NULL;
> + }
> + mutex_unlock(&list_lock);
> + return ret;
> }
> EXPORT_SYMBOL(v4l2_async_notifier_register);
>
> @@ -213,6 +245,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> list_del(¬ifier->list);
>
> list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {
> + struct v4l2_async_subdev *asd = sd->asd;
> struct device *d;
>
> d = get_device(sd->dev);
> @@ -223,7 +256,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> device_release_driver(d);
>
> if (notifier->unbind)
> - notifier->unbind(notifier, sd, sd->asd);
> + notifier->unbind(notifier, sd, asd);
>
> /*
> * Store device at the device cache, in order to call
> @@ -288,7 +321,9 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> list_for_each_entry(notifier, ¬ifier_list, list) {
> struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, sd);
> if (asd) {
> - int ret = v4l2_async_test_notify(notifier, sd, asd);
> + int ret = v4l2_async_test_notify(notifier,
> + ¬ifier->done,
> + sd, asd);
> mutex_unlock(&list_lock);
> return ret;
> }
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 9d9ec03436e4..507026feee91 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1427,6 +1427,7 @@ static int imx_csi_probe(struct platform_device *pdev)
> priv->sd.entity.ops = &csi_entity_ops;
> priv->sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> priv->sd.dev = &pdev->dev;
> + priv->sd.of_node = pdata->of_node;
> priv->sd.owner = THIS_MODULE;
> priv->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> priv->sd.grp_id = priv->csi_id ?
> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
> index 60f45fe4b506..5b4dfc1fb6ab 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -197,7 +197,7 @@ static int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
> struct imx_media_subdev *imxsd;
> int ret = -EINVAL;
>
> - imxsd = imx_media_find_async_subdev(imxmd, sd->dev->of_node,
> + imxsd = imx_media_find_async_subdev(imxmd, sd->of_node,
> dev_name(sd->dev));
> if (!imxsd)
> goto out;
>
>
Powered by blists - more mailing lists