[PATCH] thermal/of: Fix possible memleak in thermal_of_zone_register()

[PATCH] thermal/of: Fix possible memleak in thermal_of_zone_register()

[Zhang Qilong]

In the error path, we forget to free the memory that allocated
to of_ops in thermal_of_zone_register(), it can cause memleak
when error returns. We fix it by moving kmemdup to the front of
using it and freeing it in the later error path.

Fixes: 3fd6d6e2b4e8 ("thermal/of: Rework the thermal device tree initialization")
Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
---
 drivers/thermal/thermal_of.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index d4b6335ace15..fc8fa27480a1 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -596,10 +596,6 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor,
 	int ntrips, mask;
 	int ret;
 
-	of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
-	if (!of_ops)
-		return ERR_PTR(-ENOMEM);
-
 	np = of_thermal_zone_find(sensor, id);
 	if (IS_ERR(np)) {
 		if (PTR_ERR(np) != -ENODEV)
@@ -626,6 +622,12 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor,
 		goto out_kfree_trips;
 	}
 
+	of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
+	if (!of_ops) {
+		ret = -ENOMEM;
+		goto out_kfree_tzp;
+	}
+
 	of_ops->get_trip_type = of_ops->get_trip_type ? : of_thermal_get_trip_type;
 	of_ops->get_trip_temp = of_ops->get_trip_temp ? : of_thermal_get_trip_temp;
 	of_ops->get_trip_hyst = of_ops->get_trip_hyst ? : of_thermal_get_trip_hyst;
@@ -656,6 +658,7 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor,
 	return tz;
 
 out_kfree_tzp:
+	kfree(of_ops);
 	kfree(tzp);
 out_kfree_trips:
 	kfree(trips);
-- 
2.25.1

Re: [PATCH] thermal/of: Fix possible memleak in thermal_of_zone_register()

[Ido Schimmel]

On Thu, Oct 20, 2022 at 04:00:48PM +0800, Zhang Qilong wrote:
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index d4b6335ace15..fc8fa27480a1 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -596,10 +596,6 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor,
>  	int ntrips, mask;
>  	int ret;
>  
> -	of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> -	if (!of_ops)
> -		return ERR_PTR(-ENOMEM);
> -
>  	np = of_thermal_zone_find(sensor, id);
>  	if (IS_ERR(np)) {
>  		if (PTR_ERR(np) != -ENODEV)
> @@ -626,6 +622,12 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor,
>  		goto out_kfree_trips;
>  	}
>  
> +	of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> +	if (!of_ops) {
> +		ret = -ENOMEM;
> +		goto out_kfree_tzp;
> +	}
> +
>  	of_ops->get_trip_type = of_ops->get_trip_type ? : of_thermal_get_trip_type;
>  	of_ops->get_trip_temp = of_ops->get_trip_temp ? : of_thermal_get_trip_temp;
>  	of_ops->get_trip_hyst = of_ops->get_trip_hyst ? : of_thermal_get_trip_hyst;
> @@ -656,6 +658,7 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor,
>  	return tz;
>  
>  out_kfree_tzp:
> +	kfree(of_ops);
>  	kfree(tzp);
>  out_kfree_trips:
>  	kfree(trips);

The patch looks correct, but it can be cleaner. Ideally, you would have
a separate label to free 'of_ops' like we have for other variables.
Also, the error path is not symmetric with thermal_of_zone_unregister()
where this variable is the last to be freed, not the first.

I also encountered this issue and posted a patch:
https://lore.kernel.org/linux-pm/20221020103658.802457-1-idosch@nvidia.com/

Unless you see something wrong with it, can you please test and see if
it fixes your issue?

Thanks

答复: [PATCH] thermal/of: Fix possible memleak in thermal_of_zone_register()

[zhangqilong]

> On Thu, Oct 20, 2022 at 04:00:48PM +0800, Zhang Qilong wrote:
> > diff --git a/drivers/thermal/thermal_of.c
> > b/drivers/thermal/thermal_of.c index d4b6335ace15..fc8fa27480a1 100644
> > --- a/drivers/thermal/thermal_of.c
> > +++ b/drivers/thermal/thermal_of.c
> > @@ -596,10 +596,6 @@ struct thermal_zone_device
> *thermal_of_zone_register(struct device_node *sensor,
> >  	int ntrips, mask;
> >  	int ret;
> >
> > -	of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> > -	if (!of_ops)
> > -		return ERR_PTR(-ENOMEM);
> > -
> >  	np = of_thermal_zone_find(sensor, id);
> >  	if (IS_ERR(np)) {
> >  		if (PTR_ERR(np) != -ENODEV)
> > @@ -626,6 +622,12 @@ struct thermal_zone_device
> *thermal_of_zone_register(struct device_node *sensor,
> >  		goto out_kfree_trips;
> >  	}
> >
> > +	of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> > +	if (!of_ops) {
> > +		ret = -ENOMEM;
> > +		goto out_kfree_tzp;
> > +	}
> > +
> >  	of_ops->get_trip_type = of_ops->get_trip_type ? :
> of_thermal_get_trip_type;
> >  	of_ops->get_trip_temp = of_ops->get_trip_temp ? :
> of_thermal_get_trip_temp;
> >  	of_ops->get_trip_hyst = of_ops->get_trip_hyst ? :
> > of_thermal_get_trip_hyst; @@ -656,6 +658,7 @@ struct
> thermal_zone_device *thermal_of_zone_register(struct device_node
> *sensor,
> >  	return tz;
> >
> >  out_kfree_tzp:
> > +	kfree(of_ops);
> >  	kfree(tzp);
> >  out_kfree_trips:
> >  	kfree(trips);
> 
> The patch looks correct, but it can be cleaner. Ideally, you would have a
> separate label to free 'of_ops' like we have for other variables.
> Also, the error path is not symmetric with thermal_of_zone_unregister()
> where this variable is the last to be freed, not the first.
> 
> I also encountered this issue and posted a patch:
> https://lore.kernel.org/linux-pm/20221020103658.802457-1-
> idosch@nvidia.com/
> 
> Unless you see something wrong with it, can you please test and see if it
> fixes your issue?
> 

Hi,

It looks good to me!

Thanks,
Zhang

> Thanks