[PATCH 0/2] Handle bad policies for AMD-PMF better

[PATCH 0/2] Handle bad policies for AMD-PMF better

[Mario Limonciello]

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

There are assumptions in AMD-PMF code regarding policies being
valid both from firmware and from sideloading.  This series adds an
extra test to ensure policies aren't empty and also handles a memory
leak in the cleanup path for the sideloaded policies.

Mario Limonciello (2):
  drivers/platform/x86/amd: pmf: Check for invalid Smart PC Policies
  drivers/platform/x86/amd: pmf: Handle bad policies in
    amd_pmf_get_pb_data()

 drivers/platform/x86/amd/pmf/tee-if.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/2] drivers/platform/x86/amd: pmf: Check for invalid Smart PC Policies

[Mario Limonciello]

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

commit 376a8c2a14439 ("platform/x86/amd/pmf: Update PMF Driver for
Compatibility with new PMF-TA") added support for platforms that support
an updated TA, however it also exposed a number of platforms that although
they have support for the updated TA don't actually populate a policy
binary.

Add an explicit check that the policy binary isn't empty before
initializing the TA.

Reported-by: Christian Heusel <christian@heusel.eu>
Closes: https://lore.kernel.org/platform-driver-x86/ae644428-5bf2-4b30-81ba-0b259ed3449b@heusel.eu/
Fixes: 376a8c2a14439 ("platform/x86/amd/pmf: Update PMF Driver for Compatibility with new PMF-TA")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmf/tee-if.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 4c49b510ad1a7..41ab9eca5ff13 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -544,6 +544,13 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 
 	memcpy_fromio(dev->policy_buf, dev->policy_base, dev->policy_sz);
 
+	/* check if the whole policy is empty */
+	if (!memchr_inv(dev->policy_buf, 0xff, dev->policy_sz)) {
+		dev_info(dev->dev, "No Smart PC policy present\n");
+		ret = -EINVAL;
+		goto err_free_policy;
+	}
+
 	amd_pmf_hex_dump_pb(dev);
 
 	dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
-- 
2.43.0

[PATCH 2/2] drivers/platform/x86/amd: pmf: Handle bad policies in amd_pmf_get_pb_data()

[Mario Limonciello]

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

If a policy is passed into amd_pmf_get_pb_data() that causes the engine
to fail to start there is a memory leak. Check for invalid policies as
well as an error in amd_pmf_start_policy_engine() and free the memory in
the failure path.

Fixes: 10817f28e5337 ("platform/x86/amd/pmf: Add capability to sideload of policy binary")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmf/tee-if.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 41ab9eca5ff13..3c399dc1bfcca 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -377,12 +377,23 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
 	dev->policy_buf = new_policy_buf;
 	dev->policy_sz = length;
 
+	/* Check if the policy binary is valid */
+	if (!memchr_inv(dev->policy_buf, 0xff, dev->policy_sz)) {
+		ret = -EINVAL;
+		goto cleanup;
+	}
+
 	amd_pmf_hex_dump_pb(dev);
 	ret = amd_pmf_start_policy_engine(dev);
 	if (ret < 0)
-		return ret;
+		goto cleanup;
 
 	return length;
+
+cleanup:
+	kfree(dev->policy_buf);
+	dev->policy_buf = NULL;
+	return ret;
 }
 
 static const struct file_operations pb_fops = {
-- 
2.43.0

Re: [PATCH 2/2] drivers/platform/x86/amd: pmf: Handle bad policies in amd_pmf_get_pb_data()

[Ilpo Järvinen]

On Wed, 23 Apr 2025, Mario Limonciello wrote:

> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> If a policy is passed into amd_pmf_get_pb_data() that causes the engine
> to fail to start there is a memory leak. Check for invalid policies as
> well as an error in amd_pmf_start_policy_engine() and free the memory in
> the failure path.
> 
> Fixes: 10817f28e5337 ("platform/x86/amd/pmf: Add capability to sideload of policy binary")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/tee-if.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 41ab9eca5ff13..3c399dc1bfcca 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -377,12 +377,23 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
>  	dev->policy_buf = new_policy_buf;
>  	dev->policy_sz = length;
>  
> +	/* Check if the policy binary is valid */
> +	if (!memchr_inv(dev->policy_buf, 0xff, dev->policy_sz)) {

Since you're adding an explaning comment into two places, it very much 
looks you want a reasonably named helper instead for this so there's no 
need for such comments.

> +		ret = -EINVAL;
> +		goto cleanup;
> +	}
> +
>  	amd_pmf_hex_dump_pb(dev);
>  	ret = amd_pmf_start_policy_engine(dev);
>  	if (ret < 0)
> -		return ret;
> +		goto cleanup;

Isn't this an independent fix that should be in its own patch?

>  
>  	return length;
> +
> +cleanup:
> +	kfree(dev->policy_buf);
> +	dev->policy_buf = NULL;
> +	return ret;
>  }
>  
>  static const struct file_operations pb_fops = {
> 

-- 
 i.

Re: [PATCH 2/2] drivers/platform/x86/amd: pmf: Handle bad policies in amd_pmf_get_pb_data()

[Mario Limonciello]

On 4/23/25 07:19, Ilpo Järvinen wrote:
> On Wed, 23 Apr 2025, Mario Limonciello wrote:
> 
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> If a policy is passed into amd_pmf_get_pb_data() that causes the engine
>> to fail to start there is a memory leak. Check for invalid policies as
>> well as an error in amd_pmf_start_policy_engine() and free the memory in
>> the failure path.
>>
>> Fixes: 10817f28e5337 ("platform/x86/amd/pmf: Add capability to sideload of policy binary")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/platform/x86/amd/pmf/tee-if.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>> index 41ab9eca5ff13..3c399dc1bfcca 100644
>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>> @@ -377,12 +377,23 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
>>   	dev->policy_buf = new_policy_buf;
>>   	dev->policy_sz = length;
>>   
>> +	/* Check if the policy binary is valid */
>> +	if (!memchr_inv(dev->policy_buf, 0xff, dev->policy_sz)) {
> 
> Since you're adding an explaning comment into two places, it very much
> looks you want a reasonably named helper instead for this so there's no
> need for such comments.

OK; I was initially hesitant since it was a single call to memchr_inv() 
but I'll switch it to an inline helper instead.

> 
>> +		ret = -EINVAL;
>> +		goto cleanup;
>> +	}
>> +
>>   	amd_pmf_hex_dump_pb(dev);
>>   	ret = amd_pmf_start_policy_engine(dev);
>>   	if (ret < 0)
>> -		return ret;
>> +		goto cleanup;
> 
> Isn't this an independent fix that should be in its own patch?

Ack; will split into it's own patch.

> 
>>   
>>   	return length;
>> +
>> +cleanup:
>> +	kfree(dev->policy_buf);
>> +	dev->policy_buf = NULL;
>> +	return ret;
>>   }
>>   
>>   static const struct file_operations pb_fops = {
>>
>