linux-kernel - Re: [PATCH] platform/x86/amd/pmf: fix cleanup in amd_pmf_init_smart

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <32c6c456-94f0-f077-040c-09f67d60953a@linux.intel.com>
Date: Mon, 10 Mar 2025 14:43:51 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
cc: Shyam Sundar S K <Shyam-sundar.S-k@....com>, 
    Hans de Goede <hdegoede@...hat.com>, 
    Patil Rajesh Reddy <Patil.Reddy@....com>, 
    Mario Limonciello <mario.limonciello@....com>, 
    platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, 
    kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] platform/x86/amd/pmf: fix cleanup in
 amd_pmf_init_smart_pc()

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@...aro.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.