Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver

[Steve Longerbeam <slongerbeam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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, &notifier->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(&notifier->waiting) && notifier->complete)
> -		return notifier->complete(notifier);
> +	if (list_empty(&notifier->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, &notifier->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(&notifier->list, &notifier_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, &notifier->done);
> +
>  	mutex_unlock(&list_lock);
>
>  	return 0;
> +
> +err_notify:
> +	list_del(&notifier->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(&notifier->list);
>
>  	list_for_each_entry_safe(sd, tmp, &notifier->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, &notifier_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,
> +							 &notifier->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;
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html