thermal/core: fix potential unbalanced put_device during register

Commit c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
causes device_put() to be called if the get_max_state() callback fails
during __thermal_cooling_device_register().

Fix the cleanup ordering to only call device_put() if initialization
fails after the matching device_register() call.

Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/thermal/thermal_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

On 31-12-22, 21:03, Caleb Connolly wrote:
> Commit c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> causes device_put() to be called if the get_max_state() callback fails
> during __thermal_cooling_device_register().
> 
> Fix the cleanup ordering to only call device_put() if initialization
> fails after the matching device_register() call.
> 
> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f17ab2316dbd..2c6995b5dcb0 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -920,7 +920,7 @@ __thermal_cooling_device_register(struct device_node *np,
>  	}
>  	ret = device_register(&cdev->device);
>  	if (ret)
> -		goto out_kfree_type;
> +		goto out_put_device;
>  
>  	/* Add 'this' new cdev to the global cdev list */
>  	mutex_lock(&thermal_list_lock);
> @@ -939,10 +939,11 @@ __thermal_cooling_device_register(struct device_node *np,
>  
>  	return cdev;
>  
> +out_put_device:
> +	put_device(&cdev->device);
>  out_kfree_type:
>  	thermal_cooling_device_destroy_sysfs(cdev);

What about this one ? This shouldn't be called in case get_max_state() fails,
right ?

>  	kfree(cdev->type);
> -	put_device(&cdev->device);
>  	cdev = NULL;
>  out_ida_remove:
>  	ida_free(&thermal_cdev_ida, id);

On 02/01/2023 05:31, Viresh Kumar wrote:
> On 31-12-22, 21:03, Caleb Connolly wrote:
>> Commit c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
>> causes device_put() to be called if the get_max_state() callback fails
>> during __thermal_cooling_device_register().
>>
>> Fix the cleanup ordering to only call device_put() if initialization
>> fails after the matching device_register() call.
>>
>> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>>  drivers/thermal/thermal_core.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index f17ab2316dbd..2c6995b5dcb0 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -920,7 +920,7 @@ __thermal_cooling_device_register(struct device_node *np,
>>  	}
>>  	ret = device_register(&cdev->device);
>>  	if (ret)
>> -		goto out_kfree_type;
>> +		goto out_put_device;
>>  
>>  	/* Add 'this' new cdev to the global cdev list */
>>  	mutex_lock(&thermal_list_lock);
>> @@ -939,10 +939,11 @@ __thermal_cooling_device_register(struct device_node *np,
>>  
>>  	return cdev;
>>  
>> +out_put_device:
>> +	put_device(&cdev->device);
>>  out_kfree_type:
>>  	thermal_cooling_device_destroy_sysfs(cdev);
> 
> What about this one ? This shouldn't be called in case get_max_state() fails,
> right ?

Right, I missed that one! It even gets called twice if dev_set_name()
fails...
> 
>>  	kfree(cdev->type);
>> -	put_device(&cdev->device);
>>  	cdev = NULL;
>>  out_ida_remove:
>>  	ida_free(&thermal_cdev_ida, id);
>

On 31-12-22, 21:03, Caleb Connolly wrote:
> Commit c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> causes device_put() to be called if the get_max_state() callback fails
> during __thermal_cooling_device_register().
> 
> Fix the cleanup ordering to only call device_put() if initialization
> fails after the matching device_register() call.
> 
> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Should it be like this instead ?

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index f17ab2316dbd..18db011d4d68 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -910,17 +910,15 @@ __thermal_cooling_device_register(struct device_node *np,

        ret = cdev->ops->get_max_state(cdev, &cdev->max_state);
        if (ret)
-               goto out_kfree_type;
+               goto out_cdev_type;

        thermal_cooling_device_setup_sysfs(cdev);
        ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
-       if (ret) {
-               thermal_cooling_device_destroy_sysfs(cdev);
-               goto out_kfree_type;
-       }
+       if (ret)
+               goto out_cooling_dev;
        ret = device_register(&cdev->device);
        if (ret)
-               goto out_kfree_type;
+               goto out_put_device;

        /* Add 'this' new cdev to the global cdev list */
        mutex_lock(&thermal_list_lock);
@@ -939,11 +937,12 @@ __thermal_cooling_device_register(struct device_node *np,

        return cdev;

-out_kfree_type:
+out_put_device:
+       put_device(&cdev->device);
+out_cooling_dev:
        thermal_cooling_device_destroy_sysfs(cdev);
+out_cdev_type:
        kfree(cdev->type);
-       put_device(&cdev->device);
-       cdev = NULL;
 out_ida_remove:
        ida_free(&thermal_cdev_ida, id);
 out_kfree_cdev: