Re: [PATCH v4 1/7] media: qcom: camss: Add camss-pipeline helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Synchronous freeing of pipeline structure causes use-after-free for embedded V4L2 objects.
- [High] Missing file operations (`vdev.fops` = NULL) causes an immediate NULL pointer dereference on device registration.
- [High] Video devices are exposed to userspace before their internal state and media controller links are fully initialized.
- [High] Inverted subdevice cleanup order in teardown exposes freed state to concurrent userspace ioctls.
- [High] Providing NULL subdev operations triggers a kernel BUG_ON, directly violating the API contract.
- [High] If base entity registration fails, the error teardown path mistakenly unregisters it, causing a crash.
- [Medium] `v4l2_subdev_init_finalize()` is called after `v4l2_device_register_subdev()`, exposing uninitialized state.
- [Medium] Missing validation for `desc->name` causes a crash if NULL, or a silent resource leak if empty.
- [Medium] Link validation silently allows misconfigured one-sided links to be dropped.
--

--- Patch [1]: [PATCH v4 1/7] media: qcom: camss: Add camss-pipeline helper ---
commit 1a345e2180f40422d57cb436157b91e6ed4a268d
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

media: qcom: camss: Add camss-pipeline helper

Add a declarative MC topology builder for CAMSS offline ISP drivers.
Drivers describe their entire media graph, entities (video devices,
subdevs, or base entities), their pads, and the links between them
in a static descriptor table. The builder validates the table,
allocates and registers all entities, and creates all MC pad links.

> diff --git a/drivers/media/platform/qcom/camss/camss-pipeline.h b/drivers/media/platform/qcom/camss/camss-pipeline.h
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-pipeline.h

[ ... ]

> +struct camss_isp_entity_desc {
> +	const char				*name;
> +	u32					obj_type;
> +	u32					function;
> +	const struct camss_isp_pad_desc		*pads;
> +
> +	union {
> +		/* MEDIA_ENTITY_TYPE_VIDEO_DEVICE */
> +		struct {
> +			u32					caps;
> +			void					*drvdata;
> +			const struct v4l2_file_operations	*fops;

[Severity: High]
The documentation for this struct mentions that fops can be NULL to use
kernel defaults, but won't that cause a crash during registration?

If fops is NULL, video_register_device() will directly dereference it to
grab the module owner.

> +			const struct v4l2_ioctl_ops		*ioctl_ops;
> +			const struct media_entity_operations	*entity_ops;
> +		} vdev;
> +		/* MEDIA_ENTITY_TYPE_V4L2_SUBDEV */
> +		struct {
> +			const struct v4l2_subdev_ops		*ops;

[Severity: High]
Similarly, the documentation says ops can be NULL, but won't that trigger a
BUG_ON?

If ops is NULL, v4l2_subdev_init() will immediately hit BUG_ON(!ops).
Should these fields be made mandatory in the documentation?

> +			const struct v4l2_subdev_internal_ops	*internal_ops;
> +			const struct media_entity_operations	*entity_ops;
> +		} subdev;
> +	};
> +};

[ ... ]

> diff --git a/drivers/media/platform/qcom/camss/camss-pipeline.c b/drivers/media/platform/qcom/camss/camss-pipeline.c
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-pipeline.c

[ ... ]

