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

[PATCH v4 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.

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 | 110 +++++++++-----------------
 2 files changed, 37 insertions(+), 76 deletions(-)

-- 
2.43.0

[PATCH v4 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()")
Link: https://lore.kernel.org/r/20250512211154.2510397-2-superm1@kernel.org
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v4:
 * Handle failures from memory allocation on sideload (Ilpo)
 * Allocate memory before copying from user (Ilpo)
---
 drivers/platform/x86/amd/pmf/core.c   |  3 +-
 drivers/platform/x86/amd/pmf/tee-if.c | 58 +++++++++------------------
 2 files changed, 20 insertions(+), 41 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index 96821101ec773..395c011e837f1 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 d3bd12ad036ae..6d85601812225 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -350,38 +350,30 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
 				   size_t length, loff_t *pos)
 {
 	struct amd_pmf_dev *dev = filp->private_data;
-	unsigned char *new_policy_buf;
 	int ret;
 
 	/* Policy binary size cannot exceed POLICY_BUF_MAX_SZ */
 	if (length > POLICY_BUF_MAX_SZ || length == 0)
 		return -EINVAL;
 
-	/* re-alloc to the new buffer length of the policy binary */
-	new_policy_buf = memdup_user(buf, length);
-	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_kzalloc(dev->dev, length, GFP_KERNEL);
+	if (IS_ERR(dev->policy_buf))
+		return -ENOMEM;
 	dev->policy_sz = length;
 
-	if (!amd_pmf_pb_valid(dev)) {
-		ret = -EINVAL;
-		goto cleanup;
-	}
+	if (copy_from_user(dev->policy_buf, buf, length))
+		return -EFAULT;
+
+	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 +524,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 +538,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 +567,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 +576,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 +592,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 +607,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

[PATCH v4 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>
Link: https://lore.kernel.org/r/20250512211154.2510397-3-superm1@kernel.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 6d85601812225..1b268fffe96e2 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -414,12 +414,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)
@@ -454,7 +454,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);
@@ -494,9 +496,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 v4 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>
Link: https://lore.kernel.org/r/20250512211154.2510397-4-superm1@kernel.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 1b268fffe96e2..1363d03c457ca 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -524,64 +524,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)
@@ -589,16 +570,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

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

[Ilpo Järvinen]

On Thu, 15 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()")
> Link: https://lore.kernel.org/r/20250512211154.2510397-2-superm1@kernel.org
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v4:
>  * Handle failures from memory allocation on sideload (Ilpo)
>  * Allocate memory before copying from user (Ilpo)
> ---
>  drivers/platform/x86/amd/pmf/core.c   |  3 +-
>  drivers/platform/x86/amd/pmf/tee-if.c | 58 +++++++++------------------
>  2 files changed, 20 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 96821101ec773..395c011e837f1 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 d3bd12ad036ae..6d85601812225 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -350,38 +350,30 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
>  				   size_t length, loff_t *pos)
>  {
>  	struct amd_pmf_dev *dev = filp->private_data;
> -	unsigned char *new_policy_buf;
>  	int ret;
>  
>  	/* Policy binary size cannot exceed POLICY_BUF_MAX_SZ */
>  	if (length > POLICY_BUF_MAX_SZ || length == 0)
>  		return -EINVAL;
>  
> -	/* re-alloc to the new buffer length of the policy binary */
> -	new_policy_buf = memdup_user(buf, length);
> -	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_kzalloc(dev->dev, length, GFP_KERNEL);
> +	if (IS_ERR(dev->policy_buf))
> +		return -ENOMEM;
>  	dev->policy_sz = length;
>  
> -	if (!amd_pmf_pb_valid(dev)) {
> -		ret = -EINVAL;
> -		goto cleanup;
> -	}
> +	if (copy_from_user(dev->policy_buf, buf, length))
> +		return -EFAULT;

Previously, if anything failed here, the old buffer was left in place.
I always assumed it was intentional. But after your change, first thing 
that happens is freeing the old policy_buf.

We're long past the point where I've started to lose confidence in this 
patch :-(. Could we like just make the minimal changes here to convert 
into devm_*() and nothing more? If you want to make any other changes, be 
it reordering logic, removal of the local variable, or whatever, please 
put those into own patch(es) and properly justify them.

-- 
 i.

> +
> +	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 +524,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 +538,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 +567,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 +576,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 +592,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 +607,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 v4 1/3] platform/x86/amd: pmf: Use device managed allocations

[Mario Limonciello]

On 5/16/25 01:04, Ilpo Järvinen wrote:
> On Thu, 15 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()")
>> Link: https://lore.kernel.org/r/20250512211154.2510397-2-superm1@kernel.org
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v4:
>>   * Handle failures from memory allocation on sideload (Ilpo)
>>   * Allocate memory before copying from user (Ilpo)
>> ---
>>   drivers/platform/x86/amd/pmf/core.c   |  3 +-
>>   drivers/platform/x86/amd/pmf/tee-if.c | 58 +++++++++------------------
>>   2 files changed, 20 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
>> index 96821101ec773..395c011e837f1 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 d3bd12ad036ae..6d85601812225 100644
>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>> @@ -350,38 +350,30 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
>>   				   size_t length, loff_t *pos)
>>   {
>>   	struct amd_pmf_dev *dev = filp->private_data;
>> -	unsigned char *new_policy_buf;
>>   	int ret;
>>   
>>   	/* Policy binary size cannot exceed POLICY_BUF_MAX_SZ */
>>   	if (length > POLICY_BUF_MAX_SZ || length == 0)
>>   		return -EINVAL;
>>   
>> -	/* re-alloc to the new buffer length of the policy binary */
>> -	new_policy_buf = memdup_user(buf, length);
>> -	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_kzalloc(dev->dev, length, GFP_KERNEL);
>> +	if (IS_ERR(dev->policy_buf))
>> +		return -ENOMEM;
>>   	dev->policy_sz = length;
>>   
>> -	if (!amd_pmf_pb_valid(dev)) {
>> -		ret = -EINVAL;
>> -		goto cleanup;
>> -	}
>> +	if (copy_from_user(dev->policy_buf, buf, length))
>> +		return -EFAULT;
> 
> Previously, if anything failed here, the old buffer was left in place.
> I always assumed it was intentional. But after your change, first thing
> that happens is freeing the old policy_buf.

Yeah; in order to do devm without a double malloc it needs to be cleared 
immediately.  But this is a debugfs sideloading interface.  If you send 
a bad binary you can just try again with a good one.

> 
> We're long past the point where I've started to lose confidence in this
> patch :-(. Could we like just make the minimal changes here to convert
> into devm_*() and nothing more? If you want to make any other changes, be
> it reordering logic, removal of the local variable, or whatever, please
> put those into own patch(es) and properly justify them.
> 

If we're aiming for a total minimal patch that just fixes the most 
immediate issue that's v1 of this series [1].

Through the course of the discussion obviously there were more things 
raised by Dan, and I feel that v4 is more robust.

Maybe the right answer is to just pick up v1 for 6.15-rc, and this 
series for 6.16?  Or if you want this to have more time in -next I can 
just resubmit it after 6.16-rc1 and we aim for 6.17 with it.

[1] 
https://lore.kernel.org/platform-driver-x86/20250506131130.1446262-1-superm1@kernel.org/#t

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

[Ilpo Järvinen]

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

On Fri, 16 May 2025, Mario Limonciello wrote:
> On 5/16/25 01:04, Ilpo Järvinen wrote:
> > On Thu, 15 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()")
> > > Link:
> > > https://lore.kernel.org/r/20250512211154.2510397-2-superm1@kernel.org
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > > v4:
> > >   * Handle failures from memory allocation on sideload (Ilpo)
> > >   * Allocate memory before copying from user (Ilpo)
> > > ---
> > >   drivers/platform/x86/amd/pmf/core.c   |  3 +-
> > >   drivers/platform/x86/amd/pmf/tee-if.c | 58 +++++++++------------------
> > >   2 files changed, 20 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/amd/pmf/core.c
> > > b/drivers/platform/x86/amd/pmf/core.c
> > > index 96821101ec773..395c011e837f1 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 d3bd12ad036ae..6d85601812225 100644
> > > --- a/drivers/platform/x86/amd/pmf/tee-if.c
> > > +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> > > @@ -350,38 +350,30 @@ static ssize_t amd_pmf_get_pb_data(struct file
> > > *filp, const char __user *buf,
> > >   				   size_t length, loff_t *pos)
> > >   {
> > >   	struct amd_pmf_dev *dev = filp->private_data;
> > > -	unsigned char *new_policy_buf;
> > >   	int ret;
> > >     	/* Policy binary size cannot exceed POLICY_BUF_MAX_SZ */
> > >   	if (length > POLICY_BUF_MAX_SZ || length == 0)
> > >   		return -EINVAL;
> > >   -	/* re-alloc to the new buffer length of the policy binary */
> > > -	new_policy_buf = memdup_user(buf, length);
> > > -	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_kzalloc(dev->dev, length, GFP_KERNEL);
> > > +	if (IS_ERR(dev->policy_buf))
> > > +		return -ENOMEM;
> > >   	dev->policy_sz = length;
> > >   -	if (!amd_pmf_pb_valid(dev)) {
> > > -		ret = -EINVAL;
> > > -		goto cleanup;
> > > -	}
> > > +	if (copy_from_user(dev->policy_buf, buf, length))
> > > +		return -EFAULT;
> > 
> > Previously, if anything failed here, the old buffer was left in place.
> > I always assumed it was intentional. But after your change, first thing
> > that happens is freeing the old policy_buf.
> 
> Yeah; in order to do devm without a double malloc it needs to be cleared
> immediately.

I'm feeling like I must be missing something here, but I just fail to see 
why the order _has to be_ changed when changing kfree() -> devm_kfree().

> But this is a debugfs sideloading interface.  If you send a bad
> binary you can just try again with a good one.
> 
> > 
> > We're long past the point where I've started to lose confidence in this
> > patch :-(. Could we like just make the minimal changes here to convert
> > into devm_*() and nothing more? If you want to make any other changes, be
> > it reordering logic, removal of the local variable, or whatever, please
> > put those into own patch(es) and properly justify them.
> > 
> 
> If we're aiming for a total minimal patch that just fixes the most immediate
> issue that's v1 of this series [1].

As spelled out very clearly in the above comment, I'm aiming to a patch 
which converts this to devm_*() without other changes. If you want to do 
other changes, they should be in their own patch.

> Through the course of the discussion obviously there were more things raised
> by Dan, and I feel that v4 is more robust.
> 
> Maybe the right answer is to just pick up v1 for 6.15-rc, and this series for
> 6.16?  Or if you want this to have more time in -next I can just resubmit it
> after 6.16-rc1 and we aim for 6.17 with it.
> 
> [1]
> https://lore.kernel.org/platform-driver-x86/20250506131130.1446262-1-superm1@kernel.org/#t
> 

-- 
 i.

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

[Mario Limonciello]

On 5/16/2025 1:36 AM, Ilpo Järvinen wrote:
> On Fri, 16 May 2025, Mario Limonciello wrote:
>> On 5/16/25 01:04, Ilpo Järvinen wrote:
>>> On Thu, 15 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()")
>>>> Link:
>>>> https://lore.kernel.org/r/20250512211154.2510397-2-superm1@kernel.org
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>> v4:
>>>>    * Handle failures from memory allocation on sideload (Ilpo)
>>>>    * Allocate memory before copying from user (Ilpo)
>>>> ---
>>>>    drivers/platform/x86/amd/pmf/core.c   |  3 +-
>>>>    drivers/platform/x86/amd/pmf/tee-if.c | 58 +++++++++------------------
>>>>    2 files changed, 20 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/core.c
>>>> b/drivers/platform/x86/amd/pmf/core.c
>>>> index 96821101ec773..395c011e837f1 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 d3bd12ad036ae..6d85601812225 100644
>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>>>> @@ -350,38 +350,30 @@ static ssize_t amd_pmf_get_pb_data(struct file
>>>> *filp, const char __user *buf,
>>>>    				   size_t length, loff_t *pos)
>>>>    {
>>>>    	struct amd_pmf_dev *dev = filp->private_data;
>>>> -	unsigned char *new_policy_buf;
>>>>    	int ret;
>>>>      	/* Policy binary size cannot exceed POLICY_BUF_MAX_SZ */
>>>>    	if (length > POLICY_BUF_MAX_SZ || length == 0)
>>>>    		return -EINVAL;
>>>>    -	/* re-alloc to the new buffer length of the policy binary */
>>>> -	new_policy_buf = memdup_user(buf, length);
>>>> -	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_kzalloc(dev->dev, length, GFP_KERNEL);
>>>> +	if (IS_ERR(dev->policy_buf))
>>>> +		return -ENOMEM;
>>>>    	dev->policy_sz = length;
>>>>    -	if (!amd_pmf_pb_valid(dev)) {
>>>> -		ret = -EINVAL;
>>>> -		goto cleanup;
>>>> -	}
>>>> +	if (copy_from_user(dev->policy_buf, buf, length))
>>>> +		return -EFAULT;
>>>
>>> Previously, if anything failed here, the old buffer was left in place.
>>> I always assumed it was intentional. But after your change, first thing
>>> that happens is freeing the old policy_buf.
>>
>> Yeah; in order to do devm without a double malloc it needs to be cleared
>> immediately.
> 
> I'm feeling like I must be missing something here, but I just fail to see
> why the order _has to be_ changed when changing kfree() -> devm_kfree().

Because the policy is coming in from userspace and you need to have 
somewhere that is the right size to copy it to.

As you mentioned wanting to remove the double malloc in the earlier 
version this is the way to do it.

IE when using copy_from_user() instead of memdup_user() the memory must 
"already" be allocated.  That's what is done now with devm_kzalloc().

Are you suggesting some sort of way to keep it in the same order and 
subvert device managed allocations and "steal" the pointer?  Glib has a 
concept like this, but I wasn't aware of a way to do in the kernel.

> 
>> But this is a debugfs sideloading interface.  If you send a bad
>> binary you can just try again with a good one.
>>
>>>
>>> We're long past the point where I've started to lose confidence in this
>>> patch :-(. Could we like just make the minimal changes here to convert
>>> into devm_*() and nothing more? If you want to make any other changes, be
>>> it reordering logic, removal of the local variable, or whatever, please
>>> put those into own patch(es) and properly justify them.
>>>
>>
>> If we're aiming for a total minimal patch that just fixes the most immediate
>> issue that's v1 of this series [1].
> 
> As spelled out very clearly in the above comment, I'm aiming to a patch
> which converts this to devm_*() without other changes. If you want to do
> other changes, they should be in their own patch.

I guess if there's a way to do this without changing the order I will do 
it, but I don't see one RN.

> 
>> Through the course of the discussion obviously there were more things raised
>> by Dan, and I feel that v4 is more robust.
>>
>> Maybe the right answer is to just pick up v1 for 6.15-rc, and this series for
>> 6.16?  Or if you want this to have more time in -next I can just resubmit it
>> after 6.16-rc1 and we aim for 6.17 with it.
>>
>> [1]
>> https://lore.kernel.org/platform-driver-x86/20250506131130.1446262-1-superm1@kernel.org/#t
>>
> 

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

[Ilpo Järvinen]

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

On Fri, 16 May 2025, Mario Limonciello wrote:

> On 5/16/2025 1:36 AM, Ilpo Järvinen wrote:
> > On Fri, 16 May 2025, Mario Limonciello wrote:
> > > On 5/16/25 01:04, Ilpo Järvinen wrote:
> > > > On Thu, 15 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()")
> > > > > Link:
> > > > > https://lore.kernel.org/r/20250512211154.2510397-2-superm1@kernel.org
> > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > > > ---
> > > > > v4:
> > > > >    * Handle failures from memory allocation on sideload (Ilpo)
> > > > >    * Allocate memory before copying from user (Ilpo)
> > > > > ---
> > > > >    drivers/platform/x86/amd/pmf/core.c   |  3 +-
> > > > >    drivers/platform/x86/amd/pmf/tee-if.c | 58
> > > > > +++++++++------------------
> > > > >    2 files changed, 20 insertions(+), 41 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/platform/x86/amd/pmf/core.c
> > > > > b/drivers/platform/x86/amd/pmf/core.c
> > > > > index 96821101ec773..395c011e837f1 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 d3bd12ad036ae..6d85601812225 100644
> > > > > --- a/drivers/platform/x86/amd/pmf/tee-if.c
> > > > > +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> > > > > @@ -350,38 +350,30 @@ static ssize_t amd_pmf_get_pb_data(struct file
> > > > > *filp, const char __user *buf,
> > > > >    				   size_t length, loff_t *pos)
> > > > >    {
> > > > >    	struct amd_pmf_dev *dev = filp->private_data;
> > > > > -	unsigned char *new_policy_buf;
> > > > >    	int ret;
> > > > >      	/* Policy binary size cannot exceed POLICY_BUF_MAX_SZ */
> > > > >    	if (length > POLICY_BUF_MAX_SZ || length == 0)
> > > > >    		return -EINVAL;
> > > > >    -	/* re-alloc to the new buffer length of the policy binary */
> > > > > -	new_policy_buf = memdup_user(buf, length);
> > > > > -	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_kzalloc(dev->dev, length, GFP_KERNEL);
> > > > > +	if (IS_ERR(dev->policy_buf))
> > > > > +		return -ENOMEM;
> > > > >    	dev->policy_sz = length;
> > > > >    -	if (!amd_pmf_pb_valid(dev)) {
> > > > > -		ret = -EINVAL;
> > > > > -		goto cleanup;
> > > > > -	}
> > > > > +	if (copy_from_user(dev->policy_buf, buf, length))
> > > > > +		return -EFAULT;
> > > > 
> > > > Previously, if anything failed here, the old buffer was left in place.
> > > > I always assumed it was intentional. But after your change, first thing
> > > > that happens is freeing the old policy_buf.
> > > 
> > > Yeah; in order to do devm without a double malloc it needs to be cleared
> > > immediately.
> > 
> > I'm feeling like I must be missing something here, but I just fail to see
> > why the order _has to be_ changed when changing kfree() -> devm_kfree().
> 
> Because the policy is coming in from userspace and you need to have somewhere
> that is the right size to copy it to.
> 
> As you mentioned wanting to remove the double malloc in the earlier version
> this is the way to do it.
> 
> IE when using copy_from_user() instead of memdup_user() the memory must
> "already" be allocated.  That's what is done now with devm_kzalloc().

Hi Mario,

Why can't you do:

	/* re-alloc to the new buffer length of the policy binary */
	new_policy_buf = devm_kzalloc(dev->dev, length, GFP_KERNEL);
	if (!new_policy_buf)
		return -ENOMEM;

	if (copy_from_user(new_policy_buf, buf, length)) {
		devm_kfree(new_policy_buf);
		return -EFAULT;
	}

	devm_kfree(dev->policy_buf);
	ev->policy_buf = new_policy_buf;

?

That follows pretty much what the old code did with new_policy_buf 
variable, but allocation related calls are now devm_*().

> Are you suggesting some sort of way to keep it in the same order and subvert
> device managed allocations and "steal" the pointer?  Glib has a concept like
> this, but I wasn't aware of a way to do in the kernel.
> 
> > 
> > > But this is a debugfs sideloading interface.  If you send a bad
> > > binary you can just try again with a good one.
> > > 
> > > > 
> > > > We're long past the point where I've started to lose confidence in this
> > > > patch :-(. Could we like just make the minimal changes here to convert
> > > > into devm_*() and nothing more? If you want to make any other changes,
> > > > be
> > > > it reordering logic, removal of the local variable, or whatever, please
> > > > put those into own patch(es) and properly justify them.
> > > > 
> > > 
> > > If we're aiming for a total minimal patch that just fixes the most
> > > immediate
> > > issue that's v1 of this series [1].
> > 
> > As spelled out very clearly in the above comment, I'm aiming to a patch
> > which converts this to devm_*() without other changes. If you want to do
> > other changes, they should be in their own patch.
> 
> I guess if there's a way to do this without changing the order I will do it,
> but I don't see one RN.


-- 
 i.

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

[Mario Limonciello]

On 5/20/2025 10:19 AM, Ilpo Järvinen wrote:
> On Fri, 16 May 2025, Mario Limonciello wrote:
> 
>> On 5/16/2025 1:36 AM, Ilpo Järvinen wrote:
>>> On Fri, 16 May 2025, Mario Limonciello wrote:
>>>> On 5/16/25 01:04, Ilpo Järvinen wrote:
>>>>> On Thu, 15 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()")
>>>>>> Link:
>>>>>> https://lore.kernel.org/r/20250512211154.2510397-2-superm1@kernel.org
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>> ---
>>>>>> v4:
>>>>>>     * Handle failures from memory allocation on sideload (Ilpo)
>>>>>>     * Allocate memory before copying from user (Ilpo)
>>>>>> ---
>>>>>>     drivers/platform/x86/amd/pmf/core.c   |  3 +-
>>>>>>     drivers/platform/x86/amd/pmf/tee-if.c | 58
>>>>>> +++++++++------------------
>>>>>>     2 files changed, 20 insertions(+), 41 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/platform/x86/amd/pmf/core.c
>>>>>> b/drivers/platform/x86/amd/pmf/core.c
>>>>>> index 96821101ec773..395c011e837f1 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 d3bd12ad036ae..6d85601812225 100644
>>>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>>>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>>>>>> @@ -350,38 +350,30 @@ static ssize_t amd_pmf_get_pb_data(struct file
>>>>>> *filp, const char __user *buf,
>>>>>>     				   size_t length, loff_t *pos)
>>>>>>     {
>>>>>>     	struct amd_pmf_dev *dev = filp->private_data;
>>>>>> -	unsigned char *new_policy_buf;
>>>>>>     	int ret;
>>>>>>       	/* Policy binary size cannot exceed POLICY_BUF_MAX_SZ */
>>>>>>     	if (length > POLICY_BUF_MAX_SZ || length == 0)
>>>>>>     		return -EINVAL;
>>>>>>     -	/* re-alloc to the new buffer length of the policy binary */
>>>>>> -	new_policy_buf = memdup_user(buf, length);
>>>>>> -	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_kzalloc(dev->dev, length, GFP_KERNEL);
>>>>>> +	if (IS_ERR(dev->policy_buf))
>>>>>> +		return -ENOMEM;
>>>>>>     	dev->policy_sz = length;
>>>>>>     -	if (!amd_pmf_pb_valid(dev)) {
>>>>>> -		ret = -EINVAL;
>>>>>> -		goto cleanup;
>>>>>> -	}
>>>>>> +	if (copy_from_user(dev->policy_buf, buf, length))
>>>>>> +		return -EFAULT;
>>>>>
>>>>> Previously, if anything failed here, the old buffer was left in place.
>>>>> I always assumed it was intentional. But after your change, first thing
>>>>> that happens is freeing the old policy_buf.
>>>>
>>>> Yeah; in order to do devm without a double malloc it needs to be cleared
>>>> immediately.
>>>
>>> I'm feeling like I must be missing something here, but I just fail to see
>>> why the order _has to be_ changed when changing kfree() -> devm_kfree().
>>
>> Because the policy is coming in from userspace and you need to have somewhere
>> that is the right size to copy it to.
>>
>> As you mentioned wanting to remove the double malloc in the earlier version
>> this is the way to do it.
>>
>> IE when using copy_from_user() instead of memdup_user() the memory must
>> "already" be allocated.  That's what is done now with devm_kzalloc().
> 
> Hi Mario,
> 
> Why can't you do:
> 
> 	/* re-alloc to the new buffer length of the policy binary */
> 	new_policy_buf = devm_kzalloc(dev->dev, length, GFP_KERNEL);
> 	if (!new_policy_buf)
> 		return -ENOMEM;
> 
> 	if (copy_from_user(new_policy_buf, buf, length)) {
> 		devm_kfree(new_policy_buf);
> 		return -EFAULT;
> 	}
> 
> 	devm_kfree(dev->policy_buf);
> 	ev->policy_buf = new_policy_buf;
> 
> ?
> 
> That follows pretty much what the old code did with new_policy_buf
> variable, but allocation related calls are now devm_*().

Thanks for the suggestion!  I hadn't thought about using a second 
pointer like that.

Technically the devm_kfree(new_policy_buf) under the copy_from_user() 
failure shouldn't be needed though, right?  Because it will still be 
freed when the device is freed.

> 
>> Are you suggesting some sort of way to keep it in the same order and subvert
>> device managed allocations and "steal" the pointer?  Glib has a concept like
>> this, but I wasn't aware of a way to do in the kernel.
>>
>>>
>>>> But this is a debugfs sideloading interface.  If you send a bad
>>>> binary you can just try again with a good one.
>>>>
>>>>>
>>>>> We're long past the point where I've started to lose confidence in this
>>>>> patch :-(. Could we like just make the minimal changes here to convert
>>>>> into devm_*() and nothing more? If you want to make any other changes,
>>>>> be
>>>>> it reordering logic, removal of the local variable, or whatever, please
>>>>> put those into own patch(es) and properly justify them.
>>>>>
>>>>
>>>> If we're aiming for a total minimal patch that just fixes the most
>>>> immediate
>>>> issue that's v1 of this series [1].
>>>
>>> As spelled out very clearly in the above comment, I'm aiming to a patch
>>> which converts this to devm_*() without other changes. If you want to do
>>> other changes, they should be in their own patch.
>>
>> I guess if there's a way to do this without changing the order I will do it,
>> but I don't see one RN.
> 
> 

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

[Ilpo Järvinen]

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

On Tue, 20 May 2025, Mario Limonciello wrote:

> On 5/20/2025 10:19 AM, Ilpo Järvinen wrote:
> > On Fri, 16 May 2025, Mario Limonciello wrote:
> > 
> > > On 5/16/2025 1:36 AM, Ilpo Järvinen wrote:
> > > > On Fri, 16 May 2025, Mario Limonciello wrote:
> > > > > On 5/16/25 01:04, Ilpo Järvinen wrote:
> > > > > > On Thu, 15 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()")
> > > > > > > Link:
> > > > > > > https://lore.kernel.org/r/20250512211154.2510397-2-superm1@kernel.org
> > > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > > > > > ---
> > > > > > > v4:
> > > > > > >     * Handle failures from memory allocation on sideload (Ilpo)
> > > > > > >     * Allocate memory before copying from user (Ilpo)
> > > > > > > ---
> > > > > > >     drivers/platform/x86/amd/pmf/core.c   |  3 +-
> > > > > > >     drivers/platform/x86/amd/pmf/tee-if.c | 58
> > > > > > > +++++++++------------------
> > > > > > >     2 files changed, 20 insertions(+), 41 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/platform/x86/amd/pmf/core.c
> > > > > > > b/drivers/platform/x86/amd/pmf/core.c
> > > > > > > index 96821101ec773..395c011e837f1 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 d3bd12ad036ae..6d85601812225 100644
> > > > > > > --- a/drivers/platform/x86/amd/pmf/tee-if.c
> > > > > > > +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> > > > > > > @@ -350,38 +350,30 @@ static ssize_t amd_pmf_get_pb_data(struct
> > > > > > > file
> > > > > > > *filp, const char __user *buf,
> > > > > > >     				   size_t length, loff_t *pos)
> > > > > > >     {
> > > > > > >     	struct amd_pmf_dev *dev = filp->private_data;
> > > > > > > -	unsigned char *new_policy_buf;
> > > > > > >     	int ret;
> > > > > > >       	/* Policy binary size cannot exceed POLICY_BUF_MAX_SZ
> > > > > > > */
> > > > > > >     	if (length > POLICY_BUF_MAX_SZ || length == 0)
> > > > > > >     		return -EINVAL;
> > > > > > >     -	/* re-alloc to the new buffer length of the policy
> > > > > > > binary */
> > > > > > > -	new_policy_buf = memdup_user(buf, length);
> > > > > > > -	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_kzalloc(dev->dev, length, GFP_KERNEL);
> > > > > > > +	if (IS_ERR(dev->policy_buf))
> > > > > > > +		return -ENOMEM;
> > > > > > >     	dev->policy_sz = length;
> > > > > > >     -	if (!amd_pmf_pb_valid(dev)) {
> > > > > > > -		ret = -EINVAL;
> > > > > > > -		goto cleanup;
> > > > > > > -	}
> > > > > > > +	if (copy_from_user(dev->policy_buf, buf, length))
> > > > > > > +		return -EFAULT;
> > > > > > 
> > > > > > Previously, if anything failed here, the old buffer was left in
> > > > > > place.
> > > > > > I always assumed it was intentional. But after your change, first
> > > > > > thing
> > > > > > that happens is freeing the old policy_buf.
> > > > > 
> > > > > Yeah; in order to do devm without a double malloc it needs to be
> > > > > cleared
> > > > > immediately.
> > > > 
> > > > I'm feeling like I must be missing something here, but I just fail to
> > > > see
> > > > why the order _has to be_ changed when changing kfree() -> devm_kfree().
> > > 
> > > Because the policy is coming in from userspace and you need to have
> > > somewhere
> > > that is the right size to copy it to.
> > > 
> > > As you mentioned wanting to remove the double malloc in the earlier
> > > version
> > > this is the way to do it.
> > > 
> > > IE when using copy_from_user() instead of memdup_user() the memory must
> > > "already" be allocated.  That's what is done now with devm_kzalloc().
> > 
> > Hi Mario,
> > 
> > Why can't you do:
> > 
> > 	/* re-alloc to the new buffer length of the policy binary */
> > 	new_policy_buf = devm_kzalloc(dev->dev, length, GFP_KERNEL);
> > 	if (!new_policy_buf)
> > 		return -ENOMEM;
> > 
> > 	if (copy_from_user(new_policy_buf, buf, length)) {
> > 		devm_kfree(new_policy_buf);
> > 		return -EFAULT;
> > 	}
> > 
> > 	devm_kfree(dev->policy_buf);

I seem to have forgotten dev->dev from these devm_kfree() args.

> > 	ev->policy_buf = new_policy_buf;
> > 
> > ?
> > 
> > That follows pretty much what the old code did with new_policy_buf
> > variable, but allocation related calls are now devm_*().
> 
> Thanks for the suggestion!  I hadn't thought about using a second pointer like
> that.
>
> Technically the devm_kfree(new_policy_buf) under the copy_from_user() failure
> shouldn't be needed though, right?  Because it will still be freed when the
> device is freed.

Yes, but that would mean that memory is retained unused until the device 
is freed which doesn't sound the best idea. Given this can fail multiple 
times too, it's sort of memleak like behavior even if that memory is 
accounted for in the very end thanks to graciousness of devm.

> > > Are you suggesting some sort of way to keep it in the same order and
> > > subvert
> > > device managed allocations and "steal" the pointer?  Glib has a concept
> > > like
> > > this, but I wasn't aware of a way to do in the kernel.
> > > 
> > > > 
> > > > > But this is a debugfs sideloading interface.  If you send a bad
> > > > > binary you can just try again with a good one.
> > > > > 
> > > > > > 
> > > > > > We're long past the point where I've started to lose confidence in
> > > > > > this
> > > > > > patch :-(. Could we like just make the minimal changes here to
> > > > > > convert
> > > > > > into devm_*() and nothing more? If you want to make any other
> > > > > > changes,
> > > > > > be
> > > > > > it reordering logic, removal of the local variable, or whatever,
> > > > > > please
> > > > > > put those into own patch(es) and properly justify them.
> > > > > > 
> > > > > 
> > > > > If we're aiming for a total minimal patch that just fixes the most
> > > > > immediate
> > > > > issue that's v1 of this series [1].
> > > > 
> > > > As spelled out very clearly in the above comment, I'm aiming to a patch
> > > > which converts this to devm_*() without other changes. If you want to do
> > > > other changes, they should be in their own patch.
> > > 
> > > I guess if there's a way to do this without changing the order I will do
> > > it,
> > > but I don't see one RN.
> > 
> > 
> 

-- 
 i.