Re: [PATCH v6 5/9] media: synopsys: Use media_async_register_subdev() to simplify code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

New issues:
- [Critical] Critical out-of-bounds heap write in v4l2_async_nf_parse_fwnode() due to mismatched endpoint counting.
- [High] Mixed goto-based error handling and __free() cleanup violates subsystem rules and leaves a dangling pointer.
- [High] Hardcoded array indexing for pads mismatches the dynamic Device Tree enumeration order.
- [High] Heap out-of-bounds access and routing validation failure on single-endpoint Device Trees.
- [High] Memory leak of `sd->active_state` on subdev registration failure.
- [High] Unconditional memory leak of `sd->subdev_notifier` during driver removal.
- [High] Leaked async connections and fwnode references on error path.

Pre-existing issues:
- [High] Lockless traversal of the media entity links list causes a race condition.
--

commit 892732bd4672cbe12751cfc2979a3f508c5cde6d
Author: Frank Li <Frank.Li@nxp.com>
Subject: media: synopsys: Use media_async_register_subdev() to simplify code

Use the media_async_register_subdev() to simplify the driver.
Replace the local subdev registration and media pad setup code with
media_async_register_subdev(). Reduce boilerplate code and aligns the
driver with the common pattern used by simple subdevices that each media
pad has one endpoint in fwnode.

> diff --git a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> index f51367409ff46..0fabc89a49b80 100644
> --- a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> +++ b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
[ ... ]
> @@ -360,9 +350,10 @@ dw_mipi_csi2rx_find_format(struct dw_mipi_csi2rx_device *csi2, u32 mbus_code)
>  
>  static int dw_mipi_csi2rx_start(struct dw_mipi_csi2rx_device *csi2)
>  {
> +	struct media_pad *sink_pad = &csi2->sd.entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];

[Severity: High]
Does this hardcoded index match the dynamic pad allocation from the Device
Tree?

Because v4l2_async_nf_parse_fwnode() populates the pads array strictly in the
order endpoints appear in the Device Tree, accessing index
V4L2_SUBDEV_1TO1_PADS_SINK unconditionally here assumes the sink pad is always
at index 0.

If a platform's Device Tree lists the source endpoint before the sink
endpoint, will the driver access the wrong pad and potentially read incorrect
hardware parameters (like lane counts)?

