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

[<Balakrishnan.S@microchip.com>]

Hi Eugen,

On 12/08/26 10:49 pm, Eugen Hristev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 8/11/26 14:03, Balakrishnan.S@microchip.com wrote:
>> Hi Eugen,
>>
>> On 07/08/26 4:07 pm, Eugen Hristev wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> 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.
>>
>> Sure, let me move the put inside the loop at each failure site so epn is
>> not touched after it. My intention with the err_put label was only to
>> avoid repeating 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 ?
>>
>> subdev_entity is devm_kzalloc'd and the list head lives in isc, so both
>> are freed when probe fails so I felt a list_del is not really needed
>> here. Happy to add it though if you prefer having it makes the unwind clean.
>>
>> Please let me know your thoughts ?
> 
> I guess it's fine if you leave it like that. If others have any other
> opinion, please shout.

Noted. Thanks for the insight.

Thanks,
Balakrishnan

> 
> Eugen
> 
>>
>> Thanks,
>> Balakrishnan
>>>
>>>> +     return ret;
>>>>    }
>>>>
>>>>    static int microchip_xisc_probe(struct platform_device *pdev)
>>>>
>>>
>>
>