[v4] tpm_crb: fix fTPM on AMD Zen+ CPUs

Commit Message

Vanya Lazeev Sept. 14, 2019, 5:17 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.

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 a list of struct crb_resource. Each entry holds a
copy of the correcponding ACPI resourse (iores field) and a pointer
to a possibly allocated with devm_ioremap_resource memory region
(iobase 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. The list is used to find corresponding
region for each buffer with crb_containing_resource, make
the buffer size consistent with it's length and map it at most
once, storing the pointer to allocated resource in iobase field
of the entry.

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

Changes in v3:
	- make crb_containing_resource search for address only,
	  because buffer sizes aren't trusted anyway
	- improve commit message

Changes in v4:
	- rename struct crb_resource fields (style change)
	- improve commit message

I believe that storing the data in a in list of
struct crb_resource makes tracking of the resource allocation
state explicit, aiding clarity.
Whilst everything that worked before seems not to be broken,
there is a possibility of allocating with crb_map_resource a resource
that is not from ACPI table, and state of such resource is not
tracked in the current solution. It might be good to track allocation
of all resources, not just ones declared by ACPI, for complete
correctness. However, as I see it now, it will complicate the
code a bit more. Do you think the change should be made, or
such situation is completely hypothetical?

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

Comments

Jarkko Sakkinen Sept. 16, 2019, 5:26 a.m. UTC | #1

On Sat, Sep 14, 2019 at 08:17:44PM +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.
> 
> Memory configuration on ASRock x470 ITX:
> 
> db0a0000-dc59efff : Reserved
> 	dc57e000-dc57efff : MSFT0101:00
> 	dc582000-dc582fff : MSFT0101:00

This is interesting and great thing for us because we can test fully
the tpm_crb part without requiring NVS tweaks. And NVS tweak can then
be built on top of working code.

So far I've seen only AMD fTPM's that report NVS regions. This is
important thing to have in the commit message.

I'll give a detailed review later on.

/Jarkko

Jarkko Sakkinen Sept. 16, 2019, 5:51 a.m. UTC | #2

On Sat, Sep 14, 2019 at 08:17:44PM +0300, [e-mail] wrote:
> +	struct list_head acpi_resources, crb_resources;

Please do not create crb_resources. I said this already last time.

/Jarkko

Vanya Lazeev Sept. 16, 2019, 8 p.m. UTC | #3

On Mon, Sep 16, 2019 at 08:51:30AM +0300, Jarkko Sakkinen wrote:
> On Sat, Sep 14, 2019 at 08:17:44PM +0300, [e-mail] wrote:
> > +	struct list_head acpi_resources, crb_resources;
> 
> Please do not create crb_resources. I said this already last time.

But then, if I'm not mistaken, it will be impossible to track pointers
to multiple remaped regions. In this particular case, it
doesn't matter, because both buffers are in different ACPI regions,
and using acpi_resources only to fix buffer will be enough.
However, this creates incosistency between single- and
multiple-region cases: in the latter iobase field of struct crb_priv
doesn't make any difference. Am I understanding the situation correctly?
Will such fix be ok?

Jarkko Sakkinen Sept. 17, 2019, 7:10 p.m. UTC | #4

On Mon, Sep 16, 2019 at 11:00:30PM +0300, Vanya Lazeev wrote:
> On Mon, Sep 16, 2019 at 08:51:30AM +0300, Jarkko Sakkinen wrote:
> > On Sat, Sep 14, 2019 at 08:17:44PM +0300, [e-mail] wrote:
> > > +	struct list_head acpi_resources, crb_resources;
> > 
> > Please do not create crb_resources. I said this already last time.
> 
> But then, if I'm not mistaken, it will be impossible to track pointers
> to multiple remaped regions. In this particular case, it
> doesn't matter, because both buffers are in different ACPI regions,
> and using acpi_resources only to fix buffer will be enough.
> However, this creates incosistency between single- and
> multiple-region cases: in the latter iobase field of struct crb_priv
> doesn't make any difference. Am I understanding the situation correctly?
> Will such fix be ok?

So why you need to track pointers other than in initialization as devm
will take care of freeing them. Just trying to understand the problem.

/Jarkko

Vanya Lazeev Sept. 17, 2019, 8:54 p.m. UTC | #5

On Tue, Sep 17, 2019 at 10:10:13PM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 16, 2019 at 11:00:30PM +0300, Vanya Lazeev wrote:
> > On Mon, Sep 16, 2019 at 08:51:30AM +0300, Jarkko Sakkinen wrote:
> > > On Sat, Sep 14, 2019 at 08:17:44PM +0300, [e-mail] wrote:
> > > > +	struct list_head acpi_resources, crb_resources;
> > > 
> > > Please do not create crb_resources. I said this already last time.
> > 
> > But then, if I'm not mistaken, it will be impossible to track pointers
> > to multiple remaped regions. In this particular case, it
> > doesn't matter, because both buffers are in different ACPI regions,
> > and using acpi_resources only to fix buffer will be enough.
> > However, this creates incosistency between single- and
> > multiple-region cases: in the latter iobase field of struct crb_priv
> > doesn't make any difference. Am I understanding the situation correctly?
> > Will such fix be ok?
> 
> So why you need to track pointers other than in initialization as devm
> will take care of freeing them. Just trying to understand the problem.
>

We need to know, which ioremap'ed address assign to control area, command
and response buffer, based on which ACPI region contains each of them.
Is there any method of getting remapped address for the raw one after
resouce containing it has been allocated?
And what do you mean by initialization? crb_resources lives only in
crb_map_io, which seems to run only once.

Jarkko Sakkinen Sept. 20, 2019, 3:02 p.m. UTC | #6

On Tue, Sep 17, 2019 at 11:54:03PM +0300, Vanya Lazeev wrote:
> On Tue, Sep 17, 2019 at 10:10:13PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Sep 16, 2019 at 11:00:30PM +0300, Vanya Lazeev wrote:
> > > On Mon, Sep 16, 2019 at 08:51:30AM +0300, Jarkko Sakkinen wrote:
> > > > On Sat, Sep 14, 2019 at 08:17:44PM +0300, [e-mail] wrote:
> > > > > +	struct list_head acpi_resources, crb_resources;
> > > > 
> > > > Please do not create crb_resources. I said this already last time.
> > > 
> > > But then, if I'm not mistaken, it will be impossible to track pointers
> > > to multiple remaped regions. In this particular case, it
> > > doesn't matter, because both buffers are in different ACPI regions,
> > > and using acpi_resources only to fix buffer will be enough.
> > > However, this creates incosistency between single- and
> > > multiple-region cases: in the latter iobase field of struct crb_priv
> > > doesn't make any difference. Am I understanding the situation correctly?
> > > Will such fix be ok?
> > 
> > So why you need to track pointers other than in initialization as devm
> > will take care of freeing them. Just trying to understand the problem.
> >
> 
> We need to know, which ioremap'ed address assign to control area, command
> and response buffer, based on which ACPI region contains each of them.
> Is there any method of getting remapped address for the raw one after
> resouce containing it has been allocated?
> And what do you mean by initialization? crb_resources lives only in
> crb_map_io, which seems to run only once.

Aah, I see.

Well at leat we want the dynamic allocation away from the callback e.g.
use a fixed array:

#define TPM_CRB_MAX_RESOURCES 4 /* Or however many you need */

struct list_head acpi_res_list;
struct acpi_resource *acpi_res_array[TPM_CRB_MAX_RESOURCES];
void __iomem *iobase_array[TPM_CRB_MAX_RESOURCES];

If there are more resources than the constant you could issue a warning
to klog but still try top continue initialization.

PS. Use for new symbols TPM_CRB_ and tpm_crb_ prefixes. Because of
easier tracing of TPM code I want to move to this naming over time.

/Jarkko

Patch

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index e59f1f91d7f3..b301f7fc4a73 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 iores;
+	void __iomem *iobase;
+	struct list_head list;
+};
+
 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, list)
+		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, list) {
+		if (crb_resource_contains(&cres->iores, 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->iores = *res;
+		cres->iores.name = NULL;
+
+		list_add_tail(&cres->list, 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->iores);
+		if (IS_ERR(cres->iobase))
+			return cres->iobase;
+	}
+
+	return cres->iobase + (new_res.start - cres->iores.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;
 
-	if (resource_type(&io_res) != IORESOURCE_MEM) {
+	acpi_dev_free_resource_list(&acpi_resources);
+
+	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->iores, 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->iores.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->iores,
+					      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->iores,
+					      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;
 }