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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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.