Re: [PATCH] media: nxp: imx8-isi: Fix potential pipe leaks on removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] media: nxp: imx8-isi: NULL deref on
  media_device_register_entity() error
It also noticed 2 related pre-existing issues this patch does not
introduce:
- [Medium] media: nxp: imx8-isi: stateless device link leak in
  mxc_isi_async_notifier_bound() [1]
- [Critical] media: nxp: imx8-isi: unchecked dma_set_mask_and_coherent()
  in mxc_isi_probe() [2]
--

commit 1c93ad5a0d826cfb633314a1db34b0975979e67c
Author: Guangshuo Li <liguangshuo@mail.sdu.edu.cn>

media: nxp: imx8-isi: Fix potential pipe leaks on removal

This commit refcounts mxc_isi_dev so that pipe and V4L2 state stays alive
until both device removal and the last V4L2 reference complete, while
disconnecting capture and M2M queues during unregistration.

> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> index 0ae5f35e017c6..bd046a0751b9f 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c

[ ... ]

> @@ -211,33 +256,25 @@ static int mxc_isi_v4l2_init(struct mxc_isi_dev *isi)
>  	return 0;
>  
>  err_m2m:
> -	mxc_isi_m2m_unregister(isi);
>  	v4l2_async_nf_cleanup(&isi->notifier);
>  err_v4l2:
> +	mxc_isi_video_unregister_all(isi);
>  	v4l2_device_unregister(v4l2_dev);
> +	media_device_unregister(media_dev);
> +	v4l2_device_put(v4l2_dev);
>  err_media:
> -	media_device_cleanup(media_dev);
>  	return ret;
>  }
>  
>  static void mxc_isi_v4l2_cleanup(struct mxc_isi_dev *isi)
>  {
> -	unsigned int i;
> +	mxc_isi_video_unregister_all(isi);
>  
>  	v4l2_async_nf_unregister(&isi->notifier);
>  	v4l2_async_nf_cleanup(&isi->notifier);
> -
>  	v4l2_device_unregister(&isi->v4l2_dev);
>  	media_device_unregister(&isi->media_dev);
> -
> -	mxc_isi_m2m_unregister(isi);
> -
> -	for (i = 0; i < isi->pdata->num_channels; ++i)
> -		mxc_isi_pipe_unregister(&isi->pipes[i]);
> -
> -	mxc_isi_crossbar_unregister(&isi->crossbar);
> -
> -	media_device_cleanup(&isi->media_dev);
> +	v4l2_device_put(&isi->v4l2_dev);
>  }

[Severity: Medium]
This problem wasn't introduced by this patch, but does the driver leak the
stateless device link created in mxc_isi_async_notifier_bound() [1]?

drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c:mxc_isi_async_notifier_bound() {
	...
	link = device_link_add(isi->dev, sd->dev, DL_FLAG_STATELESS);
	if (!link) {
		dev_err(isi->dev,
			"Failed to create device link to source %s\n", sd->name);
		return -EINVAL;
	}

	return v4l2_create_fwnode_links_to_pad(sd, pad, link_flags);
}

Because DL_FLAG_STATELESS is used, the driver core does not remove the link
automatically.  If v4l2_create_fwnode_links_to_pad() fails, or when a
subdevice unbinds or the ISI driver is removed (since
mxc_isi_async_notifier_ops has no .unbind callback and
mxc_isi_v4l2_cleanup() does not delete the link), the device_link and its
device references are leaked, and repeated subdevice unbind/rebind cycles
increment the link's refcount indefinitely.

> @@ -479,11 +516,15 @@ static int mxc_isi_probe(struct platform_device *pdev)
>  	unsigned int i;
>  	int ret = 0;
>  
> -	isi = devm_kzalloc(dev, sizeof(*isi), GFP_KERNEL);
> +	isi = kzalloc_obj(*isi);
>  	if (!isi)
>  		return -ENOMEM;
>  
> -	isi->dev = dev;
> +	isi->dev = get_device(dev);
> +	kref_init(&isi->ref);
> +	ret = devm_add_action_or_reset(dev, mxc_isi_put, isi);
> +	if (ret)
> +		return ret;
>  	platform_set_drvdata(pdev, isi);
>  
>  	isi->pdata = of_device_get_match_data(dev);
> @@ -521,48 +562,35 @@ static int mxc_isi_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, ret,
>  				     "Failed to initialize crossbar\n");

