[PATCH v3 0/3] Improved cleanup handling for amd-pmf

[PATCH v3 0/3] Improved cleanup handling for amd-pmf

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

I noticed some memory problems with unloading and reloading amd-pmf.
These were root caused with a double free.

This series cleans up that double free by switching to device managed
allocations and also fixes other problems observed in earlier iterations
of the patches.

v2->v3:
 * Add Dan's suggestion
 * Use device managed allocations
 * Simplify error flow 

v2: https://lore.kernel.org/platform-driver-x86/20250507020838.2962896-1-superm1@kernel.org/

Mario Limonciello (3):
  platform/x86/amd: pmf: Use device managed allocations
  platform/x86/amd: pmf: Prevent amd_pmf_tee_deinit() from running twice
  platform/x86/amd: pmf: Simplify error flow in amd_pmf_init_smart_pc()

 drivers/platform/x86/amd/pmf/core.c   |   3 +-
 drivers/platform/x86/amd/pmf/tee-if.c | 100 +++++++++-----------------
 2 files changed, 33 insertions(+), 70 deletions(-)


base-commit: bfcfe6d335a967f8ea0c1980960e6f0205b5de6e
-- 
2.43.0

[PATCH v3 1/3] platform/x86/amd: pmf: Use device managed allocations

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

If setting up smart PC fails for any reason then this can lead to
a double free when unloading amd-pmf.  This is because dev->buf was
freed but never set to NULL and is again freed in amd_pmf_remove().

To avoid subtle allocation bugs in failures leading to a double free
change all allocations into device managed allocations.

Fixes: 5b1122fc4995f ("platform/x86/amd/pmf: fix cleanup in amd_pmf_init_smart_pc()")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmf/core.c   |  3 +-
 drivers/platform/x86/amd/pmf/tee-if.c | 48 +++++++++------------------
 2 files changed, 16 insertions(+), 35 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index 96821101ec77..395c011e837f 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -280,7 +280,7 @@ int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev, bool alloc_buffer)
 			dev_err(dev->dev, "Invalid CPU id: 0x%x", dev->cpu_id);
 		}
 
-		dev->buf = kzalloc(dev->mtable_size, GFP_KERNEL);
+		dev->buf = devm_kzalloc(dev->dev, dev->mtable_size, GFP_KERNEL);
 		if (!dev->buf)
 			return -ENOMEM;
 	}
@@ -493,7 +493,6 @@ static void amd_pmf_remove(struct platform_device *pdev)
 	mutex_destroy(&dev->lock);
 	mutex_destroy(&dev->update_mutex);
 	mutex_destroy(&dev->cb_mutex);
-	kfree(dev->buf);
 }
 
 static const struct attribute_group *amd_pmf_driver_groups[] = {
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index d3bd12ad036a..513dbbe3f214 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -362,26 +362,20 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
 	if (IS_ERR(new_policy_buf))
 		return PTR_ERR(new_policy_buf);
 
-	kfree(dev->policy_buf);
-	dev->policy_buf = new_policy_buf;
+	devm_kfree(dev->dev, dev->policy_buf);
+	dev->policy_buf = devm_kmemdup(dev->dev, new_policy_buf, dev->policy_sz, GFP_KERNEL);
 	dev->policy_sz = length;
+	kfree(new_policy_buf);
 
-	if (!amd_pmf_pb_valid(dev)) {
-		ret = -EINVAL;
-		goto cleanup;
-	}
+	if (!amd_pmf_pb_valid(dev))
+		return -EINVAL;
 
 	amd_pmf_hex_dump_pb(dev);
 	ret = amd_pmf_start_policy_engine(dev);
 	if (ret < 0)
-		goto cleanup;
+		return ret;
 
 	return length;
-
-cleanup:
-	kfree(dev->policy_buf);
-	dev->policy_buf = NULL;
-	return ret;
 }
 
 static const struct file_operations pb_fops = {
@@ -532,13 +526,13 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
 	if (IS_ERR(dev->policy_base)) {
 		ret = PTR_ERR(dev->policy_base);
-		goto err_free_dram_buf;
+		goto err_cancel_work;
 	}
 
-	dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
+	dev->policy_buf = devm_kzalloc(dev->dev, dev->policy_sz, GFP_KERNEL);
 	if (!dev->policy_buf) {
 		ret = -ENOMEM;
-		goto err_free_dram_buf;
+		goto err_cancel_work;
 	}
 
 	memcpy_fromio(dev->policy_buf, dev->policy_base, dev->policy_sz);
@@ -546,21 +540,21 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 	if (!amd_pmf_pb_valid(dev)) {
 		dev_info(dev->dev, "No Smart PC policy present\n");
 		ret = -EINVAL;
-		goto err_free_policy;
+		goto err_cancel_work;
 	}
 
 	amd_pmf_hex_dump_pb(dev);
 
-	dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
+	dev->prev_data = devm_kzalloc(dev->dev, sizeof(*dev->prev_data), GFP_KERNEL);
 	if (!dev->prev_data) {
 		ret = -ENOMEM;
-		goto err_free_policy;
+		goto err_cancel_work;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(amd_pmf_ta_uuid); i++) {
 		ret = amd_pmf_tee_init(dev, &amd_pmf_ta_uuid[i]);
 		if (ret)
-			goto err_free_prev_data;
+			goto err_cancel_work;
 
 		ret = amd_pmf_start_policy_engine(dev);
 		switch (ret) {
@@ -575,7 +569,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 		default:
 			ret = -EINVAL;
 			amd_pmf_tee_deinit(dev);
-			goto err_free_prev_data;
+			goto err_cancel_work;
 		}
 
 		if (status)
@@ -584,7 +578,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 
 	if (!status && !pb_side_load) {
 		ret = -EINVAL;
-		goto err_free_prev_data;
+		goto err_cancel_work;
 	}
 
 	if (pb_side_load)
@@ -600,12 +594,6 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 	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);
 
@@ -621,11 +609,5 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
 		amd_pmf_remove_pb(dev);
 
 	cancel_delayed_work_sync(&dev->pb_work);
-	kfree(dev->prev_data);
-	dev->prev_data = NULL;
-	kfree(dev->policy_buf);
-	dev->policy_buf = NULL;
-	kfree(dev->buf);
-	dev->buf = NULL;
 	amd_pmf_tee_deinit(dev);
 }
-- 
2.43.0

Re: [PATCH v3 1/3] platform/x86/amd: pmf: Use device managed allocations

[Ilpo Järvinen]

On Mon, 12 May 2025, Mario Limonciello wrote:

> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> If setting up smart PC fails for any reason then this can lead to
> a double free when unloading amd-pmf.  This is because dev->buf was
> freed but never set to NULL and is again freed in amd_pmf_remove().
> 
> To avoid subtle allocation bugs in failures leading to a double free
> change all allocations into device managed allocations.
> 
> Fixes: 5b1122fc4995f ("platform/x86/amd/pmf: fix cleanup in amd_pmf_init_smart_pc()")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/core.c   |  3 +-
>  drivers/platform/x86/amd/pmf/tee-if.c | 48 +++++++++------------------
>  2 files changed, 16 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 96821101ec77..395c011e837f 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -280,7 +280,7 @@ int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev, bool alloc_buffer)
>  			dev_err(dev->dev, "Invalid CPU id: 0x%x", dev->cpu_id);
>  		}
>  
> -		dev->buf = kzalloc(dev->mtable_size, GFP_KERNEL);
> +		dev->buf = devm_kzalloc(dev->dev, dev->mtable_size, GFP_KERNEL);
>  		if (!dev->buf)
>  			return -ENOMEM;
>  	}
> @@ -493,7 +493,6 @@ static void amd_pmf_remove(struct platform_device *pdev)
>  	mutex_destroy(&dev->lock);
>  	mutex_destroy(&dev->update_mutex);
>  	mutex_destroy(&dev->cb_mutex);
> -	kfree(dev->buf);
>  }
>  
>  static const struct attribute_group *amd_pmf_driver_groups[] = {
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index d3bd12ad036a..513dbbe3f214 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -362,26 +362,20 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
>  	if (IS_ERR(new_policy_buf))
>  		return PTR_ERR(new_policy_buf);
>  
> -	kfree(dev->policy_buf);
> -	dev->policy_buf = new_policy_buf;
> +	devm_kfree(dev->dev, dev->policy_buf);
> +	dev->policy_buf = devm_kmemdup(dev->dev, new_policy_buf, dev->policy_sz, GFP_KERNEL);

So now there are two memdups?? And there's no error handling either?!

>  	dev->policy_sz = length;
> +	kfree(new_policy_buf);
>  
> -	if (!amd_pmf_pb_valid(dev)) {
> -		ret = -EINVAL;
> -		goto cleanup;
> -	}
> +	if (!amd_pmf_pb_valid(dev))

Due to lack of error handling, this can deref NULL.

> +		return -EINVAL;
>  
>  	amd_pmf_hex_dump_pb(dev);
>  	ret = amd_pmf_start_policy_engine(dev);
>  	if (ret < 0)
> -		goto cleanup;
> +		return ret;
>  
>  	return length;
> -
> -cleanup:
> -	kfree(dev->policy_buf);
> -	dev->policy_buf = NULL;
> -	return ret;
>  }
>  
>  static const struct file_operations pb_fops = {
> @@ -532,13 +526,13 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
>  	if (IS_ERR(dev->policy_base)) {
>  		ret = PTR_ERR(dev->policy_base);
> -		goto err_free_dram_buf;
> +		goto err_cancel_work;
>  	}
>  
> -	dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
> +	dev->policy_buf = devm_kzalloc(dev->dev, dev->policy_sz, GFP_KERNEL);
>  	if (!dev->policy_buf) {
>  		ret = -ENOMEM;
> -		goto err_free_dram_buf;
> +		goto err_cancel_work;
>  	}
>  
>  	memcpy_fromio(dev->policy_buf, dev->policy_base, dev->policy_sz);
> @@ -546,21 +540,21 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  	if (!amd_pmf_pb_valid(dev)) {
>  		dev_info(dev->dev, "No Smart PC policy present\n");
>  		ret = -EINVAL;
> -		goto err_free_policy;
> +		goto err_cancel_work;
>  	}
>  
>  	amd_pmf_hex_dump_pb(dev);
>  
> -	dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
> +	dev->prev_data = devm_kzalloc(dev->dev, sizeof(*dev->prev_data), GFP_KERNEL);
>  	if (!dev->prev_data) {
>  		ret = -ENOMEM;
> -		goto err_free_policy;
> +		goto err_cancel_work;
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(amd_pmf_ta_uuid); i++) {
>  		ret = amd_pmf_tee_init(dev, &amd_pmf_ta_uuid[i]);
>  		if (ret)
> -			goto err_free_prev_data;
> +			goto err_cancel_work;
>  
>  		ret = amd_pmf_start_policy_engine(dev);
>  		switch (ret) {
> @@ -575,7 +569,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  		default:
>  			ret = -EINVAL;
>  			amd_pmf_tee_deinit(dev);
> -			goto err_free_prev_data;
> +			goto err_cancel_work;
>  		}
>  
>  		if (status)
> @@ -584,7 +578,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  
>  	if (!status && !pb_side_load) {
>  		ret = -EINVAL;
> -		goto err_free_prev_data;
> +		goto err_cancel_work;
>  	}
>  
>  	if (pb_side_load)
> @@ -600,12 +594,6 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  	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);
>  
> @@ -621,11 +609,5 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
>  		amd_pmf_remove_pb(dev);
>  
>  	cancel_delayed_work_sync(&dev->pb_work);
> -	kfree(dev->prev_data);
> -	dev->prev_data = NULL;
> -	kfree(dev->policy_buf);
> -	dev->policy_buf = NULL;
> -	kfree(dev->buf);
> -	dev->buf = NULL;
>  	amd_pmf_tee_deinit(dev);
>  }
> 

-- 
 i.

Re: [PATCH v3 1/3] platform/x86/amd: pmf: Use device managed allocations

[Mario Limonciello]

