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

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

[ivan.lazeev]

From: Vanya 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 a fixed array and adding an array for pointers to
corresponding possibly allocated resources in crb_map_io function.
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 and make the buffer size
consistent with region's length. Array of pointers to allocated
resources is used to map the region at most once.

Signed-off-by: Ivan Lazeev <ivan.lazeev@gmail.com>
---
Changes in v6:
	- got rid of new structures
	- open coded helper functions
	- removed incorrect FW_BUG

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

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index e59f1f91d7f3..8177406aecd6 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;
@@ -434,21 +434,27 @@ static const struct tpm_class_ops tpm_crb = {
 
 static int crb_check_resource(struct acpi_resource *ares, void *data)
 {
-	struct resource *io_res = data;
+	struct resource **iores_range = 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 (iores_range[0] == iores_range[1])
+			iores_range[0] = NULL;
+
+		if (iores_range[0]) {
+			*iores_range[0] = *res;
+			iores_range[0]->name = NULL;
+			iores_range[0] += 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 resource *iores,
+				 void __iomem **iobase_ptr, u64 start, u32 size)
 {
 	struct resource new_res = {
 		.start	= start,
@@ -460,10 +466,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 (!iores)
 		return devm_ioremap_resource(dev, &new_res);
 
-	return priv->iobase + (new_res.start - io_res->start);
+	if (!*iobase_ptr) {
+		*iobase_ptr = devm_ioremap_resource(dev, iores);
+		if (IS_ERR(*iobase_ptr))
+			return *iobase_ptr;
+	}
+
+	return *iobase_ptr + (new_res.start - iores->start);
 }
 
 /*
@@ -490,9 +502,17 @@ 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_resource_list;
+	struct resource iores_array[TPM_CRB_MAX_RESOURCES];
+	void __iomem *iobase_array[TPM_CRB_MAX_RESOURCES] = {0};
+	struct resource *iores_range[] = {
+		iores_array,
+		iores_array + TPM_CRB_MAX_RESOURCES
+	};
 	struct device *dev = &device->dev;
+	struct resource *iores;
+	void __iomem **iobase_ptr;
+	int num_resources, i;
 	u32 pa_high, pa_low;
 	u64 cmd_pa;
 	u32 cmd_size;
@@ -501,21 +521,40 @@ 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_resource_list);
+	ret = acpi_dev_get_resources(device, &acpi_resource_list,
+				     crb_check_resource, iores_range);
 	if (ret < 0)
 		return ret;
-	acpi_dev_free_resource_list(&resources);
+	acpi_dev_free_resource_list(&acpi_resource_list);
 
-	if (resource_type(&io_res) != IORESOURCE_MEM) {
+	if (iores_range[0] == iores_array) {
 		dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
 		return -EINVAL;
+	} else if (!iores_range[0]) {
+		dev_warn(dev, "TPM2 ACPI table defines too many memory resources\n");
+		iores_range[0] = iores_range[1];
 	}
 
-	priv->iobase = devm_ioremap_resource(dev, &io_res);
-	if (IS_ERR(priv->iobase))
-		return PTR_ERR(priv->iobase);
+	num_resources = iores_range[0] - iores_array;
+
+	iores = NULL;
+	iobase_ptr = NULL;
+	for (i = 0; i < num_resources; ++i) {
+		if (buf->control_address >= iores_array[i].start &&
+		    buf->control_address + sizeof(struct crb_regs_tail) - 1 <=
+		    iores_array[i].end) {
+			iores = iores_array + i;
+			iobase_ptr = iobase_array + i;
+			break;
+		}
+	}
+
+	priv->regs_t = crb_map_res(dev, iores, iobase_ptr, 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 +562,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 (iores &&
+		    buf->control_address == iores->start +
 		    sizeof(*priv->regs_h))
-			priv->regs_h = priv->iobase;
+			priv->regs_h = *iobase_ptr;
 		else
 			dev_warn(dev, FW_BUG "Bad ACPI memory layout");
 	}
@@ -534,13 +574,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 +585,26 @@ 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);
+
+	iores = NULL;
+	iobase_ptr = NULL;
+	for (i = 0; i < num_resources; ++i) {
+		if (cmd_pa >= iores_array[i].start &&
+		    cmd_pa <= iores_array[i].end) {
+			iores = iores_array + i;
+			iobase_ptr = iobase_array + i;
+			break;
+		}
+	}
+
+	if (iores)
+		cmd_size = crb_fixup_cmd_size(dev, 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, iores, iobase_ptr,	cmd_pa, cmd_size);
 	if (IS_ERR(priv->cmd)) {
 		ret = PTR_ERR(priv->cmd);
 		goto out;
@@ -566,11 +612,25 @@ 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);
+
+	iores = NULL;
+	iobase_ptr = NULL;
+	for (i = 0; i < num_resources; ++i) {
+		if (rsp_pa >= iores_array[i].start &&
+		    rsp_pa <= iores_array[i].end) {
+			iores = iores_array + i;
+			iobase_ptr = iobase_array + i;
+			break;
+		}
+	}
+
+	if (iores)
+		rsp_size = crb_fixup_cmd_size(dev, 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, iores, iobase_ptr,
+					rsp_pa, rsp_size);
 		ret = PTR_ERR_OR_ZERO(priv->rsp);
 		goto out;
 	}
-- 
2.20.1

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

[Jarkko Sakkinen]

On Wed, Oct 02, 2019 at 11:12:12PM +0300, ivan.lazeev@gmail.com wrote:
> From: Vanya 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 a fixed array and adding an array for pointers to
> corresponding possibly allocated resources in crb_map_io function.
> 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 and make the buffer size
> consistent with region's length. Array of pointers to allocated
> resources is used to map the region at most once.
> 
> Signed-off-by: Ivan Lazeev <ivan.lazeev@gmail.com>

I'm getting soon Udoo Bolt that I should be able to use to test this
change.

> ---
> Changes in v6:
> 	- got rid of new structures
> 	- open coded helper functions
> 	- removed incorrect FW_BUG
> 
>  drivers/char/tpm/tpm_crb.c | 126 +++++++++++++++++++++++++++----------
>  1 file changed, 93 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index e59f1f91d7f3..8177406aecd6 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;
> @@ -434,21 +434,27 @@ static const struct tpm_class_ops tpm_crb = {
>  
>  static int crb_check_resource(struct acpi_resource *ares, void *data)
>  {
> -	struct resource *io_res = data;
> +	struct resource **iores_range = 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 (iores_range[0] == iores_range[1])
> +			iores_range[0] = NULL;
> +
> +		if (iores_range[0]) {
> +			*iores_range[0] = *res;
> +			iores_range[0]->name = NULL;
> +			iores_range[0] += 1;
> +		}

You could get away without iores_range by having an extra
terminator entry in the iores_array.

Overally this starts to look good.

/Jarkko

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

[Jarkko Sakkinen]

On Thu, Oct 03, 2019 at 03:03:02PM +0300, Jarkko Sakkinen wrote:
> You could get away without iores_range by having an extra
> terminator entry in the iores_array.
> 
> Overally this starts to look good.

I'd also advice you to memset it with zeros so that you don't need to
have a variable for number of entries.

/Jarkko

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

[kbuild test robot]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jss-tpmdd/next]
[cannot apply to v5.4-rc3 next-20191011]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/ivan-lazeev-gmail-com/tpm_crb-fix-fTPM-on-AMD-Zen-CPUs/20191003-054300
base:   git://git.infradead.org/users/jjs/linux-tpmdd next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-rc1-43-g0ccb3b4-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/char/tpm/tpm_crb.c:507:62: sparse: sparse: Using plain integer as NULL pointer

vim +507 drivers/char/tpm/tpm_crb.c

   501	
   502	static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
   503			      struct acpi_table_tpm2 *buf)
   504	{
   505		struct list_head acpi_resource_list;
   506		struct resource iores_array[TPM_CRB_MAX_RESOURCES];
 > 507		void __iomem *iobase_array[TPM_CRB_MAX_RESOURCES] = {0};
   508		struct resource *iores_range[] = {
   509			iores_array,
   510			iores_array + TPM_CRB_MAX_RESOURCES
   511		};
   512		struct device *dev = &device->dev;
   513		struct resource *iores;
   514		void __iomem **iobase_ptr;
   515		int num_resources, i;
   516		u32 pa_high, pa_low;
   517		u64 cmd_pa;
   518		u32 cmd_size;
   519		__le64 __rsp_pa;
   520		u64 rsp_pa;
   521		u32 rsp_size;
   522		int ret;
   523	
   524		INIT_LIST_HEAD(&acpi_resource_list);
   525		ret = acpi_dev_get_resources(device, &acpi_resource_list,
   526					     crb_check_resource, iores_range);
   527		if (ret < 0)
   528			return ret;
   529		acpi_dev_free_resource_list(&acpi_resource_list);
   530	
   531		if (iores_range[0] == iores_array) {
   532			dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
   533			return -EINVAL;
   534		} else if (!iores_range[0]) {
   535			dev_warn(dev, "TPM2 ACPI table defines too many memory resources\n");
   536			iores_range[0] = iores_range[1];
   537		}
   538	
   539		num_resources = iores_range[0] - iores_array;
   540	
   541		iores = NULL;
   542		iobase_ptr = NULL;
   543		for (i = 0; i < num_resources; ++i) {
   544			if (buf->control_address >= iores_array[i].start &&
   545			    buf->control_address + sizeof(struct crb_regs_tail) - 1 <=
   546			    iores_array[i].end) {
   547				iores = iores_array + i;
   548				iobase_ptr = iobase_array + i;
   549				break;
   550			}
   551		}
   552	
   553		priv->regs_t = crb_map_res(dev, iores, iobase_ptr, buf->control_address,
   554					   sizeof(struct crb_regs_tail));
   555	
   556		if (IS_ERR(priv->regs_t))
   557			return PTR_ERR(priv->regs_t);
   558	
   559		/* The ACPI IO region starts at the head area and continues to include
   560		 * the control area, as one nice sane region except for some older
   561		 * stuff that puts the control area outside the ACPI IO region.
   562		 */
   563		if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
   564		    (priv->sm == ACPI_TPM2_MEMORY_MAPPED)) {
   565			if (iores &&
   566			    buf->control_address == iores->start +
   567			    sizeof(*priv->regs_h))
   568				priv->regs_h = *iobase_ptr;
   569			else
   570				dev_warn(dev, FW_BUG "Bad ACPI memory layout");
   571		}
   572	
   573		ret = __crb_request_locality(dev, priv, 0);
   574		if (ret)
   575			return ret;
   576	
   577		/*
   578		 * PTT HW bug w/a: wake up the device to access
   579		 * possibly not retained registers.
   580		 */
   581		ret = __crb_cmd_ready(dev, priv);
   582		if (ret)
   583			goto out_relinquish_locality;
   584	
   585		pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
   586		pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);
   587		cmd_pa = ((u64)pa_high << 32) | pa_low;
   588		cmd_size = ioread32(&priv->regs_t->ctrl_cmd_size);
   589	
   590		iores = NULL;
   591		iobase_ptr = NULL;
   592		for (i = 0; i < num_resources; ++i) {
   593			if (cmd_pa >= iores_array[i].start &&
   594			    cmd_pa <= iores_array[i].end) {
   595				iores = iores_array + i;
   596				iobase_ptr = iobase_array + i;
   597				break;
   598			}
   599		}
   600	
   601		if (iores)
   602			cmd_size = crb_fixup_cmd_size(dev, iores, cmd_pa, cmd_size);
   603	
   604		dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
   605			pa_high, pa_low, cmd_size);
   606	
   607		priv->cmd = crb_map_res(dev, iores, iobase_ptr,	cmd_pa, cmd_size);
   608		if (IS_ERR(priv->cmd)) {
   609			ret = PTR_ERR(priv->cmd);
   610			goto out;
   611		}
   612	
   613		memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
   614		rsp_pa = le64_to_cpu(__rsp_pa);
   615		rsp_size = ioread32(&priv->regs_t->ctrl_rsp_size);
   616	
   617		iores = NULL;
   618		iobase_ptr = NULL;
   619		for (i = 0; i < num_resources; ++i) {
   620			if (rsp_pa >= iores_array[i].start &&
   621			    rsp_pa <= iores_array[i].end) {
   622				iores = iores_array + i;
   623				iobase_ptr = iobase_array + i;
   624				break;
   625			}
   626		}
   627	
   628		if (iores)
   629			rsp_size = crb_fixup_cmd_size(dev, iores, rsp_pa, rsp_size);
   630	
   631		if (cmd_pa != rsp_pa) {
   632			priv->rsp = crb_map_res(dev, iores, iobase_ptr,
   633						rsp_pa, rsp_size);
   634			ret = PTR_ERR_OR_ZERO(priv->rsp);
   635			goto out;
   636		}
   637	
   638		/* According to the PTP specification, overlapping command and response
   639		 * buffer sizes must be identical.
   640		 */
   641		if (cmd_size != rsp_size) {
   642			dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical");
   643			ret = -EINVAL;
   644			goto out;
   645		}
   646	
   647		priv->rsp = priv->cmd;
   648	
   649	out:
   650		if (!ret)
   651			priv->cmd_size = cmd_size;
   652	
   653		__crb_go_idle(dev, priv);
   654	
   655	out_relinquish_locality:
   656	
   657		__crb_relinquish_locality(dev, priv, 0);
   658	
   659		return ret;
   660	}
   661	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

[Jarkko Sakkinen]

On Mon, Oct 14, 2019 at 04:32:20PM +0800, kbuild test robot wrote:
> Hi,
> 
> Thank you for the patch! Perhaps something to improve:

Please fix this and send v8 (with full change log).

/Jarkko