> +	u32 lanes = sink_pad->vep.bus.mipi_csi2.num_data_lanes;
>  	struct media_pad *source_pad;
[ ... ]
> @@ -549,12 +539,12 @@ static int dw_mipi_csi2rx_enable_streams(struct v4l2_subdev *sd,
>  	u64 mask;
>  	int ret;
>  
> -	sink_pad = &sd->entity.pads[DW_MIPI_CSI2RX_PAD_SINK];
> +	sink_pad = &sd->entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
>  	remote_pad = media_pad_remote_pad_first(sink_pad);

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless list
traversal race with userspace ioctls?

Calling media_pad_remote_pad_first() iterates the media entity links list:

drivers/media/mc/mc-entity.c:media_pad_remote_pad_first() {
    ...
    for_each_media_entity_data_link(pad->entity, link) {
    ...
}

Since v4l2_subdev_enable_streams() does not acquire the global
mdev->graph_mutex, could this traversal race with userspace ioctls like
MEDIA_IOC_SETUP_LINK that mutate the graph structure, leading to linked list
corruption or a use-after-free?

>  	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
[ ... ]
> @@ -649,9 +639,9 @@ static int dw_mipi_csi2rx_init_state(struct v4l2_subdev *sd,
>  {
>  	struct v4l2_subdev_route routes[] = {
>  		{
> -			.sink_pad = DW_MIPI_CSI2RX_PAD_SINK,
> +			.sink_pad = V4L2_SUBDEV_1TO1_PADS_SINK,
>  			.sink_stream = 0,
> -			.source_pad = DW_MIPI_CSI2RX_PAD_SRC,
> +			.source_pad = V4L2_SUBDEV_1TO1_PADS_SOURCE,

[Severity: High]
Could this cause an out-of-bounds heap access if the Device Tree only defines
a single endpoint?

When __media_async_register_subdev() dynamically sizes the pads array based
on ep_count, a Device Tree with only a sink endpoint will result in an array
of size 1.

Since the driver unconditionally sets up active routes referring to
V4L2_SUBDEV_1TO1_PADS_SOURCE (index 1), won't this attempt to access pads[1]
on a size-1 array?

>  			.source_stream = 0,
>  			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
>  		},
[ ... ]
> @@ -764,45 +697,12 @@ static int dw_mipi_csi2rx_register(struct dw_mipi_csi2rx_device *csi2)
>  	snprintf(sd->name, sizeof(sd->name), "dw-mipi-csi2rx %s",
>  		 dev_name(csi2->dev));
>  
> -	pads[DW_MIPI_CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK |
> -					      MEDIA_PAD_FL_MUST_CONNECT;
> -	pads[DW_MIPI_CSI2RX_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE;
> -	ret = media_entity_pads_init(&sd->entity, DW_MIPI_CSI2RX_PAD_MAX, pads);
> +	ret = media_async_register_subdev(&csi2->sd);

[Severity: High]
Does calling media_async_register_subdev() here expose a dangling pointer due
to mixed goto and __free() usage in __media_async_register_subdev()?

In drivers/media/v4l2-core/v4l2-fwnode.c:__media_async_register_subdev(), the
pads array is bound to scope-based cleanup and assigned to sd->entity.pads:

    struct media_pad *pads __free(kfree) = kzalloc_objs(struct media_pad,
                                                        ep_count);
    ...
    sd->entity.pads = pads;

If __v4l2_async_register_subdev() fails, the code jumps to out_unregister:

    out_unregister:
        v4l2_async_nf_unregister(notifier);

Because the function returns, pads is automatically freed, but sd->entity.pads
is not set to NULL. Does this leave a dangling pointer in the subdevice?

[Severity: High]
Could media_async_register_subdev() leak the active state if registration
fails?

In __media_async_register_subdev(), v4l2_subdev_init_finalize() dynamically
allocates sd->active_state. If the subsequent call to
__v4l2_async_register_subdev() fails:

    ret = v4l2_subdev_init_finalize(sd);
    if (ret)
        goto out_unregister;

    ret = __v4l2_async_register_subdev(sd, module);
    if (ret < 0)
        goto out_unregister;

The error path jumps to out_unregister and returns without calling
v4l2_subdev_cleanup(). Does this bypass freeing the active state?

[Severity: Critical]
Can this call trigger a heap out-of-bounds write if the Device Tree contains
disabled or disconnected endpoints?

In __media_async_register_subdev(), the pads array is allocated based on
ep_count, which only counts available connected endpoints (flags=0):

    struct media_pad *pads __free(kfree) = kzalloc_objs(struct media_pad,
                                                        ep_count);

However, v4l2_async_nf_parse_fwnode() iterates over all endpoints without
filtering:

    fwnode_graph_for_each_endpoint_scoped(dev_fwnode(dev), ep) {
        ...
        ret = v4l2_fwnode_endpoint_parse(ep, &pad->vep);

If there are disconnected endpoints in the Device Tree, the loop will iterate
more times than ep_count. Will this increment the pad pointer beyond the
allocated array and overwrite adjacent memory?

[Severity: High]
Does media_async_register_subdev() leak async connections and fwnode
references if an error occurs during endpoint parsing?

In v4l2_async_nf_parse_fwnode(), newly allocated async connections are added
to the notifier. If an error occurs in a subsequent loop iteration:

    if (IS_ERR(asd))
        return dev_err_probe(dev, PTR_ERR(asd), "failed to add notifier\n");

The function returns directly. Then in __media_async_register_subdev():

    ret = v4l2_async_nf_parse_fwnode(dev, pads, notifier);
    if (ret < 0)
        return ret;

This bypasses the out_cleanup label and fails to call v4l2_async_nf_cleanup(),
which seems to leak everything added to the notifier so far.

>  	if (ret)
> -		goto err_notifier_unregister;
[ ... ]
> @@ -993,7 +895,7 @@ static void dw_mipi_csi2rx_remove(struct platform_device *pdev)
>  {
>  	struct dw_mipi_csi2rx_device *csi2 = platform_get_drvdata(pdev);
>  
> -	dw_mipi_csi2rx_unregister(csi2);
> +	media_async_subdev_cleanup(&csi2->sd);

[Severity: High]
Does media_async_subdev_cleanup() unconditionally leak the subdevice notifier?

The function __media_async_register_subdev() dynamically allocates the
notifier and assigns it to sd->subdev_notifier. During teardown here,
media_async_subdev_cleanup() calls v4l2_async_nf_cleanup() to clean the lists:

drivers/media/v4l2-core/v4l2-fwnode.c:media_async_subdev_cleanup() {
    ...
    v4l2_async_nf_cleanup(sd->subdev_notifier);
    ...
}

However, it does not appear to call kfree(sd->subdev_notifier). Will this
leak the notifier structure every time the module is unbound or removed?

>  	phy_exit(csi2->phy);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-imx8qxp_pcam-v6-0-4b3f45920d2f@nxp.com?part=5