tpm_crb: fix fTPM on AMD Zen+ CPUs

Commit Message

Vanya Lazeev Sept. 4, 2019, 7:03 p.m. UTC

From: Ivan Lazeev <[e-mail]>

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.

Work around the issue by storing ACPI regions declared for the
device in a list, then using it to find entry corresponding
to each buffer. Use information from the entry to map each resource
at most once and make buffer size consistent with the length of the
region.

Signed-off-by: Ivan Lazeev <[e-mail]>
---

Tested on ASRock x470 ITX motherboard with Ryzen 2600X CPU.
However, I don't have any other hardware to test the changes on and no
expertise to be sure that other TPMs won't break as a result.

 drivers/char/tpm/tpm_crb.c | 137 +++++++++++++++++++++++++++----------
 1 file changed, 101 insertions(+), 36 deletions(-)

Comments

Jarkko Sakkinen Sept. 7, 2019, 9:49 p.m. UTC | #1

On Wed, 2019-09-04 at 22:03 +0300, [e-mail] wrote:
> From: Ivan Lazeev <[e-mail]>
> 
> 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.
> 
> Work around the issue by storing ACPI regions declared for the
> device in a list, then using it to find entry corresponding
> to each buffer. Use information from the entry to map each resource
> at most once and make buffer size consistent with the length of the
> region.
> 
> Signed-off-by: Ivan Lazeev <[e-mail]>

Can you add the relevant pieces of /proc/iomem output to the commit
message so we can see the memory configuration? I don't have the
hardware available where this kind of situation occurs.

> ---
> 
> Tested on ASRock x470 ITX motherboard with Ryzen 2600X CPU.
> However, I don't have any other hardware to test the changes on and no
> expertise to be sure that other TPMs won't break as a result.

You can still ask yourself that if the hardware describes the memory
with only one region do the code flows reduce mostly to do the same as
before this patch. It is not the same as testing with such hardware but
it is still a good mental exercise.

To summarize, I do get "not having hardware" part but completely
disqualify "not having expertise" part as the expertise needed here has
nothing specific to do with TPMs :-)

> 
>  drivers/char/tpm/tpm_crb.c | 137 +++++++++++++++++++++++++++----------
>  1 file changed, 101 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index e59f1f91d7f3..0bcfe52db5d6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -91,7 +91,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 +107,12 @@ struct tpm2_crb_smc {
>  	u32 smc_func_id;
>  };
>  
> +struct crb_resource {
> +	struct resource io_res;
> +	void __iomem *iobase;
> +	struct list_head node;
> +};

Please rename 'node' as 'list' for the sake of coherency with the rest
of the kernel and io_res as iores for the sake of coherency with the
other fields.

I think it would be cleaner to do the following:

1. Start with empty resource list.
2. Everytime crb_map_res() is called it does check the existing
   resources and does a ACPI walk on need basis at most adding
   one new entry.

In other words, crb_map_res() gets the list and appends one entry when
necessary.

Overhead of doing multiple walks is irrelevant here. The simplicity and
clarity win.

/Jarkko

Jarkko Sakkinen Sept. 7, 2019, 11:58 p.m. UTC | #2

On Sun, 2019-09-08 at 00:49 +0300, Jarkko Sakkinen wrote:
> On Wed, 2019-09-04 at 22:03 +0300, [e-mail] wrote:
> > From: Ivan Lazeev <[e-mail]>
> > 
> > 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.
> > 
> > Work around the issue by storing ACPI regions declared for the
> > device in a list, then using it to find entry corresponding
> > to each buffer. Use information from the entry to map each resource
> > at most once and make buffer size consistent with the length of the
> > region.
> > 
> > Signed-off-by: Ivan Lazeev <[e-mail]>
> 
> Can you add the relevant pieces of /proc/iomem output to the commit
> message so we can see the memory configuration? I don't have the
> hardware available where this kind of situation occurs.

Here in particular this is more than relevant because given that this
patch fixes the issue for you, it seems that you don't have the regions
marked as NVS regions.

/Jarkko

Jarkko Sakkinen Sept. 10, 2019, 12:32 p.m. UTC | #3

On Wed, Sep 04, 2019 at 10:03:32PM +0300, [e-mail] wrote:
> From: Ivan Lazeev <[e-mail]>
> 
> 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.
> 
> Work around the issue by storing ACPI regions declared for the
> device in a list, then using it to find entry corresponding
> to each buffer. Use information from the entry to map each resource
> at most once and make buffer size consistent with the length of the
> region.
> 
> Signed-off-by: Ivan Lazeev <[e-mail]>
> ---

Partial re-review. Please ignore the comment about multiple ACPI walks
in my previous review. Most of the other comments hold (like partial
/proc/iomem output *must* be included).

One remark I forgot is that this the v3 of your patch. Please version
your patches properly and provide the full changelog in v4. There is no
documentation what happend between v2 and v3.

Please provide more verbose explanation what the patch does. "Using it"
does not tell at all *what* is used and *how*. Please introduce the new
data structures and explain how they are used and what they replace and
so forth (you probably get the idea).

The commit message is definitely the weakest link of this patch right
now.

>  static int crb_check_resource(struct acpi_resource *ares, void *data)
>  {
> -	struct resource *io_res = data;
> +	struct list_head *list = data;
> +	struct crb_resource *cres;
>  	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;
> +		cres = kzalloc(sizeof(*cres), GFP_KERNEL);
> +		if (!cres)
> +			return -ENOMEM;
> +
> +		cres->io_res = *res;
> +		cres->io_res.name = NULL;
> +
> +		list_add_tail(&cres->node, list);
>  	}
>  
>  	return 1;

I think that you might get away just by collecting acpi_resources and
pass that to your helpers. See the documentation of the function on
how to make it collect all the resources:

https://elixir.bootlin.com/linux/latest/source/drivers/acpi/resource.c#L622

Of course you then have to move acpi_dev_free_resource_list() to be
called after you've done ioremaps.

This was the essential thing that I wanted to remark compared to my
previous review.

/Jarkko

Patch

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index e59f1f91d7f3..0bcfe52db5d6 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -91,7 +91,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 +107,12 @@  struct tpm2_crb_smc {
 	u32 smc_func_id;
 };
 
+struct crb_resource {
+	struct resource io_res;
+	void __iomem *iobase;
+	struct list_head node;
+};
+
 static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
 				unsigned long timeout)
 {
@@ -432,23 +437,57 @@  static const struct tpm_class_ops tpm_crb = {
 	.req_complete_val = CRB_DRV_STS_COMPLETE,
 };
 
+static void crb_free_resource_list(struct list_head *resources)
+{
+	struct crb_resource *cres, *tmp;
+
+	list_for_each_entry_safe(cres, tmp, resources, node)
+		kfree(cres);
+}
+
+static inline bool crb_resource_contains(const struct resource *io_res,
+					 u64 address)
+{
+	return address >= io_res->start && address <= io_res->end;
+}
+
+static struct crb_resource *crb_containing_resource(
+		const struct list_head *resources, u64 start)
+{
+	struct crb_resource *cres;
+
+	list_for_each_entry(cres, resources, node) {
+		if (crb_resource_contains(&cres->io_res, start))
+			return cres;
+	}
+
+	return NULL;
+}
+
 static int crb_check_resource(struct acpi_resource *ares, void *data)
 {
-	struct resource *io_res = data;
+	struct list_head *list = data;
+	struct crb_resource *cres;
 	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;
+		cres = kzalloc(sizeof(*cres), GFP_KERNEL);
+		if (!cres)
+			return -ENOMEM;
+
+		cres->io_res = *res;
+		cres->io_res.name = NULL;
+
+		list_add_tail(&cres->node, list);
 	}
 
 	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 crb_resource *cres,
+				 u64 start, u32 size)
 {
 	struct resource new_res = {
 		.start	= start,
@@ -460,10 +499,16 @@  static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
 	if (start != new_res.start)
 		return (void __iomem *) ERR_PTR(-EINVAL);
 
-	if (!resource_contains(io_res, &new_res))
+	if (!cres)
 		return devm_ioremap_resource(dev, &new_res);
 
-	return priv->iobase + (new_res.start - io_res->start);
+	if (!cres->iobase) {
+		cres->iobase = devm_ioremap_resource(dev, &cres->io_res);
+		if (IS_ERR(cres->iobase))
+			return cres->iobase;
+	}
+
+	return cres->iobase + (new_res.start - cres->io_res.start);
 }
 
 /*
@@ -490,9 +535,9 @@  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, crb_resources;
 	struct device *dev = &device->dev;
+	struct crb_resource *cres;
 	u32 pa_high, pa_low;
 	u64 cmd_pa;
 	u32 cmd_size;
@@ -501,21 +546,34 @@  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);
+	INIT_LIST_HEAD(&crb_resources);
+	ret = acpi_dev_get_resources(device, &acpi_resources,
+				     crb_check_resource, &crb_resources);
 	if (ret < 0)
-		return ret;
-	acpi_dev_free_resource_list(&resources);
+		goto out_free_crb_resources;
+
+	acpi_dev_free_resource_list(&acpi_resources);
 
-	if (resource_type(&io_res) != IORESOURCE_MEM) {
+	if (list_empty(&crb_resources)) {
 		dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_free_crb_resources;
 	}
 
-	priv->iobase = devm_ioremap_resource(dev, &io_res);
-	if (IS_ERR(priv->iobase))
-		return PTR_ERR(priv->iobase);
+	cres = crb_containing_resource(&crb_resources, buf->control_address);
+
+	if (cres &&
+	    !crb_resource_contains(&cres->io_res, buf->control_address +
+	    sizeof(struct crb_regs_tail) - 1))
+		cres = NULL;
+
+	priv->regs_t = crb_map_res(dev, cres, buf->control_address,
+				   sizeof(struct crb_regs_tail));
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_free_crb_resources;
+	}
 
 	/* 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,23 +581,17 @@  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 (cres &&
+		    buf->control_address == cres->io_res.start +
 		    sizeof(*priv->regs_h))
-			priv->regs_h = priv->iobase;
+			priv->regs_h = cres->iobase;
 		else
 			dev_warn(dev, FW_BUG "Bad ACPI memory layout");
 	}
 
 	ret = __crb_request_locality(dev, priv, 0);
 	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;
-	}
+		goto out_free_crb_resources;
 
 	/*
 	 * PTT HW bug w/a: wake up the device to access
@@ -552,13 +604,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);
+
+	cres = crb_containing_resource(&crb_resources, cmd_pa);
+	if (cres)
+		cmd_size = crb_fixup_cmd_size(dev, &cres->io_res,
+					      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, cres, cmd_pa, cmd_size);
 	if (IS_ERR(priv->cmd)) {
 		ret = PTR_ERR(priv->cmd);
 		goto out;
@@ -566,11 +622,16 @@  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);
+
+	cres = crb_containing_resource(&crb_resources, rsp_pa);
+
+	if (cres)
+		rsp_size = crb_fixup_cmd_size(dev, &cres->io_res,
+					      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, cres, rsp_pa, rsp_size);
 		ret = PTR_ERR_OR_ZERO(priv->rsp);
 		goto out;
 	}
@@ -596,6 +657,10 @@  static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	__crb_relinquish_locality(dev, priv, 0);
 
+out_free_crb_resources:
+
+	crb_free_resource_list(&crb_resources);
+
 	return ret;
 }