> +static int isp_pipeline_validate(struct device *dev,
> +				 const struct camss_isp_entity_desc *descs,
> +				 unsigned int num_entities)
> +{
> +	unsigned int i, pi;
> +
> +	for (i = 0; i < num_entities; i++) {
> +		const struct camss_isp_pad_desc *pads = descs[i].pads;
> +		unsigned int num_pads = isp_count_pads(pads);
> +
> +		for (pi = 0; pi < num_pads; pi++) {
> +			const struct camss_isp_pad_desc *pad = &pads[pi];
> +			const struct camss_isp_pad_desc *peer_pad;

[ ... ]

> +			/* Verify back-reference consistency */
> +			if (peer_pad->peer_entity >= 0 &&
> +			    ((unsigned int)peer_pad->peer_entity != i ||
> +			     peer_pad->peer_pad != pi)) {
> +				dev_err(dev, "entity[%u].p%u <-> entity[%d].p%u: mismatch\n",
> +					i, pi, peer_ent, pad->peer_pad);
> +				return -EINVAL;
> +			}

[Severity: Medium]
Does this back-reference check silently accept invalid graph topologies?

If a SINK pad descriptor defines a peer_entity, but the targeted SOURCE pad
defines no link (peer_entity == -1), the peer_pad->peer_entity >= 0 check
evaluates to false, bypassing the mismatch validation.

Since links are only created from the SOURCE side later in registration,
will this silently drop the specified SINK link instead of warning the user?

> +		}
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +void camss_isp_pipeline_free(struct camss_isp_pipeline *pipeline)
> +{
> +	kfree(pipeline);
> +}

[Severity: High]
Could freeing the pipeline structure synchronously here cause a use-after-free
for the embedded V4L2 objects?

Because isp_register_vdev() sets video_device_release_empty as the release
callback, the V4L2 core won't defer the memory release. If a userspace process
holds an open file descriptor to the video node when this module is unbound,
could it later access this freed memory?

> +EXPORT_SYMBOL_GPL(camss_isp_pipeline_free);
> +
> +/* -------- Registration -------- */
> +
> +void camss_isp_pipeline_unregister(struct camss_isp_pipeline *pipeline)
> +{
> +	int i;
> +
> +	/* Unregister entities in reverse order */
> +	for (i = (int)pipeline->num_entities - 1; i >= 0; i--) {
> +		struct camss_isp_pipeline_entity *slot = &pipeline->entities[i];
> +
> +		switch (slot->obj_type) {
> +		case MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
> +			if (slot->vdev.name[0])
> +				video_unregister_device(&slot->vdev);
> +			break;
> +		case MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
> +			if (slot->subdev.name[0]) {
> +				v4l2_subdev_cleanup(&slot->subdev);
> +				v4l2_device_unregister_subdev(&slot->subdev);

[Severity: High]
Could this cleanup order expose freed state to concurrent userspace ioctls?

Calling v4l2_subdev_cleanup() frees internal state like sd->active_state.
Because the devnode is not unregistered until v4l2_device_unregister_subdev()
runs on the next line, could a concurrent ioctl like VIDIOC_SUBDEV_G_FMT
reach the subdev and access the freed state during driver teardown?

> +			}
> +			break;
> +		case MEDIA_ENTITY_TYPE_BASE:
> +			if (slot->entity.name) {
> +				media_entity_remove_links(&slot->entity);
> +				media_device_unregister_entity(&slot->entity);
> +			}

[Severity: High]
Could this error handling path cause a NULL pointer dereference?

If an entity fails to register, the err_unregister path calls
camss_isp_pipeline_unregister(), which iterates over all slots and checks
if (slot->entity.name) to determine if they need cleanup.

Because isp_register_base_entity() assigns the name before the registration
attempt:

    isp_register_base_entity()
        entity->name = desc->name;
        ...
        return media_device_register_entity(...);

The unregister function will call media_device_unregister_entity() on an
entity that failed registration. Will this dereference a zeroed list_head
and cause an oops?

> +			break;
> +		}
> +
> +		kfree(slot->pads);
> +		slot->pads = NULL;
> +	}
> +
> +	pipeline->v4l2_dev = NULL;
> +}
> +EXPORT_SYMBOL_GPL(camss_isp_pipeline_unregister);
> +
> +static int isp_register_vdev(struct camss_isp_pipeline_entity *slot,
> +			     const struct camss_isp_entity_desc *desc,
> +			     struct v4l2_device *v4l2_dev)
> +{
> +	struct video_device *vdev = &slot->vdev;
> +	int ret;
> +
> +	strscpy(vdev->name, desc->name, sizeof(vdev->name));

[Severity: Medium]
What happens if desc->name is NULL or an empty string?

If desc->name is NULL, strscpy() will crash. If it is an empty string,
vdev->name[0] will be '\0'. In camss_isp_pipeline_unregister(), the cleanup
path checks if (slot->vdev.name[0]) before unregistering. Will an empty string
cause the teardown to silently skip the entity and leak the V4L2 structures?

> +	vdev->vfl_dir     = isp_caps_to_vfl_dir(desc->vdev.caps);
> +	vdev->v4l2_dev    = v4l2_dev;
> +	vdev->device_caps = desc->vdev.caps | V4L2_CAP_IO_MC;
> +	vdev->release     = video_device_release_empty;
> +	if (desc->vdev.fops)
> +		vdev->fops = desc->vdev.fops;

[ ... ]

> +	ret = media_entity_pads_init(&vdev->entity, slot->num_pads, slot->pads);
> +	if (ret)
> +		return ret;
> +
> +	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> +	if (ret)
> +		return ret;
> +
> +	video_set_drvdata(vdev, desc->vdev.drvdata);

[Severity: High]
Is there a race condition exposing this video device to userspace too early?

video_register_device() makes the /dev/videoX node visible immediately.
Could userspace open it and encounter uninitialized driver data before
video_set_drvdata() runs?

Additionally, could userspace interact with an incomplete topology before the
media links are created later in camss_isp_pipeline_register()?

> +
> +	return 0;
> +}
> +
> +static int isp_register_subdev(struct camss_isp_pipeline_entity *slot,
> +			       const struct camss_isp_entity_desc *desc,
> +			       struct v4l2_device *v4l2_dev)
> +{
> +	struct v4l2_subdev *sd = &slot->subdev;
> +	int ret;
> +
> +	v4l2_subdev_init(sd, desc->subdev.ops);
> +	strscpy(sd->name, desc->name, sizeof(sd->name));

[ ... ]

> +	ret = media_entity_pads_init(&sd->entity, slot->num_pads, slot->pads);
> +	if (ret)
> +		return ret;
> +
> +	ret = v4l2_device_register_subdev(v4l2_dev, sd);
> +	if (ret)
> +		return ret;
> +
> +	ret = v4l2_subdev_init_finalize(sd);

[Severity: Medium]
Is it safe to call v4l2_subdev_init_finalize() after the subdevice has
already been registered?

Since v4l2_device_register_subdev() makes the subdevice visible via the
media controller, could concurrent in-kernel accesses or ioctls racing with
the initialization dereference uninitialized state?

> +	if (ret) {
> +		v4l2_device_unregister_subdev(sd);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-camss-isp-ope-v4-0-51207a0319d8@oss.qualcomm.com?part=1