[Severity: Critical]
This problem wasn't introduced by this patch, but should mxc_isi_probe()
check the return value of dma_set_mask_and_coherent() [2]?

drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c:mxc_isi_probe() {
	...
	dma_size = isi->pdata->has_36bit_dma ? 36 : 32;
	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(dma_size));

	ret = devm_pm_runtime_enable(dev);
	...
}

If setting the 32-bit or 36-bit DMA mask fails, probe continues with an
invalid DMA configuration, which can truncate physical addresses during DMA
transfers and cause memory corruption or IOMMU faults.

[ ... ]

> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> index b729eecad0fef..5349e09e9b57a 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c

[ ... ]

> @@ -790,11 +801,11 @@ int mxc_isi_m2m_register(struct mxc_isi_dev *isi, struct v4l2_device *v4l2_dev)
>  	vdev->entity.function = MEDIA_ENT_F_IO_V4L;
>  	ret = media_entity_pads_init(&vdev->entity, 1, &m2m->pad);
>  	if (ret)
> -		goto err_video;
> +		goto err_unregister;
>  
>  	ret = media_device_register_entity(v4l2_dev->mdev, &vdev->entity);
>  	if (ret)
> -		goto err_entity_cleanup;
> +		goto err_unregister;

[ ... ]

>  err_unregister:
> +	mxc_isi_m2m_unregister(isi);
> +err_return:
>  	return ret;
>  }
>  
> @@ -849,13 +851,43 @@ int mxc_isi_m2m_unregister(struct mxc_isi_dev *isi)
>  {
>  	struct mxc_isi_m2m *m2m = &isi->m2m;
>  	struct video_device *vdev = &m2m->vdev;
> +	struct mxc_isi_m2m_ctx *ctx;
>  
> +	if (!video_is_registered(vdev))
> +		return 0;
> +
> +	guard(mutex)(&m2m->contexts_lock);
>  	video_unregister_device(vdev);
> +	v4l2_m2m_suspend(m2m->m2m_dev);
> +	list_for_each_entry(ctx, &m2m->contexts, list) {
> +		guard(mutex)(&ctx->vb2_lock);
> +		vb2_queue_error(v4l2_m2m_get_src_vq(ctx->fh.m2m_ctx));
> +		vb2_queue_error(v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx));
> +		v4l2_m2m_streamoff(NULL, ctx->fh.m2m_ctx,
> +				   V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
> +		v4l2_m2m_streamoff(NULL, ctx->fh.m2m_ctx,
> +				   V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> +	}
>  
> -	v4l2_m2m_release(m2m->m2m_dev);
> -	media_devnode_remove(m2m->intf);
> -	media_entity_cleanup(&vdev->entity);
> -	mutex_destroy(&m2m->lock);
> +	if (m2m->intf) {
> +		media_devnode_remove(m2m->intf);
> +		m2m->intf = NULL;
> +	}
> +	media_device_unregister_entity(&vdev->entity);

[Severity: Medium]
Can jumping to err_unregister when media_device_register_entity() fails
cause a NULL pointer dereference in media_device_unregister_entity()?

drivers/media/mc/mc-device.c:media_device_register_entity() {
	...
	entity->graph_obj.mdev = mdev;
	INIT_LIST_HEAD(&entity->links);
	entity->num_links = 0;
	entity->num_backlinks = 0;

	ret = ida_alloc_min(&mdev->entity_internal_idx, 1, GFP_KERNEL);
	if (ret < 0)
		return ret;
	...
}

If ida_alloc_min() fails inside media_device_register_entity(),
entity->graph_obj.mdev is already set, while entity->internal_idx is still 0
and media_gobj_create() has not initialized entity->graph_obj.list.
Calling mxc_isi_m2m_unregister() -> media_device_unregister_entity() on this
partially initialized entity then calls ida_free() on index 0 and
list_del(&gobj->list) on a zeroed list_head, dereferencing NULL.

[1] https://sashiko.dev/#/bug/linux-feae4a20-e500-4215-90d3-b1654008ecd5
[2] https://sashiko.dev/#/bug/linux-939c7345-a73b-4e71-844e-5708e3fa78c4

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-0d0c29e10a-media-imx8-isi-1@mail.sdu.edu.cn?part=1