Handle bad policies for AMD-PMF better

PATCH Superseded Mario Limonciello 2 participants 5 messages Apr 23, 2025

Subsystems (2)

x86 platform drivers AMD pmf driver

lore

Share & tools

lore

Mario Limonciello

author

2025-04-23 12:12 UTC #0 lore raw

cover Handle bad policies for AMD-PMF better

From: Mario Limonciello <[email protected]>

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(-)

Signature · 1 line

2.43.0

Mario Limonciello

author

2025-04-23 12:12 UTC #1 lore raw

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

From: Mario Limonciello <[email protected]>

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 <[email protected]>

Closes https://lore.kernel.org/platform-driver-x86/[email protected]/ open here

Fixes 376a8c2a14439 ("platform/x86/amd/pmf: Update PMF Driver for Compatibility with new PMF-TA")

Signed-off-by Mario Limonciello <[email protected]>

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

drivers/platform/x86/amd/pmf/tee-if.c +7 -0

index 4c49b510ad1a7..41ab9eca5ff13 100644
@@ -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);

Signature · 1 line

2.43.0

Mario Limonciello

author

2025-04-23 12:12 UTC #2 lore raw

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

In reply to Mario Limonciello

From: Mario Limonciello <[email protected]>

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 <[email protected]>

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

drivers/platform/x86/amd/pmf/tee-if.c +12 -1

index 41ab9eca5ff13..3c399dc1bfcca 100644
@@ -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 = {

Signature · 1 line

2.43.0

Ilpo Järvinen

maintainer

2025-04-23 12:19 UTC #3 lore raw

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

Quoted 18 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>
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 <[email protected]>
---
 drivers/platform/x86/amd/pmf/tee-if.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/platform/x86/amd/pmf/tee-if.c +0 -0
index 41ab9eca5ff13..3c399dc1bfcca 100644

drivers/platform/x86/amd/pmf/tee-if.c static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf, · line 381

Quoted 5 lines

drivers/platform/x86/amd/pmf/tee-if.c +2 -0

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

drivers/platform/x86/amd/pmf/tee-if.c static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf, · line 389

Quoted 9 lines

drivers/platform/x86/amd/pmf/tee-if.c +5 -1

+		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?

Quoted 11 lines

drivers/platform/x86/amd/pmf/tee-if.c +5 -0

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

Signature · 1 line

i.

Mario Limonciello

author

2025-04-23 12:43 UTC #4 lore raw

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

Quoted 23 lines · Ilpo Järvinen

Quoted 21 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
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 <[email protected]>
---
  drivers/platform/x86/amd/pmf/tee-if.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
drivers/platform/x86/amd/pmf/tee-if.c +0 -0
index 41ab9eca5ff13..3c399dc1bfcca 100644
@@ -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;
 

Quoted 6 lines

drivers/platform/x86/amd/pmf/tee-if.c static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf, · line 381
Quoted 2 lines
drivers/platform/x86/amd/pmf/tee-if.c +2 -0
+	/* 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.

Quoted 12 lines

drivers/platform/x86/amd/pmf/tee-if.c static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf, · line 389
Quoted 10 lines
drivers/platform/x86/amd/pmf/tee-if.c +5 -1
+		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.

Quoted 13 lines

Quoted 12 lines

drivers/platform/x86/amd/pmf/tee-if.c +5 -0

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