[PATCH] platform/x86/amd/pmf: fix cleanup in amd_pmf_init_smart_pc()

[PATCH] platform/x86/amd/pmf: fix cleanup in amd_pmf_init_smart_pc()

[Dan Carpenter]

There are a couple problems in this code:

First, if amd_pmf_tee_init() fails then the function returns directly
instead of cleaning up.  We cannot simply do a "goto error;" because
that would lead to a double free.  I have re-written this code to
use an unwind ladder to free the allocations.

Second, if amd_pmf_start_policy_engine() fails on every iteration though
the loop then the code calls amd_pmf_tee_deinit() twice which is also a
double free.  Call amd_pmf_tee_deinit() inside the loop for each failed
iteration.  Also on that path the error codes are not necessarily
negative kernel error codes.  Set the error code to -EINVAL.

Fixes: 376a8c2a1443 ("platform/x86/amd/pmf: Update PMF Driver for Compatibility with new PMF-TA")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/platform/x86/amd/pmf/tee-if.c | 36 +++++++++++++++++++--------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index ceaff1ebb7b9..a1e43873a07b 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -510,18 +510,18 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 
 	ret = amd_pmf_set_dram_addr(dev, true);
 	if (ret)
-		goto error;
+		goto err_cancel_work;
 
 	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
 	if (IS_ERR(dev->policy_base)) {
 		ret = PTR_ERR(dev->policy_base);
-		goto error;
+		goto err_free_dram_buf;
 	}
 
 	dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
 	if (!dev->policy_buf) {
 		ret = -ENOMEM;
-		goto error;
+		goto err_free_dram_buf;
 	}
 
 	memcpy_fromio(dev->policy_buf, dev->policy_base, dev->policy_sz);
@@ -531,13 +531,13 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 	dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
 	if (!dev->prev_data) {
 		ret = -ENOMEM;
-		goto error;
+		goto err_free_policy;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(amd_pmf_ta_uuid); i++) {
 		ret = amd_pmf_tee_init(dev, &amd_pmf_ta_uuid[i]);
 		if (ret)
-			return ret;
+			goto err_free_prev_data;
 
 		ret = amd_pmf_start_policy_engine(dev);
 		switch (ret) {
@@ -550,27 +550,41 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 			status = false;
 			break;
 		default:
-			goto error;
+			ret = -EINVAL;
+			amd_pmf_tee_deinit(dev);
+			goto err_free_prev_data;
 		}
 
 		if (status)
 			break;
 	}
 
-	if (!status && !pb_side_load)
-		goto error;
+	if (!status && !pb_side_load) {
+		ret = -EINVAL;
+		goto err_free_prev_data;
+	}
 
 	if (pb_side_load)
 		amd_pmf_open_pb(dev, dev->dbgfs_dir);
 
 	ret = amd_pmf_register_input_device(dev);
 	if (ret)
-		goto error;
+		goto err_pmf_remove_pb;
 
 	return 0;
 
-error:
-	amd_pmf_deinit_smart_pc(dev);
+err_pmf_remove_pb:
+	if (pb_side_load && dev->esbin)
+		amd_pmf_remove_pb(dev);
+	amd_pmf_tee_deinit(dev);
+err_free_prev_data:
+	kfree(dev->prev_data);
+err_free_policy:
+	kfree(dev->policy_buf);
+err_free_dram_buf:
+	kfree(dev->buf);
+err_cancel_work:
+	cancel_delayed_work_sync(&dev->pb_work);
 
 	return ret;
 }
-- 
2.47.2

Re: [PATCH] platform/x86/amd/pmf: fix cleanup in amd_pmf_init_smart_pc()

[Ilpo Järvinen]

On Mon, 10 Mar 2025, Dan Carpenter wrote:

> There are a couple problems in this code:
> 
> First, if amd_pmf_tee_init() fails then the function returns directly
> instead of cleaning up.  We cannot simply do a "goto error;" because
> that would lead to a double free.  I have re-written this code to
> use an unwind ladder to free the allocations.

