Re: [PATCH v6 1/9] media: mc-entity: Store parsed V4L2 fwnode endpoint in media_pad

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Captured fwnode endpoint contains a dangling pointer to `local_fwnode` if the caller drops its reference after parsing.
- [Medium] Embedding a large, V4L2-specific `struct v4l2_fwnode_endpoint` into the generic Media Controller `struct media_pad` breaks subsystem abstraction and unconditionally bloats memory.
--

commit 7acfb56349845d6638c963825f0f6b087ebaa54d
Author: Frank Li <Frank.Li@nxp.com>

media: mc-entity: Store parsed V4L2 fwnode endpoint in media_pad

This commit captures the parsed v4l2 fwnode endpoint information directly
within struct media_pad to allow reuse by consumers without reparsing.

> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index d9b72cd87d524..4a3785cd9f370 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -20,6 +20,8 @@
>  #include <linux/minmax.h>
>  #include <linux/types.h>
>  
> +#include <media/v4l2-fwnode.h>
> +

[Severity: Medium]
Does this include break subsystem isolation? struct media_pad is a generic
structure used across several subsystems like alsa, dvb, drm, and iio.
Including a v4l2-specific header here appears to tie the generic media
controller to v4l2.

[ ... ]

> @@ -240,7 +243,7 @@ struct media_pad {
>  	u16 num_links;
>  	enum media_pad_signal_type sig_type;
>  	unsigned long flags;
> -
> +	struct v4l2_fwnode_endpoint vep;

[Severity: Medium]
Is it expected to embed this v4l2-specific structure into every generic
media_pad? The v4l2_fwnode_endpoint struct is quite large, and
unconditionally embedding it adds memory overhead for all non-v4l2
entities that do not use fwnode endpoints.

[Severity: Critical]
Can this cause a use-after-free regression with the fwnode pointers?

When the endpoint is parsed, v4l2_fwnode_endpoint_parse() assigns the
fwnode to vep->base.local_fwnode without taking a reference count. This
was safe when vep was a transient stack variable, but now that it is
stored persistently in the media_pad, the caller might drop the
reference.

For example, if the caller iterates using a scoped iterator:

drivers/media/v4l2-core/v4l2-fwnode.c:v4l2_async_nf_parse_fwnode() {
    ...
    fwnode_graph_for_each_endpoint_scoped(dev_fwnode(dev), ep) {
        ret = v4l2_fwnode_endpoint_parse(ep, &pad->vep);
    ...
}

The scoped iterator automatically drops the reference to ep at the end
of the iteration, leaving pad->vep.base.local_fwnode as a dangling
pointer.

Later access or pointer comparisons, like in v4l2_common_notifier_bound(),
might dereference freed memory if the fwnode is deleted.

>  	/*
>  	 * The fields below are private, and should only be accessed via
>  	 * appropriate functions.

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