Re: [PATCH v4 02/10] media: microchip-isc: take a reference on the parsed endpoints

[Eugen Hristev <ehristev@kernel.org>]

On 8/3/26 13:20, Balakrishnan Sambath wrote:
> for_each_endpoint_of_node() drops the reference on the current node as
> it advances. xisc_parse_dt() and isc_parse_dt() store the node in
> subdev_entity->epn and release it later with of_node_put(), but never
> took their own reference, so the stored pointer refers to an
> already-released node. This underflows the refcount and can
> use-after-free, reachable through the camera device tree overlay.
> 
> Take a reference with of_node_get() when storing the node, and drop it
> in microchip_isc_subdev_cleanup() so the entities the bind loop never
> reaches on an early exit do not leak it.
> 
> Fixes: c9aa973884a1 ("media: atmel: atmel-isc: add microchip-xisc driver")
> Fixes: d6701f13bd07 ("media: atmel: Use v4l2_async_notifier_add_fwnode_remote_subdev")
> Cc: stable@vger.kernel.org
> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
> ---
>  drivers/media/platform/microchip/microchip-isc-base.c  |  6 ++++++
>  .../media/platform/microchip/microchip-sama5d2-isc.c   | 18 ++++++++++++------
>  .../media/platform/microchip/microchip-sama7g5-isc.c   | 18 ++++++++++++------
>  3 files changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index eebbcb28a7ee..ca4f3b5f58aa 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -1853,6 +1853,12 @@ void microchip_isc_subdev_cleanup(struct isc_device *isc)
>  	list_for_each_entry(subdev_entity, &isc->subdev_entities, list) {
>  		v4l2_async_nf_unregister(&subdev_entity->notifier);
>  		v4l2_async_nf_cleanup(&subdev_entity->notifier);
> +		/*
> +		 * Release the endpoint reference taken while parsing. It is
> +		 * NULL for entities the bind loop already consumed, so this
> +		 * only drops the ones left over on an early exit.
> +		 */
> +		of_node_put(subdev_entity->epn);
>  	}
>  
>  	INIT_LIST_HEAD(&isc->subdev_entities);
> diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
> index 25d241b4c66a..532f2b50e2ea 100644
> --- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
> +++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
> @@ -357,28 +357,28 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
>  	struct device_node *epn;
>  	struct isc_subdev_entity *subdev_entity;
>  	unsigned int flags;
> +	int ret;
>  
>  	INIT_LIST_HEAD(&isc->subdev_entities);
>  
>  	for_each_endpoint_of_node(np, epn) {
>  		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
> -		int ret;
>  
>  		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
>  						 &v4l2_epn);
>  		if (ret) {
> -			of_node_put(epn);
>  			dev_err(dev, "Could not parse the endpoint\n");
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			goto err_put;
>  		}
>  
>  		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
>  					     GFP_KERNEL);
>  		if (!subdev_entity) {
> -			of_node_put(epn);
> -			return -ENOMEM;
> +			ret = -ENOMEM;
> +			goto err_put;
>  		}
> -		subdev_entity->epn = epn;
> +		subdev_entity->epn = of_node_get(epn);
>  
>  		flags = v4l2_epn.bus.parallel.flags;
>  
> @@ -399,6 +399,12 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
>  	}
>  
>  	return 0;
> +
> +err_put:
> +	of_node_put(epn);
> +	list_for_each_entry(subdev_entity, &isc->subdev_entities, list)
> +		of_node_put(subdev_entity->epn);
> +	return ret;
>  }
>  
>  static int microchip_isc_probe(struct platform_device *pdev)
> diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> index 998146adabd8..0b72a61b51ee 100644
> --- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> +++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> @@ -341,6 +341,7 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
>  	struct isc_subdev_entity *subdev_entity;
>  	unsigned int flags;
>  	bool mipi_mode;
> +	int ret;
>  
>  	INIT_LIST_HEAD(&isc->subdev_entities);
>  
> @@ -348,23 +349,22 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
>  
>  	for_each_endpoint_of_node(np, epn) {
>  		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
> -		int ret;
>  
>  		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
>  						 &v4l2_epn);
>  		if (ret) {
> -			of_node_put(epn);
>  			dev_err(dev, "Could not parse the endpoint\n");
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			goto err_put;
>  		}
>  
>  		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
>  					     GFP_KERNEL);
>  		if (!subdev_entity) {
> -			of_node_put(epn);
> -			return -ENOMEM;
> +			ret = -ENOMEM;
> +			goto err_put;
>  		}
> -		subdev_entity->epn = epn;
> +		subdev_entity->epn = of_node_get(epn);
>  
>  		flags = v4l2_epn.bus.parallel.flags;
>  
> @@ -388,6 +388,12 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
>  	}
>  
>  	return 0;
> +
> +err_put:
> +	of_node_put(epn);

epn is used for the iterator right ? Why calling put here outside of the
iterator ?
Even if the code is duplicated it makes more sense for me to cleanup
inside the iterator.
Outside the iterator the epn should be unused, at least my take on it.

> +	list_for_each_entry(subdev_entity, &isc->subdev_entities, list)
> +		of_node_put(subdev_entity->epn);

Does it make sense to remove the entities from the list as well , aka
cleanup the list ?

> +	return ret;
>  }
>  
>  static int microchip_xisc_probe(struct platform_device *pdev)
>