Thanks Dan,

Could you please amend this with the information of what is getting 
double freed, it took considerable amount of time for me to figure out.
I assume it's ->fw_shm_pool ?

> Second, if amd_pmf_start_policy_engine() fails on every iteration though
> the loop then the code calls amd_pmf_tee_deinit() twice which is also a
> double free.  Call amd_pmf_tee_deinit() inside the loop for each failed
> iteration.  Also on that path the error codes are not necessarily
> negative kernel error codes.  Set the error code to -EINVAL.

Maybe I should start to consistently reject any attempt to use 
cleanup/deinit helper functions instead of a proper rollback. It 
seems a pattern that is very prone to errors like this.

> Fixes: 376a8c2a1443 ("platform/x86/amd/pmf: Update PMF Driver for Compatibility with new PMF-TA")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/platform/x86/amd/pmf/tee-if.c | 36 +++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index ceaff1ebb7b9..a1e43873a07b 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -510,18 +510,18 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  
>  	ret = amd_pmf_set_dram_addr(dev, true);
>  	if (ret)
> -		goto error;
> +		goto err_cancel_work;
>  
>  	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
>  	if (IS_ERR(dev->policy_base)) {
>  		ret = PTR_ERR(dev->policy_base);
> -		goto error;
> +		goto err_free_dram_buf;
>  	}
>  
>  	dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
>  	if (!dev->policy_buf) {
>  		ret = -ENOMEM;
> -		goto error;
> +		goto err_free_dram_buf;
>  	}
>  
>  	memcpy_fromio(dev->policy_buf, dev->policy_base, dev->policy_sz);
> @@ -531,13 +531,13 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  	dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
>  	if (!dev->prev_data) {
>  		ret = -ENOMEM;
> -		goto error;
> +		goto err_free_policy;
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(amd_pmf_ta_uuid); i++) {
>  		ret = amd_pmf_tee_init(dev, &amd_pmf_ta_uuid[i]);
>  		if (ret)
> -			return ret;
> +			goto err_free_prev_data;
>  
>  		ret = amd_pmf_start_policy_engine(dev);
>  		switch (ret) {
> @@ -550,27 +550,41 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  			status = false;
>  			break;
>  		default:
> -			goto error;
> +			ret = -EINVAL;
> +			amd_pmf_tee_deinit(dev);
> +			goto err_free_prev_data;
>  		}
>  
>  		if (status)
>  			break;
>  	}
>  
> -	if (!status && !pb_side_load)
> -		goto error;
> +	if (!status && !pb_side_load) {
> +		ret = -EINVAL;
> +		goto err_free_prev_data;
> +	}
>  
>  	if (pb_side_load)
>  		amd_pmf_open_pb(dev, dev->dbgfs_dir);
>  
>  	ret = amd_pmf_register_input_device(dev);
>  	if (ret)
> -		goto error;
> +		goto err_pmf_remove_pb;
>  
>  	return 0;
>  
> -error:
> -	amd_pmf_deinit_smart_pc(dev);
> +err_pmf_remove_pb:
> +	if (pb_side_load && dev->esbin)
> +		amd_pmf_remove_pb(dev);
> +	amd_pmf_tee_deinit(dev);
> +err_free_prev_data:
> +	kfree(dev->prev_data);
> +err_free_policy:
> +	kfree(dev->policy_buf);
> +err_free_dram_buf:
> +	kfree(dev->buf);
> +err_cancel_work:
> +	cancel_delayed_work_sync(&dev->pb_work);
>  
>  	return ret;
>  }
> 

-- 
 i.

Re: [PATCH] platform/x86/amd/pmf: fix cleanup in amd_pmf_init_smart_pc()

[Dan Carpenter]