On 5/14/2025 5:21 AM, Ilpo Järvinen wrote:
> On Mon, 12 May 2025, Mario Limonciello wrote:
> 
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> If setting up smart PC fails for any reason then this can lead to
>> a double free when unloading amd-pmf.  This is because dev->buf was
>> freed but never set to NULL and is again freed in amd_pmf_remove().
>>
>> To avoid subtle allocation bugs in failures leading to a double free
>> change all allocations into device managed allocations.
>>
>> Fixes: 5b1122fc4995f ("platform/x86/amd/pmf: fix cleanup in amd_pmf_init_smart_pc()")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/platform/x86/amd/pmf/core.c   |  3 +-
>>   drivers/platform/x86/amd/pmf/tee-if.c | 48 +++++++++------------------
>>   2 files changed, 16 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
>> index 96821101ec77..395c011e837f 100644
>> --- a/drivers/platform/x86/amd/pmf/core.c
>> +++ b/drivers/platform/x86/amd/pmf/core.c
>> @@ -280,7 +280,7 @@ int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev, bool alloc_buffer)
>>   			dev_err(dev->dev, "Invalid CPU id: 0x%x", dev->cpu_id);
>>   		}
>>   
>> -		dev->buf = kzalloc(dev->mtable_size, GFP_KERNEL);
>> +		dev->buf = devm_kzalloc(dev->dev, dev->mtable_size, GFP_KERNEL);
>>   		if (!dev->buf)
>>   			return -ENOMEM;
>>   	}
>> @@ -493,7 +493,6 @@ static void amd_pmf_remove(struct platform_device *pdev)
>>   	mutex_destroy(&dev->lock);
>>   	mutex_destroy(&dev->update_mutex);
>>   	mutex_destroy(&dev->cb_mutex);
>> -	kfree(dev->buf);
>>   }
>>   
>>   static const struct attribute_group *amd_pmf_driver_groups[] = {
>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>> index d3bd12ad036a..513dbbe3f214 100644
>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>> @@ -362,26 +362,20 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
>>   	if (IS_ERR(new_policy_buf))
>>   		return PTR_ERR(new_policy_buf);
>>   
>> -	kfree(dev->policy_buf);
>> -	dev->policy_buf = new_policy_buf;
>> +	devm_kfree(dev->dev, dev->policy_buf);
>> +	dev->policy_buf = devm_kmemdup(dev->dev, new_policy_buf, dev->policy_sz, GFP_KERNEL);
> 
> So now there are two memdups?? 

Is there a safe way to do it without the double memdups that I'm missing?

I didn't see a helper for device managed memory to copy from user like 
memdup_user() does.

> And there's no error handling either?!

Whoops; will fix.

> 
>>   	dev->policy_sz = length;
>> +	kfree(new_policy_buf);
>>   
>> -	if (!amd_pmf_pb_valid(dev)) {
>> -		ret = -EINVAL;
>> -		goto cleanup;
>> -	}
>> +	if (!amd_pmf_pb_valid(dev))
> 
> Due to lack of error handling, this can deref NULL.
> 
>> +		return -EINVAL;
>>   
>>   	amd_pmf_hex_dump_pb(dev);
>>   	ret = amd_pmf_start_policy_engine(dev);
>>   	if (ret < 0)
>> -		goto cleanup;
>> +		return ret;
>>   
>>   	return length;
>> -
>> -cleanup:
>> -	kfree(dev->policy_buf);
>> -	dev->policy_buf = NULL;
>> -	return ret;
>>   }
>>   
>>   static const struct file_operations pb_fops = {
>> @@ -532,13 +526,13 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>   	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
>>   	if (IS_ERR(dev->policy_base)) {
>>   		ret = PTR_ERR(dev->policy_base);
>> -		goto err_free_dram_buf;
>> +		goto err_cancel_work;
>>   	}
>>   
>> -	dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
>> +	dev->policy_buf = devm_kzalloc(dev->dev, dev->policy_sz, GFP_KERNEL);
>>   	if (!dev->policy_buf) {
>>   		ret = -ENOMEM;
>> -		goto err_free_dram_buf;
>> +		goto err_cancel_work;
>>   	}
>>   
>>   	memcpy_fromio(dev->policy_buf, dev->policy_base, dev->policy_sz);
>> @@ -546,21 +540,21 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>   	if (!amd_pmf_pb_valid(dev)) {
>>   		dev_info(dev->dev, "No Smart PC policy present\n");
>>   		ret = -EINVAL;
>> -		goto err_free_policy;
>> +		goto err_cancel_work;
>>   	}
>>   
>>   	amd_pmf_hex_dump_pb(dev);
>>   
>> -	dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
>> +	dev->prev_data = devm_kzalloc(dev->dev, sizeof(*dev->prev_data), GFP_KERNEL);
>>   	if (!dev->prev_data) {
>>   		ret = -ENOMEM;
>> -		goto err_free_policy;
>> +		goto err_cancel_work;
>>   	}
>>   
>>   	for (i = 0; i < ARRAY_SIZE(amd_pmf_ta_uuid); i++) {
>>   		ret = amd_pmf_tee_init(dev, &amd_pmf_ta_uuid[i]);
>>   		if (ret)
>> -			goto err_free_prev_data;
>> +			goto err_cancel_work;
>>   
>>   		ret = amd_pmf_start_policy_engine(dev);
>>   		switch (ret) {
>> @@ -575,7 +569,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>   		default:
>>   			ret = -EINVAL;
>>   			amd_pmf_tee_deinit(dev);
>> -			goto err_free_prev_data;
>> +			goto err_cancel_work;
>>   		}
>>   
>>   		if (status)
>> @@ -584,7 +578,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>   
>>   	if (!status && !pb_side_load) {
>>   		ret = -EINVAL;
>> -		goto err_free_prev_data;
>> +		goto err_cancel_work;
>>   	}
>>   
>>   	if (pb_side_load)
>> @@ -600,12 +594,6 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>   	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);
>>   
>> @@ -621,11 +609,5 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
>>   		amd_pmf_remove_pb(dev);
>>   
>>   	cancel_delayed_work_sync(&dev->pb_work);
>> -	kfree(dev->prev_data);
>> -	dev->prev_data = NULL;
>> -	kfree(dev->policy_buf);
>> -	dev->policy_buf = NULL;
>> -	kfree(dev->buf);
>> -	dev->buf = NULL;
>>   	amd_pmf_tee_deinit(dev);
>>   }
>>
> 

