[PATCH v5] tpm_crb: fix fTPM on AMD Zen+ CPUs

* [PATCH v5] tpm_crb: fix fTPM on AMD Zen+ CPUs
@ 2019-09-25 21:56 ivan.lazeev
  2019-09-27 14:56 ` Jarkko Sakkinen
  0 siblings, 1 reply; 2+ messages in thread
From: ivan.lazeev @ 2019-09-25 21:56 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	linux-integrity, linux-kernel, Ivan Lazeev

From: Ivan Lazeev <ivan.lazeev@gmail.com>

Bug link: https://bugzilla.kernel.org/show_bug.cgi?id=195657

cmd/rsp buffers are expected to be in the same ACPI region.
For Zen+ CPUs BIOS's might report two different regions, some of
them also report region sizes inconsistent with values from TPM
registers.

Memory configuration on ASRock x470 ITX:

db0a0000-dc59efff : Reserved
        dc57e000-dc57efff : MSFT0101:00
        dc582000-dc582fff : MSFT0101:00

Work around the issue by storing ACPI regions declared for the
device in struct crb_resources. It contains fixed arrays of
copies of ACPI resourses (iores field) and pointers
to a possibly allocated with devm_ioremap_resource memory region
(iobase field), along with number of entries (num field).
This data was previously held for a single resource
in struct crb_priv (iobase field) and local variable io_res in
crb_map_io function. ACPI resources array is used to find index of
corresponding region for each buffer with crb_containing_res_idx,
make the buffer size consistent with region's length and map it at
most once, storing pointer to the allocated resource in iobase
array at the same index.

Signed-off-by: Ivan Lazeev <ivan.lazeev@gmail.com>
---

Changes in v5:
	- prefix new symbols with tpm_crb_
	- get rid of dynamic allocation in ACPI walker

 drivers/char/tpm/tpm_crb.c | 129 +++++++++++++++++++++++++++----------
 1 file changed, 96 insertions(+), 33 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index e59f1f91d7f3..3c2485c71260 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -22,6 +22,7 @@
 #include "tpm.h"
 
 #define ACPI_SIG_TPM2 "TPM2"
+#define TPM_CRB_MAX_RESOURCES 3
 
 static const guid_t crb_acpi_start_guid =
 	GUID_INIT(0x6BBF6CAB, 0x5463, 0x4714,
@@ -91,7 +92,6 @@ enum crb_status {
 struct crb_priv {
 	u32 sm;
 	const char *hid;
-	void __iomem *iobase;
 	struct crb_regs_head __iomem *regs_h;
 	struct crb_regs_tail __iomem *regs_t;
 	u8 __iomem *cmd;
@@ -108,6 +108,12 @@ struct tpm2_crb_smc {
 	u32 smc_func_id;
 };
 
+struct tpm_crb_resources {
+	struct resource iores[TPM_CRB_MAX_RESOURCES];
+	void __iomem *iobase[TPM_CRB_MAX_RESOURCES];
+	int num;
+};
+
 static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
 				unsigned long timeout)
 {
@@ -432,38 +438,75 @@ static const struct tpm_class_ops tpm_crb = {
 	.req_complete_val = CRB_DRV_STS_COMPLETE,
 };
 
+static bool tpm_crb_resource_contains(struct resource *iores,
+				      u64 address)
+{
+	return address >= iores->start && address <= iores->end;
+}
+
+static int tpm_crb_containing_res_idx(struct tpm_crb_resources *resources,
+				      u64 address)
+{
+	int res_idx;
+
+	for (res_idx = 0; res_idx < resources->num; ++res_idx) {
+		if (tpm_crb_resource_contains(&resources->iores[res_idx],
+					      address))
+			return res_idx;
+	}
+
+	return -1;
+}
+
 static int crb_check_resource(struct acpi_resource *ares, void *data)
 {
-	struct resource *io_res = data;
+	struct tpm_crb_resources *resources = data;
 	struct resource_win win;
 	struct resource *res = &(win.res);
 
 	if (acpi_dev_resource_memory(ares, res) ||
 	    acpi_dev_resource_address_space(ares, &win)) {
-		*io_res = *res;
-		io_res->name = NULL;
+		if (resources->num < TPM_CRB_MAX_RESOURCES) {
+			resources->iores[resources->num] = *res;
+			resources->iores[resources->num].name = NULL;
+		}
+		resources->num += 1;
 	}
 
 	return 1;
 }
 
-static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
-				 struct resource *io_res, u64 start, u32 size)
+static void __iomem *crb_map_res(struct device *dev,
+				 struct tpm_crb_resources *resources,
+				 int res_idx,
+				 u64 start, u32 size)
 {
 	struct resource new_res = {
 		.start	= start,
 		.end	= start + size - 1,
 		.flags	= IORESOURCE_MEM,
 	};
+	struct resource *iores;
+	void __iomem *iobase;
 
 	/* Detect a 64 bit address on a 32 bit system */
 	if (start != new_res.start)
 		return (void __iomem *) ERR_PTR(-EINVAL);
 
-	if (!resource_contains(io_res, &new_res))
+	if (res_idx < 0)
 		return devm_ioremap_resource(dev, &new_res);
 
-	return priv->iobase + (new_res.start - io_res->start);
+	iores = &resources->iores[res_idx];
+	iobase = resources->iobase[res_idx];
+	if (!iobase) {
+		iobase = devm_ioremap_resource(dev, iores);
+		if (IS_ERR(iobase))
+			return iobase;
+
+		resources->iobase[res_idx] = iobase;
+	}
+
+	return iobase + (new_res.start - iores->start);
 }
 
 /*
@@ -490,9 +533,10 @@ static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res,
 static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 		      struct acpi_table_tpm2 *buf)
 {
-	struct list_head resources;
-	struct resource io_res;
+	struct list_head acpi_resources;
 	struct device *dev = &device->dev;
+	struct tpm_crb_resources resources;
+	int res_idx;
 	u32 pa_high, pa_low;
 	u64 cmd_pa;
 	u32 cmd_size;
@@ -501,21 +545,36 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	u32 rsp_size;
 	int ret;
 
-	INIT_LIST_HEAD(&resources);
-	ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
-				     &io_res);
+	INIT_LIST_HEAD(&acpi_resources);
+	memset(&resources, 0, sizeof(resources));
+	ret = acpi_dev_get_resources(device, &acpi_resources,
+				     crb_check_resource, &resources);
 	if (ret < 0)
 		return ret;
-	acpi_dev_free_resource_list(&resources);
+	acpi_dev_free_resource_list(&acpi_resources);
 
-	if (resource_type(&io_res) != IORESOURCE_MEM) {
+	if (resources.num == 0) {
 		dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
 		return -EINVAL;
+	} else if (resources.num > TPM_CRB_MAX_RESOURCES) {
+		dev_warn(dev, FW_BUG "TPM2 ACPI table defines too many memory resources\n");
+		resources.num = TPM_CRB_MAX_RESOURCES;
 	}
 
-	priv->iobase = devm_ioremap_resource(dev, &io_res);
-	if (IS_ERR(priv->iobase))
-		return PTR_ERR(priv->iobase);
+	res_idx = tpm_crb_containing_res_idx(&resources, buf->control_address);
+
+	if (res_idx >= 0 &&
+	    !tpm_crb_resource_contains(&resources.iores[res_idx],
+				       buf->control_address +
+				       sizeof(struct crb_regs_tail) - 1))
+		res_idx = -1;
+
+	priv->regs_t = crb_map_res(dev, &resources, res_idx,
+				   buf->control_address,
+				   sizeof(struct crb_regs_tail));
+
+	if (IS_ERR(priv->regs_t))
+		return PTR_ERR(priv->regs_t);
 
 	/* The ACPI IO region starts at the head area and continues to include
 	 * the control area, as one nice sane region except for some older
@@ -523,9 +582,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	 */
 	if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
 	    (priv->sm == ACPI_TPM2_MEMORY_MAPPED)) {
-		if (buf->control_address == io_res.start +
+		if (res_idx >= 0 &&
+		    buf->control_address == resources.iores[res_idx].start +
 		    sizeof(*priv->regs_h))
-			priv->regs_h = priv->iobase;
+			priv->regs_h = resources.iobase[res_idx];
 		else
 			dev_warn(dev, FW_BUG "Bad ACPI memory layout");
 	}
@@ -534,13 +594,6 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	if (ret)
 		return ret;
 
-	priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
-				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t)) {
-		ret = PTR_ERR(priv->regs_t);
-		goto out_relinquish_locality;
-	}
-
 	/*
 	 * PTT HW bug w/a: wake up the device to access
 	 * possibly not retained registers.
@@ -552,13 +605,17 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);
 	cmd_pa = ((u64)pa_high << 32) | pa_low;
-	cmd_size = crb_fixup_cmd_size(dev, &io_res, cmd_pa,
-				      ioread32(&priv->regs_t->ctrl_cmd_size));
+	cmd_size = ioread32(&priv->regs_t->ctrl_cmd_size);
+
+	res_idx = tpm_crb_containing_res_idx(&resources, cmd_pa);
+	if (res_idx >= 0)
+		cmd_size = crb_fixup_cmd_size(dev, &resources.iores[res_idx],
+					      cmd_pa, cmd_size);
 
 	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
 		pa_high, pa_low, cmd_size);
 
-	priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size);
+	priv->cmd = crb_map_res(dev, &resources, res_idx, cmd_pa, cmd_size);
 	if (IS_ERR(priv->cmd)) {
 		ret = PTR_ERR(priv->cmd);
 		goto out;
@@ -566,11 +623,17 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
 	rsp_pa = le64_to_cpu(__rsp_pa);
-	rsp_size = crb_fixup_cmd_size(dev, &io_res, rsp_pa,
-				      ioread32(&priv->regs_t->ctrl_rsp_size));
+	rsp_size = ioread32(&priv->regs_t->ctrl_rsp_size);
+
+	res_idx = tpm_crb_containing_res_idx(&resources, rsp_pa);
+
+	if (res_idx >= 0)
+		rsp_size = crb_fixup_cmd_size(dev, &resources.iores[res_idx],
+					      rsp_pa, rsp_size);
 
 	if (cmd_pa != rsp_pa) {
-		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
+		priv->rsp = crb_map_res(dev, &resources, res_idx,
+					rsp_pa, rsp_size);
 		ret = PTR_ERR_OR_ZERO(priv->rsp);
 		goto out;
 	}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread