[PATCH v3 0/4] platform/x86/amd/pmf: Fix policy binary handling

[PATCH v3 0/4] platform/x86/amd/pmf: Fix policy binary handling

[Armin Wolf]

This patch series fixes various issues inside the policy binary
handling code.
The first patch makes sure that a valid error code is returned upon
failing to start the policy engine, while the second patch drops the
usage of readl() on non-io memory.
The last two patches fix a possible out-of-bounds memory access when
parsing the policy binary header.

All patches are compile-tested only.

Changes since v2:
- add patches 1 and 3

Changes since v1:
- get the full dword instead of only 8 bits when reading the header
- check if the policy buffer also has enough room for storing the length

Armin Wolf (4):
  platform/x86/amd/pmf: Fix return value of
    amd_pmf_start_policy_engine()
  platform/x86/amd/pmf: Do not use readl() for policy buffer access
  platform/x86/amd/pmf: Use struct for cookie header
  platform/x86/amd/pmf: Fix possible out-of-bound memory accesses

 drivers/platform/x86/amd/pmf/pmf.h    |  6 +++++-
 drivers/platform/x86/amd/pmf/tee-if.c | 21 +++++++++++++--------
 2 files changed, 18 insertions(+), 9 deletions(-)

--
2.39.2

[PATCH v3 1/4] platform/x86/amd/pmf: Fix return value of amd_pmf_start_policy_engine()

[Armin Wolf]

amd_pmf_start_policy_engine() returns an negative error code upon
failure, so the TA_PMF_* error codes cannot be used here.

Return -EIO instead. Also stop shadowing the return code in
amd_pmf_get_pb_data().

Compile-tested only.

Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Fixes: 7c45534afa44 ("platform/x86/amd/pmf: Add support for PMF Policy Binary")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/amd/pmf/tee-if.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 16973bebf55f..13dd4462e1e3 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -269,7 +269,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
 	} else {
 		dev_err(dev->dev, "ta invoke cmd init failed err: %x\n", res);
 		dev->smart_pc_enabled = PMF_SMART_PC_DISABLED;
-		return res;
+		return -EIO;
 	}

 	return 0;
@@ -309,8 +309,8 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,

 	amd_pmf_hex_dump_pb(dev);
 	ret = amd_pmf_start_policy_engine(dev);
-	if (ret)
-		return -EINVAL;
+	if (ret < 0)
+		return ret;

 	return length;
 }
--
2.39.2

[PATCH v3 2/4] platform/x86/amd/pmf: Do not use readl() for policy buffer access

[Armin Wolf]

The policy buffer is allocated using normal memory allocation
functions, so readl() should not be used on it.

Compile-tested only.

Fixes: 7c45534afa44 ("platform/x86/amd/pmf: Add support for PMF Policy Binary")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/amd/pmf/tee-if.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 13dd4462e1e3..58ec2c9606e1 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -249,8 +249,8 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
 	u32 cookie, length;
 	int res;

-	cookie = readl(dev->policy_buf + POLICY_COOKIE_OFFSET);
-	length = readl(dev->policy_buf + POLICY_COOKIE_LEN);
+	cookie = *(u32 *)(dev->policy_buf + POLICY_COOKIE_OFFSET);
+	length = *(u32 *)(dev->policy_buf + POLICY_COOKIE_LEN);

 	if (cookie != POLICY_SIGN_COOKIE || !length)
 		return -EINVAL;
--
2.39.2

[PATCH v3 3/4] platform/x86/amd/pmf: Use struct for cookie header

[Armin Wolf]

The cookie header consists of a sign field and a length field.
Combine both in a single struct to make accesses simpler.

Compile-tested only.

Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/amd/pmf/pmf.h    | 6 +++++-
 drivers/platform/x86/amd/pmf/tee-if.c | 9 ++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index bcf777a5659a..0c90805dc85b 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -17,7 +17,11 @@
 #define POLICY_BUF_MAX_SZ		0x4b000
 #define POLICY_SIGN_COOKIE		0x31535024
 #define POLICY_COOKIE_OFFSET		0x10
-#define POLICY_COOKIE_LEN		0x14
+
+struct cookie_header {
+	u32 sign;
+	u32 length;
+} __packed;

 /* APMF Functions */
 #define APMF_FUNC_VERIFY_INTERFACE			0
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 58ec2c9606e1..71ea7eefc211 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -246,17 +246,16 @@ static void amd_pmf_invoke_cmd(struct work_struct *work)

 static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
 {
-	u32 cookie, length;
+	struct cookie_header *header;
 	int res;

-	cookie = *(u32 *)(dev->policy_buf + POLICY_COOKIE_OFFSET);
-	length = *(u32 *)(dev->policy_buf + POLICY_COOKIE_LEN);
+	header = (struct cookie_header *)(dev->policy_buf + POLICY_COOKIE_OFFSET);

-	if (cookie != POLICY_SIGN_COOKIE || !length)
+	if (header->sign != POLICY_SIGN_COOKIE || !header->length)
 		return -EINVAL;

 	/* Update the actual length */
-	dev->policy_sz = length + 512;
+	dev->policy_sz = header->length + 512;
 	res = amd_pmf_invoke_cmd_init(dev);
 	if (res == TA_PMF_TYPE_SUCCESS) {
 		/* Now its safe to announce that smart pc is enabled */
--
2.39.2

[PATCH v3 4/4] platform/x86/amd/pmf: Fix possible out-of-bound memory accesses

[Armin Wolf]

The length of the policy buffer is not validated before accessing it,
which means that multiple out-of-bounds memory accesses can occur.

This is especially bad since userspace can load policy binaries over
debugfs.

Compile-tested only.

Fixes: 7c45534afa44 ("platform/x86/amd/pmf: Add support for PMF Policy Binary")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/amd/pmf/tee-if.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 71ea7eefc211..75370431a82e 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -249,11 +249,17 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
 	struct cookie_header *header;
 	int res;

+	if (dev->policy_sz < POLICY_COOKIE_OFFSET + sizeof(*header))
+		return -EINVAL;
+
 	header = (struct cookie_header *)(dev->policy_buf + POLICY_COOKIE_OFFSET);

 	if (header->sign != POLICY_SIGN_COOKIE || !header->length)
 		return -EINVAL;

+	if (dev->policy_sz < header->length + 512)
+		return -EINVAL;
+
 	/* Update the actual length */
 	dev->policy_sz = header->length + 512;
 	res = amd_pmf_invoke_cmd_init(dev);
--
2.39.2

Re: [PATCH v3 0/4] platform/x86/amd/pmf: Fix policy binary handling

[Shyam Sundar S K]

On 3/5/2024 02:20, Armin Wolf wrote:
> This patch series fixes various issues inside the policy binary
> handling code.
> The first patch makes sure that a valid error code is returned upon
> failing to start the policy engine, while the second patch drops the
> usage of readl() on non-io memory.
> The last two patches fix a possible out-of-bounds memory access when
> parsing the policy binary header.
> 
> All patches are compile-tested only.
> 
> Changes since v2:
> - add patches 1 and 3
> 
> Changes since v1:
> - get the full dword instead of only 8 bits when reading the header
> - check if the policy buffer also has enough room for storing the length

Thank you Armin. Series looks good to me.

Reviewed-by: Shyam Sundar S K Shyam-sundar.S-k@amd.com

> 
> Armin Wolf (4):
>   platform/x86/amd/pmf: Fix return value of
>     amd_pmf_start_policy_engine()
>   platform/x86/amd/pmf: Do not use readl() for policy buffer access
>   platform/x86/amd/pmf: Use struct for cookie header
>   platform/x86/amd/pmf: Fix possible out-of-bound memory accesses
> 
>  drivers/platform/x86/amd/pmf/pmf.h    |  6 +++++-
>  drivers/platform/x86/amd/pmf/tee-if.c | 21 +++++++++++++--------
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 
> --
> 2.39.2
> 

Re: [PATCH v3 0/4] platform/x86/amd/pmf: Fix policy binary handling

[Ilpo Järvinen]

On Wed, 6 Mar 2024, Shyam Sundar S K wrote:
> On 3/5/2024 02:20, Armin Wolf wrote:
> > This patch series fixes various issues inside the policy binary
> > handling code.
> > The first patch makes sure that a valid error code is returned upon
> > failing to start the policy engine, while the second patch drops the
> > usage of readl() on non-io memory.
> > The last two patches fix a possible out-of-bounds memory access when
> > parsing the policy binary header.
> > 
> > All patches are compile-tested only.
> > 
> > Changes since v2:
> > - add patches 1 and 3
> > 
> > Changes since v1:
> > - get the full dword instead of only 8 bits when reading the header
> > - check if the policy buffer also has enough room for storing the length
> 
> Thank you Armin. Series looks good to me.
> 
> Reviewed-by: Shyam Sundar S K Shyam-sundar.S-k@amd.com

Thanks for taking a look.

Btw, you were missing <> around the address so the patchwork automation 
didn't catch that line. To help the patchwork to capture that tag (no need 
for you to do anything):

Reviewed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

--
 i.

> > Armin Wolf (4):
> >   platform/x86/amd/pmf: Fix return value of
> >     amd_pmf_start_policy_engine()
> >   platform/x86/amd/pmf: Do not use readl() for policy buffer access
> >   platform/x86/amd/pmf: Use struct for cookie header
> >   platform/x86/amd/pmf: Fix possible out-of-bound memory accesses

Re: [PATCH v3 0/4] platform/x86/amd/pmf: Fix policy binary handling

[Ilpo Järvinen]

On Mon, 04 Mar 2024 21:50:01 +0100, Armin Wolf wrote:

> This patch series fixes various issues inside the policy binary
> handling code.
> The first patch makes sure that a valid error code is returned upon
> failing to start the policy engine, while the second patch drops the
> usage of readl() on non-io memory.
> The last two patches fix a possible out-of-bounds memory access when
> parsing the policy binary header.
> 
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo branch. Note it will show up in the public
platform-drivers-x86/review-ilpo branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/4] platform/x86/amd/pmf: Fix return value of amd_pmf_start_policy_engine()
      commit: 98cfcece0ab86c99bc106633d764fb6ad4a35b8e
[2/4] platform/x86/amd/pmf: Do not use readl() for policy buffer access
      commit: 379a7c64c4fa33315b504ede86a87188dc88fef4
[3/4] platform/x86/amd/pmf: Use struct for cookie header
      commit: a87d92223084f61d37da4952ad68634ea8a7caaf
[4/4] platform/x86/amd/pmf: Fix possible out-of-bound memory accesses
      commit: 1e7a14ee259e2ff85be51bf36a7692b20233159a

--
 i.