Re: [PATCH v3 1/3] platform/x86/amd: pmf: Use device managed allocations

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 7164 bytes --]

On Wed, 14 May 2025, Mario Limonciello wrote:
> On 5/14/2025 5:21 AM, Ilpo Järvinen wrote:
> > On Mon, 12 May 2025, Mario Limonciello wrote:
> > 
> > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > 
> > > If setting up smart PC fails for any reason then this can lead to
> > > a double free when unloading amd-pmf.  This is because dev->buf was
> > > freed but never set to NULL and is again freed in amd_pmf_remove().
> > > 
> > > To avoid subtle allocation bugs in failures leading to a double free
> > > change all allocations into device managed allocations.
> > > 
> > > Fixes: 5b1122fc4995f ("platform/x86/amd/pmf: fix cleanup in
> > > amd_pmf_init_smart_pc()")
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > >   drivers/platform/x86/amd/pmf/core.c   |  3 +-
> > >   drivers/platform/x86/amd/pmf/tee-if.c | 48 +++++++++------------------
> > >   2 files changed, 16 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/amd/pmf/core.c
> > > b/drivers/platform/x86/amd/pmf/core.c
> > > index 96821101ec77..395c011e837f 100644
> > > --- a/drivers/platform/x86/amd/pmf/core.c
> > > +++ b/drivers/platform/x86/amd/pmf/core.c
> > > @@ -280,7 +280,7 @@ int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev,
> > > bool alloc_buffer)
> > >   			dev_err(dev->dev, "Invalid CPU id: 0x%x",
> > > dev->cpu_id);
> > >   		}
> > >   -		dev->buf = kzalloc(dev->mtable_size, GFP_KERNEL);
> > > +		dev->buf = devm_kzalloc(dev->dev, dev->mtable_size,
> > > GFP_KERNEL);
> > >   		if (!dev->buf)
> > >   			return -ENOMEM;
> > >   	}
> > > @@ -493,7 +493,6 @@ static void amd_pmf_remove(struct platform_device
> > > *pdev)
> > >   	mutex_destroy(&dev->lock);
> > >   	mutex_destroy(&dev->update_mutex);
> > >   	mutex_destroy(&dev->cb_mutex);
> > > -	kfree(dev->buf);
> > >   }
> > >     static const struct attribute_group *amd_pmf_driver_groups[] = {
> > > diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
> > > b/drivers/platform/x86/amd/pmf/tee-if.c
> > > index d3bd12ad036a..513dbbe3f214 100644
> > > --- a/drivers/platform/x86/amd/pmf/tee-if.c
> > > +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> > > @@ -362,26 +362,20 @@ static ssize_t amd_pmf_get_pb_data(struct file
> > > *filp, const char __user *buf,
> > >   	if (IS_ERR(new_policy_buf))
> > >   		return PTR_ERR(new_policy_buf);
> > >   -	kfree(dev->policy_buf);
> > > -	dev->policy_buf = new_policy_buf;
> > > +	devm_kfree(dev->dev, dev->policy_buf);
> > > +	dev->policy_buf = devm_kmemdup(dev->dev, new_policy_buf,
> > > dev->policy_sz, GFP_KERNEL);
> > 
> > So now there are two memdups?? 
> 
> Is there a safe way to do it without the double memdups that I'm missing?

Why doing it in two stages, first devm_kmalloc() then copy from user, 
wouldn't be safe?

> I didn't see a helper for device managed memory to copy from user like
> memdup_user() does.

There seemingly isn't. I'm not sure how common use case this is as devm is 
typically an init thing, not something called from fops, so it might not 
be worth adding devm_kmemdup_user() for this. But I haven't looked if 
there are other cases in the kernel sources that could use it (it's not
a one-liner with grep and both patterns are extremely frequent, maybe 
coccinelle could find the relevant ones easily if there are others in 
case you want to look).

> > And there's no error handling either?!
> 
> Whoops; will fix.
> 
> > 
> > >   	dev->policy_sz = length;
> > > +	kfree(new_policy_buf);
> > >   -	if (!amd_pmf_pb_valid(dev)) {
> > > -		ret = -EINVAL;
> > > -		goto cleanup;
> > > -	}
> > > +	if (!amd_pmf_pb_valid(dev))
> > 
> > Due to lack of error handling, this can deref NULL.
> > 
> > > +		return -EINVAL;
> > >     	amd_pmf_hex_dump_pb(dev);
> > >   	ret = amd_pmf_start_policy_engine(dev);
> > >   	if (ret < 0)
> > > -		goto cleanup;
> > > +		return ret;
> > >     	return length;
> > > -
> > > -cleanup:
> > > -	kfree(dev->policy_buf);
> > > -	dev->policy_buf = NULL;
> > > -	return ret;
> > >   }
> > >     static const struct file_operations pb_fops = {
> > > @@ -532,13 +526,13 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> > >   	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
> > >   	if (IS_ERR(dev->policy_base)) {
> > >   		ret = PTR_ERR(dev->policy_base);
> > > -		goto err_free_dram_buf;
> > > +		goto err_cancel_work;
> > >   	}
> > >   -	dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
> > > +	dev->policy_buf = devm_kzalloc(dev->dev, dev->policy_sz, GFP_KERNEL);
> > >   	if (!dev->policy_buf) {
> > >   		ret = -ENOMEM;
> > > -		goto err_free_dram_buf;
> > > +		goto err_cancel_work;
> > >   	}
> > >     	memcpy_fromio(dev->policy_buf, dev->policy_base,
> > > dev->policy_sz);
> > > @@ -546,21 +540,21 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> > >   	if (!amd_pmf_pb_valid(dev)) {
> > >   		dev_info(dev->dev, "No Smart PC policy present\n");
> > >   		ret = -EINVAL;
> > > -		goto err_free_policy;
> > > +		goto err_cancel_work;
> > >   	}
> > >     	amd_pmf_hex_dump_pb(dev);
> > >   -	dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
> > > +	dev->prev_data = devm_kzalloc(dev->dev, sizeof(*dev->prev_data),
> > > GFP_KERNEL);
> > >   	if (!dev->prev_data) {
> > >   		ret = -ENOMEM;
> > > -		goto err_free_policy;
> > > +		goto err_cancel_work;
> > >   	}
> > >     	for (i = 0; i < ARRAY_SIZE(amd_pmf_ta_uuid); i++) {
> > >   		ret = amd_pmf_tee_init(dev, &amd_pmf_ta_uuid[i]);
> > >   		if (ret)
> > > -			goto err_free_prev_data;
> > > +			goto err_cancel_work;
> > >     		ret = amd_pmf_start_policy_engine(dev);
> > >   		switch (ret) {
> > > @@ -575,7 +569,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> > >   		default:
> > >   			ret = -EINVAL;
> > >   			amd_pmf_tee_deinit(dev);
> > > -			goto err_free_prev_data;
> > > +			goto err_cancel_work;
> > >   		}
> > >     		if (status)
> > > @@ -584,7 +578,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> > >     	if (!status && !pb_side_load) {
> > >   		ret = -EINVAL;
> > > -		goto err_free_prev_data;
> > > +		goto err_cancel_work;
> > >   	}
> > >     	if (pb_side_load)
> > > @@ -600,12 +594,6 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> > >   	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);
> > >   @@ -621,11 +609,5 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev
> > > *dev)
> > >   		amd_pmf_remove_pb(dev);
> > >     	cancel_delayed_work_sync(&dev->pb_work);
> > > -	kfree(dev->prev_data);
> > > -	dev->prev_data = NULL;
> > > -	kfree(dev->policy_buf);
> > > -	dev->policy_buf = NULL;
> > > -	kfree(dev->buf);
> > > -	dev->buf = NULL;
> > >   	amd_pmf_tee_deinit(dev);
> > >   }
> > > 
> > 
> 

