diff mbox series

[v2,1/2] tpm: tpm_crb: enhance command and response buffer size calculation code

Message ID 20190909090906.28700-2-kkamagui@gmail.com (mailing list archive)
State New, archived
Headers show
Series Enhance support for the AMD's fTPM | expand

Commit Message

Seunghun Han Sept. 9, 2019, 9:09 a.m. UTC 
The purpose of crb_fixup_cmd_size() function is to work around broken
BIOSes and get the trustable size between the ACPI region and register.
When the TPM has a command buffer and response buffer independently,
the crb_map_io() function calls crb_fixup_cmd_size() twice to calculate
each buffer size.  However, the current implementation of it considers
one of two buffers.

To support independent command and response buffers, I changed
crb_check_resource() function for storing ACPI TPB regions to a list.
I also changed crb_fixup_cmd_size() to use the list for calculating each
buffer size.

Signed-off-by: Seunghun Han <kkamagui@gmail.com>
---
Changes in v2: same as v1.

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

Comments

Jarkko Sakkinen Sept. 10, 2019, 12:34 p.m. UTC | #1 
On Mon, Sep 09, 2019 at 06:09:05PM +0900, Seunghun Han wrote:
> The purpose of crb_fixup_cmd_size() function is to work around broken
> BIOSes and get the trustable size between the ACPI region and register.
> When the TPM has a command buffer and response buffer independently,
> the crb_map_io() function calls crb_fixup_cmd_size() twice to calculate
> each buffer size.  However, the current implementation of it considers
> one of two buffers.
> 
> To support independent command and response buffers, I changed
> crb_check_resource() function for storing ACPI TPB regions to a list.
> I also changed crb_fixup_cmd_size() to use the list for calculating each
> buffer size.
> 
> Signed-off-by: Seunghun Han <kkamagui@gmail.com>

I think as far as the tpm_crb goes I focus on getting Vanya's change
landed because it is better structured, more mature and the first
version was sent couple of weeks earlier. You are welcome to make
your remarks on that patch.

/Jarkko
Seunghun Han Sept. 10, 2019, 3:12 p.m. UTC | #2 
>
> On Mon, Sep 09, 2019 at 06:09:05PM +0900, Seunghun Han wrote:
> > The purpose of crb_fixup_cmd_size() function is to work around broken
> > BIOSes and get the trustable size between the ACPI region and register.
> > When the TPM has a command buffer and response buffer independently,
> > the crb_map_io() function calls crb_fixup_cmd_size() twice to calculate
> > each buffer size.  However, the current implementation of it considers
> > one of two buffers.
> >
> > To support independent command and response buffers, I changed
> > crb_check_resource() function for storing ACPI TPB regions to a list.
> > I also changed crb_fixup_cmd_size() to use the list for calculating each
> > buffer size.
> >
> > Signed-off-by: Seunghun Han <kkamagui@gmail.com>
>
> I think as far as the tpm_crb goes I focus on getting Vanya's change
> landed because it is better structured, more mature and the first
> version was sent couple of weeks earlier. You are welcome to make
> your remarks on that patch.

Thank you for your review. I already knew Vanya's patch,
https://lkml.org/lkml/2019/8/11/151, and this patch didn't work for
me. I also couldn't agree on some points like memory allocating inside
the ACPI walker and changing many parts of TPM driver. I would like to
support AMD's fTPM with the smallest changes since this is a
workaround as you know.

I didn't understand clearly what your point is. Do you want me to
change my patches structurally like Vanya's patch and make patch v3?
or want me to give some advice to Vanya?

>
> /Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index e59f1f91d7f3..14f486c23af2 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -442,6 +442,9 @@  static int crb_check_resource(struct acpi_resource *ares, void *data)
 	    acpi_dev_resource_address_space(ares, &win)) {
 		*io_res = *res;
 		io_res->name = NULL;
+
+		/* Add this TPM CRB resource to the list */
+		return 0;
 	}
 
 	return 1;
@@ -471,7 +474,7 @@  static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
  * region vs the registers. Trust the ACPI region. Such broken systems
  * probably cannot send large TPM commands since the buffer will be truncated.
  */
-static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res,
+static u64 __crb_fixup_cmd_size(struct device *dev, struct resource *io_res,
 			      u64 start, u64 size)
 {
 	if (io_res->start > start || io_res->end < start)
@@ -487,6 +490,26 @@  static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res,
 	return io_res->end - start + 1;
 }
 
+static u64 crb_fixup_cmd_size(struct device *dev, struct list_head *resources,
+			      u64 start, u64 size)
+{
+	struct resource_entry *pos;
+	struct resource *cur_res;
+	u64 ret = size;
+
+	/* Check all TPM CRB resources with the start and size values */
+	resource_list_for_each_entry(pos, resources) {
+		cur_res = pos->res;
+
+		ret = __crb_fixup_cmd_size(dev, cur_res, start, size);
+		/* Broken BIOS is detected. Trust the ACPI region. */
+		if (ret < size)
+			break;
+	}
+
+	return ret;
+}
+
 static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 		      struct acpi_table_tpm2 *buf)
 {
@@ -506,16 +529,18 @@  static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 				     &io_res);
 	if (ret < 0)
 		return ret;
-	acpi_dev_free_resource_list(&resources);
 
 	if (resource_type(&io_res) != IORESOURCE_MEM) {
 		dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_early;
 	}
 
 	priv->iobase = devm_ioremap_resource(dev, &io_res);
-	if (IS_ERR(priv->iobase))
-		return PTR_ERR(priv->iobase);
+	if (IS_ERR(priv->iobase)) {
+		ret = PTR_ERR(priv->iobase);
+		goto out_early;
+	}
 
 	/* 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
@@ -532,7 +557,7 @@  static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	ret = __crb_request_locality(dev, priv, 0);
 	if (ret)
-		return ret;
+		goto out_early;
 
 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
 				   sizeof(struct crb_regs_tail));
@@ -552,7 +577,7 @@  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,
+	cmd_size = crb_fixup_cmd_size(dev, &resources, cmd_pa,
 				      ioread32(&priv->regs_t->ctrl_cmd_size));
 
 	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
@@ -566,7 +591,7 @@  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,
+	rsp_size = crb_fixup_cmd_size(dev, &resources, rsp_pa,
 				      ioread32(&priv->regs_t->ctrl_rsp_size));
 
 	if (cmd_pa != rsp_pa) {
@@ -596,6 +621,9 @@  static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	__crb_relinquish_locality(dev, priv, 0);
 
+out_early:
+	acpi_dev_free_resource_list(&resources);
+
 	return ret;
 }