On Mon, Mar 10, 2025 at 02:43:51PM +0200, Ilpo Järvinen wrote:
> On Mon, 10 Mar 2025, Dan Carpenter wrote:
> 
> > There are a couple problems in this code:
> > 
> > First, if amd_pmf_tee_init() fails then the function returns directly
> > instead of cleaning up.  We cannot simply do a "goto error;" because
> > that would lead to a double free.  I have re-written this code to
> > use an unwind ladder to free the allocations.
> 
> Thanks Dan,
> 
> Could you please amend this with the information of what is getting 
> double freed, it took considerable amount of time for me to figure out.
> I assume it's ->fw_shm_pool ?
> 

Yes, that's it.  Sure, I can re-write that.

> > Second, if amd_pmf_start_policy_engine() fails on every iteration though
> > the loop then the code calls amd_pmf_tee_deinit() twice which is also a
> > double free.  Call amd_pmf_tee_deinit() inside the loop for each failed
> > iteration.  Also on that path the error codes are not necessarily
> > negative kernel error codes.  Set the error code to -EINVAL.
> 
> Maybe I should start to consistently reject any attempt to use 
> cleanup/deinit helper functions instead of a proper rollback. It 
> seems a pattern that is very prone to errors like this.

I do not like deinit functions.  They are so hard to review.  But I
detected this bug because of a Smatch warning:

drivers/platform/x86/amd/pmf/tee-if.c:540 amd_pmf_init_smart_pc() warn: missing unwind goto?

regards,
dan carpenter

Re: [PATCH] platform/x86/amd/pmf: fix cleanup in amd_pmf_init_smart_pc()

[Shyam Sundar S K]

Hi Dan,

On 3/10/2025 20:22, Dan Carpenter wrote:
> On Mon, Mar 10, 2025 at 02:43:51PM +0200, Ilpo Järvinen wrote:
>> On Mon, 10 Mar 2025, Dan Carpenter wrote:
>>
>>> There are a couple problems in this code:
>>>
>>> First, if amd_pmf_tee_init() fails then the function returns directly
>>> instead of cleaning up.  We cannot simply do a "goto error;" because
>>> that would lead to a double free.  I have re-written this code to
>>> use an unwind ladder to free the allocations.
>>
>> Thanks Dan,
>>
>> Could you please amend this with the information of what is getting 
>> double freed, it took considerable amount of time for me to figure out.
>> I assume it's ->fw_shm_pool ?
>>
> 
> Yes, that's it.  Sure, I can re-write that.
> 
>>> Second, if amd_pmf_start_policy_engine() fails on every iteration though
>>> the loop then the code calls amd_pmf_tee_deinit() twice which is also a
>>> double free.  Call amd_pmf_tee_deinit() inside the loop for each failed
>>> iteration.  Also on that path the error codes are not necessarily
>>> negative kernel error codes.  Set the error code to -EINVAL.
>>
>> Maybe I should start to consistently reject any attempt to use 
>> cleanup/deinit helper functions instead of a proper rollback. It 
>> seems a pattern that is very prone to errors like this.
> 
> I do not like deinit functions.  They are so hard to review.  But I
> detected this bug because of a Smatch warning:
> 
> drivers/platform/x86/amd/pmf/tee-if.c:540 amd_pmf_init_smart_pc() warn: missing unwind goto?

Thank you for the fix. We have a CI that runs sparse/smatch/coverity
to catch all these issues and unfortunately this was not caught by the
CI system.

But, I can confirm that Smatch is triggering this.

drivers/platform/x86/amd/pmf/tee-if.c
  CHECK   scripts/mod/empty.c
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC [M]  drivers/platform/x86/amd/pmf/tee-if.o
  CHECK   drivers/platform/x86/amd/pmf/tee-if.c
drivers/platform/x86/amd/pmf/tee-if.c:540 amd_pmf_init_smart_pc()
warn: missing unwind goto?

Thanks,
Shyam