-- 
 i.

[PATCH v3 2/3] platform/x86/amd: pmf: Prevent amd_pmf_tee_deinit() from running twice

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

If any of the tee init fails, pass up the errors and clear the tee_ctx
pointer. This will prevent cleaning up multiple times.

Fixes: ac052d8c08f9d ("platform/x86/amd/pmf: Add PMF TEE interface")
Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmf/tee-if.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 513dbbe3f214..eb049e36ccb6 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -416,12 +416,12 @@ static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id, const uuid_
 	rc = tee_client_open_session(ctx, &sess_arg, NULL);
 	if (rc < 0 || sess_arg.ret != 0) {
 		pr_err("Failed to open TEE session err:%#x, rc:%d\n", sess_arg.ret, rc);
-		return rc;
+		return rc ?: -EINVAL;
 	}
 
 	*id = sess_arg.session;
 
-	return rc;
+	return 0;
 }
 
 static int amd_pmf_register_input_device(struct amd_pmf_dev *dev)
@@ -456,7 +456,9 @@ static int amd_pmf_tee_init(struct amd_pmf_dev *dev, const uuid_t *uuid)
 	dev->tee_ctx = tee_client_open_context(NULL, amd_pmf_amdtee_ta_match, NULL, NULL);
 	if (IS_ERR(dev->tee_ctx)) {
 		dev_err(dev->dev, "Failed to open TEE context\n");
-		return PTR_ERR(dev->tee_ctx);
+		ret = PTR_ERR(dev->tee_ctx);
+		dev->tee_ctx = NULL;
+		return ret;
 	}
 
 	ret = amd_pmf_ta_open_session(dev->tee_ctx, &dev->session_id, uuid);
@@ -496,9 +498,12 @@ static int amd_pmf_tee_init(struct amd_pmf_dev *dev, const uuid_t *uuid)
 
 static void amd_pmf_tee_deinit(struct amd_pmf_dev *dev)
 {
+	if (!dev->tee_ctx)
+		return;
 	tee_shm_free(dev->fw_shm_pool);
 	tee_client_close_session(dev->tee_ctx, dev->session_id);
 	tee_client_close_context(dev->tee_ctx);
+	dev->tee_ctx = NULL;
 }
 
 int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
-- 
2.43.0

[PATCH v3 3/3] platform/x86/amd: pmf: Simplify error flow in amd_pmf_init_smart_pc()

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

commit 5b1122fc4995f ("platform/x86/amd/pmf: fix cleanup in
amd_pmf_init_smart_pc()") adjusted the error handling flow to use a ladder
but this isn't actually needed because work is only scheduled in
amd_pmf_start_policy_engine() and with device managed cleanups pointers
for allocations don't need to be freed.

Adjust the error flow to a single call to amd_pmf_deinit_smart_pc() for
the cases that need to clean up.

Cc: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmf/tee-if.c | 55 ++++++++-------------------
 1 file changed, 16 insertions(+), 39 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index eb049e36ccb6..7f80f70bcc5f 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -526,64 +526,45 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 
 	ret = amd_pmf_set_dram_addr(dev, true);
 	if (ret)
-		goto err_cancel_work;
+		return ret;
 
 	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
-	if (IS_ERR(dev->policy_base)) {
-		ret = PTR_ERR(dev->policy_base);
-		goto err_cancel_work;
-	}
+	if (IS_ERR(dev->policy_base))
+		return PTR_ERR(dev->policy_base);
 
 	dev->policy_buf = devm_kzalloc(dev->dev, dev->policy_sz, GFP_KERNEL);
-	if (!dev->policy_buf) {
-		ret = -ENOMEM;
-		goto err_cancel_work;
-	}
+	if (!dev->policy_buf)
+		return -ENOMEM;
 
 	memcpy_fromio(dev->policy_buf, dev->policy_base, dev->policy_sz);
 
 	if (!amd_pmf_pb_valid(dev)) {
 		dev_info(dev->dev, "No Smart PC policy present\n");
-		ret = -EINVAL;
-		goto err_cancel_work;
+		return -EINVAL;
 	}
 
 	amd_pmf_hex_dump_pb(dev);
 
 	dev->prev_data = devm_kzalloc(dev->dev, sizeof(*dev->prev_data), GFP_KERNEL);
-	if (!dev->prev_data) {
-		ret = -ENOMEM;
-		goto err_cancel_work;
-	}
+	if (!dev->prev_data)
+		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(amd_pmf_ta_uuid); i++) {
 		ret = amd_pmf_tee_init(dev, &amd_pmf_ta_uuid[i]);
 		if (ret)
-			goto err_cancel_work;
+			return ret;
 
 		ret = amd_pmf_start_policy_engine(dev);
-		switch (ret) {
-		case TA_PMF_TYPE_SUCCESS:
-			status = true;
-			break;
-		case TA_ERROR_CRYPTO_INVALID_PARAM:
-		case TA_ERROR_CRYPTO_BIN_TOO_LARGE:
-			amd_pmf_tee_deinit(dev);
-			status = false;
-			break;
-		default:
-			ret = -EINVAL;
-			amd_pmf_tee_deinit(dev);
-			goto err_cancel_work;
-		}
-
+		dev_dbg(dev->dev, "start policy engine ret: %d\n", ret);
+		status = ret == TA_PMF_TYPE_SUCCESS;
 		if (status)
 			break;
+		amd_pmf_tee_deinit(dev);
 	}
 
 	if (!status && !pb_side_load) {
 		ret = -EINVAL;
-		goto err_cancel_work;
+		goto err;
 	}
 
 	if (pb_side_load)
@@ -591,16 +572,12 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 
 	ret = amd_pmf_register_input_device(dev);
 	if (ret)
-		goto err_pmf_remove_pb;
+		goto err;
 
 	return 0;
 
-err_pmf_remove_pb:
-	if (pb_side_load && dev->esbin)
-		amd_pmf_remove_pb(dev);
-	amd_pmf_tee_deinit(dev);
-err_cancel_work:
-	cancel_delayed_work_sync(&dev->pb_work);
+err:
+	amd_pmf_deinit_smart_pc(dev);
 
 	return ret;
 }
-- 
2.43.0