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

On 8/11/26 14:03, [email protected] 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: [email protected]
>>> Signed-off-by: Balakrishnan Sambath <[email protected]>
>>> ---
>>>   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.

Eugen

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