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

[Balakrishnan.S]

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@xxxxxxxxxxxxxxx
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@xxxxxxxxxxxxx>
>> ---
>>   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 ?

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