[PATCH] tpm_crb: fix mapping of the buffers

[PATCH] tpm_crb: fix mapping of the buffers

[Jarkko Sakkinen]

On my Lenovo x250 the following situation occurs:

[18697.813871] tpm_crb MSFT0101:00: can't request region for resource
[mem 0xacdff080-0xacdfffff]

The mapping of the control area overlaps the mapping of the command
buffer. The control area is mapped over page, which is not right. It
should mapped over sizeof(struct crb_control_area).

Fixing this issue unmasks another issue. Command and response buffers
can overlap and they do interleave on this machine. According to the PTP
specification the overlapping means that they are mapped to the same
buffer.

The commit has been also on a Haswell NUC where things worked before
applying this fix so that the both code paths for response buffer
initialization are tested.

Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Fixes: 1bd047be37d9 ("tpm_crb: Use devm_ioremap_resource")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm_crb.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 733cd0e..5afe684 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -259,7 +259,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	struct list_head resources;
 	struct resource io_res;
 	struct device *dev = &device->dev;
-	u64 pa;
+	u64 cmd_pa;
+	u32 cmd_size;
+	u64 rsp_pa;
+	u32 rsp_size;
 	int ret;
 
 	INIT_LIST_HEAD(&resources);
@@ -280,22 +283,36 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 		return PTR_ERR(priv->iobase);
 
 	priv->cca = crb_map_res(dev, priv, &io_res, buf->control_address,
-				0x1000);
+				sizeof(struct crb_control_area));
 	if (IS_ERR(priv->cca))
 		return PTR_ERR(priv->cca);
 
-	pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
-	      (u64) ioread32(&priv->cca->cmd_pa_low);
-	priv->cmd = crb_map_res(dev, priv, &io_res, pa,
-				ioread32(&priv->cca->cmd_size));
+	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
+		  (u64) ioread32(&priv->cca->cmd_pa_low);
+	cmd_size = ioread32(&priv->cca->cmd_size);
+	priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size);
 	if (IS_ERR(priv->cmd))
 		return PTR_ERR(priv->cmd);
 
-	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
-	pa = le64_to_cpu(pa);
-	priv->rsp = crb_map_res(dev, priv, &io_res, pa,
-				ioread32(&priv->cca->rsp_size));
-	return PTR_ERR_OR_ZERO(priv->rsp);
+	memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8);
+	rsp_pa = le64_to_cpu(rsp_pa);
+	rsp_size = ioread32(&priv->cca->rsp_size);
+
+	if (cmd_pa != rsp_pa) {
+		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
+		return PTR_ERR_OR_ZERO(priv->rsp);
+	}
+
+	/* According to the PTP specification, overlapping command and response
+	 * buffer sizes must be identical.
+	 */
+	if (cmd_size != rsp_size) {
+		dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical");
+		return -EINVAL;
+	}
+
+	priv->rsp = priv->cmd;
+	return 0;
 }
 
 static int crb_acpi_add(struct acpi_device *device)
-- 
2.7.4


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z

Re: [PATCH] tpm_crb: fix mapping of the buffers

[Jarkko Sakkinen]

On Tue, Apr 19, 2016 at 12:54:18PM +0300, Jarkko Sakkinen wrote:
> On my Lenovo x250 the following situation occurs:
> 
> [18697.813871] tpm_crb MSFT0101:00: can't request region for resource
> [mem 0xacdff080-0xacdfffff]
> 
> The mapping of the control area overlaps the mapping of the command
> buffer. The control area is mapped over page, which is not right. It
> should mapped over sizeof(struct crb_control_area).
> 
> Fixing this issue unmasks another issue. Command and response buffers
> can overlap and they do interleave on this machine. According to the PTP
> specification the overlapping means that they are mapped to the same
> buffer.
> 
> The commit has been also on a Haswell NUC where things worked before
> applying this fix so that the both code paths for response buffer
> initialization are tested.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1bd047be37d9 ("tpm_crb: Use devm_ioremap_resource")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Forgot to add --subject-prefix="PATCH v2", sorry.

/Jarkko

> ---
>  drivers/char/tpm/tpm_crb.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 733cd0e..5afe684 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -259,7 +259,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  	struct list_head resources;
>  	struct resource io_res;
>  	struct device *dev = &device->dev;
> -	u64 pa;
> +	u64 cmd_pa;
> +	u32 cmd_size;
> +	u64 rsp_pa;
> +	u32 rsp_size;
>  	int ret;
>  
>  	INIT_LIST_HEAD(&resources);
> @@ -280,22 +283,36 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  		return PTR_ERR(priv->iobase);
>  
>  	priv->cca = crb_map_res(dev, priv, &io_res, buf->control_address,
> -				0x1000);
> +				sizeof(struct crb_control_area));
>  	if (IS_ERR(priv->cca))
>  		return PTR_ERR(priv->cca);
>  
> -	pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
> -	      (u64) ioread32(&priv->cca->cmd_pa_low);
> -	priv->cmd = crb_map_res(dev, priv, &io_res, pa,
> -				ioread32(&priv->cca->cmd_size));
> +	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
> +		  (u64) ioread32(&priv->cca->cmd_pa_low);
> +	cmd_size = ioread32(&priv->cca->cmd_size);
> +	priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size);
>  	if (IS_ERR(priv->cmd))
>  		return PTR_ERR(priv->cmd);
>  
> -	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
> -	pa = le64_to_cpu(pa);
> -	priv->rsp = crb_map_res(dev, priv, &io_res, pa,
> -				ioread32(&priv->cca->rsp_size));
> -	return PTR_ERR_OR_ZERO(priv->rsp);
> +	memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8);
> +	rsp_pa = le64_to_cpu(rsp_pa);
> +	rsp_size = ioread32(&priv->cca->rsp_size);
> +
> +	if (cmd_pa != rsp_pa) {
> +		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
> +		return PTR_ERR_OR_ZERO(priv->rsp);
> +	}
> +
> +	/* According to the PTP specification, overlapping command and response
> +	 * buffer sizes must be identical.
> +	 */
> +	if (cmd_size != rsp_size) {
> +		dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical");
> +		return -EINVAL;
> +	}
> +
> +	priv->rsp = priv->cmd;
> +	return 0;
>  }
>  
>  static int crb_acpi_add(struct acpi_device *device)
> -- 
> 2.7.4
> 

Re: [PATCH] tpm_crb: fix mapping of the buffers

[Jason Gunthorpe]

On Tue, Apr 19, 2016 at 12:54:18PM +0300, Jarkko Sakkinen wrote:
> Cc: stable@vger.kernel.org
> Fixes: 1bd047be37d9 ("tpm_crb: Use devm_ioremap_resource")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>  drivers/char/tpm/tpm_crb.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)

This looks OK

Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

> +	if (cmd_pa != rsp_pa) {
> +		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
> +		return PTR_ERR_OR_ZERO(priv->rsp);
> +	}

I would use an else here, 'exit on success' is considered an
anti-pattern.

Eg:

if (cmd_pa == rsp_pa) {
	/* According to the PTP specification, overlapping command and response
	 * buffer sizes must be identical.
	 */
	if (cmd_size != rsp_size) {
		dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical");
		return -EINVAL;
	}
	priv->rsp = priv->cmd;
}
else {
	priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
        if (IS_ERR(priv->rsp))
	   	return PTR_ERR(priv->rsp);
}

return 0;


Jason

Re: [PATCH] tpm_crb: fix mapping of the buffers

[Jarkko Sakkinen]

On Tue, Apr 19, 2016 at 11:09:53AM -0600, Jason Gunthorpe wrote:
> On Tue, Apr 19, 2016 at 12:54:18PM +0300, Jarkko Sakkinen wrote:
> > Cc: stable@vger.kernel.org
> > Fixes: 1bd047be37d9 ("tpm_crb: Use devm_ioremap_resource")
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >  drivers/char/tpm/tpm_crb.c | 39 ++++++++++++++++++++++++++++-----------
> >  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> This looks OK
> 
> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Thanks!

> > +	if (cmd_pa != rsp_pa) {
> > +		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
> > +		return PTR_ERR_OR_ZERO(priv->rsp);
> > +	}
> 
> I would use an else here, 'exit on success' is considered an
> anti-pattern.

> Eg:
> 
> if (cmd_pa == rsp_pa) {
> 	/* According to the PTP specification, overlapping command and response
> 	 * buffer sizes must be identical.
> 	 */
> 	if (cmd_size != rsp_size) {
> 		dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical");
> 		return -EINVAL;
> 	}
> 	priv->rsp = priv->cmd;
> }
> else {
> 	priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
>         if (IS_ERR(priv->rsp))
> 	   	return PTR_ERR(priv->rsp);
> }
> 
> return 0;

I have to (in order to do right things right):

1. Update the patch.
2. Smoke test with the machines that I have.
3. Send a new version for review (because of the revamped control flow).

It's not that I wouldn't be willing to do this. I just don't think
this matters enough to be worth of the trouble.

> Jason

/Jarkko

[PATCH] tpm_crb: fix mapping of the buffers

[Jarkko Sakkinen]

On my Lenovo x250 the following situation occurs:

[18697.813871] tpm_crb MSFT0101:00: can't request region for resource
[mem 0xacdff080-0xacdfffff]

The mapping of the control area interleaves the mapping of the command
buffer. The control area is mapped over page, which is not right. It
should mapped over sizeof(struct crb_control_area).

Fixing this issue unmasks another issue. Command and response buffers
can interleave and they do interleave on this machine.

This commit changes driver to check that the new resource does not
interleave any of the previously mapped resources. If interleaving
happens, the existing mapping is used.

I've also tested this patch on a Haswell NUC where things worked before
applying this fix.

Cc: stable@vger.kernel.org
Fixes: 1bd047be37d9 ("tpm_crb: Use devm_ioremap_resource")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 77 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 56 insertions(+), 21 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 733cd0e..c957d85 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -75,9 +75,18 @@ enum crb_flags {
 	CRB_FL_CRB_START	= BIT(1),
 };
 
+enum crb_res {
+	CRB_RES_IOMEM,
+	CRB_RES_CONTROL,
+	CRB_RES_COMMAND,
+	CRB_RES_RESPONSE,
+	CRB_NR_RESOURCES
+};
+
 struct crb_priv {
 	unsigned int flags;
-	void __iomem *iobase;
+	struct resource res[CRB_NR_RESOURCES];
+	void __iomem *res_ptr[CRB_NR_RESOURCES];
 	struct crb_control_area __iomem *cca;
 	u8 __iomem *cmd;
 	u8 __iomem *rsp;
@@ -234,9 +243,12 @@ static int crb_check_resource(struct acpi_resource *ares, void *data)
 	return 1;
 }
 
-static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
-				 struct resource *io_res, u64 start, u32 size)
+static int crb_map_res(struct device *dev, struct crb_priv *priv,
+		       int res_i, u64 start, u32 size)
 {
+	u8 __iomem *ptr;
+	int i;
+
 	struct resource new_res = {
 		.start	= start,
 		.end	= start + size - 1,
@@ -245,12 +257,25 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
 
 	/* Detect a 64 bit address on a 32 bit system */
 	if (start != new_res.start)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 
-	if (!resource_contains(io_res, &new_res))
-		return devm_ioremap_resource(dev, &new_res);
+	for (i = 0; i < CRB_NR_RESOURCES; i++) {
+		if (resource_contains(&priv->res[i], &new_res)) {
+			priv->res[res_i] = new_res;
+			priv->res_ptr[res_i] = priv->res_ptr[i] +
+				(new_res.start - priv->res[i].start);
+			return 0;
+		}
+	}
 
-	return priv->iobase + (new_res.start - io_res->start);
+	ptr = devm_ioremap_resource(dev, &new_res);
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
+
+	priv->res[res_i] = new_res;
+	priv->res_ptr[res_i] = ptr;
+
+	return 0;
 }
 
 static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
@@ -275,27 +300,37 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 		return -EINVAL;
 	}
 
-	priv->iobase = devm_ioremap_resource(dev, &io_res);
-	if (IS_ERR(priv->iobase))
-		return PTR_ERR(priv->iobase);
+	ret = crb_map_res(dev, priv, CRB_RES_IOMEM, io_res.start,
+			  io_res.end - io_res.start + 1);
+	if (ret)
+		return ret;
 
-	priv->cca = crb_map_res(dev, priv, &io_res, buf->control_address,
-				0x1000);
-	if (IS_ERR(priv->cca))
-		return PTR_ERR(priv->cca);
+	ret = crb_map_res(dev, priv, CRB_RES_CONTROL, buf->control_address,
+			  sizeof(struct crb_control_area));
+	if (ret)
+		return ret;
+
+	priv->cca = priv->res_ptr[CRB_RES_CONTROL];
 
 	pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
 	      (u64) ioread32(&priv->cca->cmd_pa_low);
-	priv->cmd = crb_map_res(dev, priv, &io_res, pa,
-				ioread32(&priv->cca->cmd_size));
-	if (IS_ERR(priv->cmd))
-		return PTR_ERR(priv->cmd);
+	ret = crb_map_res(dev, priv, CRB_RES_COMMAND, pa,
+			  ioread32(&priv->cca->cmd_size));
+	if (ret)
+		return ret;
+
+	priv->cmd = priv->res_ptr[CRB_RES_COMMAND];
 
 	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
 	pa = le64_to_cpu(pa);
-	priv->rsp = crb_map_res(dev, priv, &io_res, pa,
-				ioread32(&priv->cca->rsp_size));
-	return PTR_ERR_OR_ZERO(priv->rsp);
+	ret = crb_map_res(dev, priv, CRB_RES_RESPONSE, pa,
+			  ioread32(&priv->cca->rsp_size));
+	if (ret)
+		return ret;
+
+	priv->rsp = priv->res_ptr[CRB_RES_RESPONSE];
+
+	return 0;
 }
 
 static int crb_acpi_add(struct acpi_device *device)
-- 
2.7.4

Re: [PATCH] tpm_crb: fix mapping of the buffers

[Jason Gunthorpe]

On Tue, Apr 19, 2016 at 02:08:00AM +0300, Jarkko Sakkinen wrote:
> On my Lenovo x250 the following situation occurs:
> 
> [18697.813871] tpm_crb MSFT0101:00: can't request region for resource
> [mem 0xacdff080-0xacdfffff]

Sigh, the BIOS vendors seem to be screwing this up a lot.. No doubt
because the spec doesn't really say what to do very well...

> The mapping of the control area interleaves the mapping of the command
> buffer. The control area is mapped over page, which is not right. It
> should mapped over sizeof(struct crb_control_area).

Good

> Fixing this issue unmasks another issue. Command and response buffers
> can interleave and they do interleave on this machine.

Do they 100% overlap because one is 'read' and the other is 'write'?

Or did the BIOS guys screw up the length of one of the regions, and
they were supposed to be back to back? In which case it is just luck
this proposed patch solves the issue :(

The request_io stuff is there specifically to prevent two peices of
code from trying to control the same registers, I'm really reluctant to
work-around it like this, though granted, crazy BIOS is a fine good reason.

Is this patch below enough to deal with it sanely?

If you do stick with this then:

> -static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
> -				 struct resource *io_res, u64 start, u32 size)
> +static int crb_map_res(struct device *dev, struct crb_priv *priv,
> +		       int res_i, u64 start, u32 size)

I wouldn't change the signature at all, just add a counter to the priv and
'append to the list'

This change is creating a lot of needless churn which is not good at
all for the stable rules.

Removing the pointer return is not an improvement..

>  {
> +	u8 __iomem *ptr;
> +	int i;
> +
>  	struct resource new_res = {
>  		.start	= start,
>  		.end	= start + size - 1,
> @@ -245,12 +257,25 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
>  
>  	/* Detect a 64 bit address on a 32 bit system */
>  	if (start != new_res.start)
> -		return ERR_PTR(-EINVAL);
> +		return -EINVAL;
>  
> -	if (!resource_contains(io_res, &new_res))
> -		return devm_ioremap_resource(dev, &new_res);
> +	for (i = 0; i < CRB_NR_RESOURCES; i++) {
> +		if (resource_contains(&priv->res[i], &new_res)) {
> +			priv->res[res_i] = new_res;
> +			priv->res_ptr[res_i] = priv->res_ptr[i] +
> +				(new_res.start - priv->res[i].start);
> +			return 0;
> +		}
> +	}


Just add:

   id = priv->num_res++;
   priv->res[id] = *io_res;
   priv->res_ptr[id] = priv->iobase + (new_res.start  - io_res->start);
   return priv->res_ptr[id];

And drop all the other hunks, except for the sizeof change and the
above loop.

Maybe print a FW bug if it overlaps with id != 0, that is just
crazy beans.

Jason

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 20155d55a62b..0a87c813d004 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -253,7 +253,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	struct list_head resources;
 	struct resource io_res;
 	struct device *dev = &device->dev;
-	u64 pa;
+	u64 cmd_pa,rsp_pa;
 	int ret;
 
 	INIT_LIST_HEAD(&resources);
@@ -274,22 +274,33 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 		return PTR_ERR(priv->iobase);
 
 	priv->cca = crb_map_res(dev, priv, &io_res, buf->control_address,
-				0x1000);
+				sizeof(*priv->cca));
 	if (IS_ERR(priv->cca))
 		return PTR_ERR(priv->cca);
 
-	pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
-	      (u64) ioread32(&priv->cca->cmd_pa_low);
-	priv->cmd = crb_map_res(dev, priv, &io_res, pa,
-				ioread32(&priv->cca->cmd_size));
-	if (IS_ERR(priv->cmd))
-		return PTR_ERR(priv->cmd);
-
+	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
+		(u64) ioread32(&priv->cca->cmd_pa_low);
 	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
-	pa = le64_to_cpu(pa);
-	priv->rsp = crb_map_res(dev, priv, &io_res, pa,
-				ioread32(&priv->cca->rsp_size));
-	return PTR_ERR_OR_ZERO(priv->rsp);
+	rsp_pa = le64_to_cpu(pa);
+
+	if (cmd_pa == rsp_pa) {
+		u32 len = max_t(size_t,ioread32(&priv->cca->cmd_size),
+				   ioread32(&priv->cca->rsp_size));
+		priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, len);
+		if (IS_ERR(priv->cmd))
+			return PTR_ERR(priv->cmd);
+		priv->rsp = priv->cmd;
+	} else {
+		priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa,
+					ioread32(&priv->cca->rsp_size));
+		if (IS_ERR(priv->cmd))
+			return PTR_ERR(priv->cmd);
+		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa,
+					ioread32(&priv->cca->rsp_size));
+		if (IS_ERR(priv->rsp))
+			return PTR_ERR(priv->rsp);
+	}
+	return 0;
 }
 
 static int crb_acpi_add(struct acpi_device *device)

Re: [PATCH] tpm_crb: fix mapping of the buffers

[Jarkko Sakkinen]

On Mon, Apr 18, 2016 at 05:34:57PM -0600, Jason Gunthorpe wrote:
> On Tue, Apr 19, 2016 at 02:08:00AM +0300, Jarkko Sakkinen wrote:
> > On my Lenovo x250 the following situation occurs:
> > 
> > [18697.813871] tpm_crb MSFT0101:00: can't request region for resource
> > [mem 0xacdff080-0xacdfffff]
> 
> Sigh, the BIOS vendors seem to be screwing this up a lot.. No doubt
> because the spec doesn't really say what to do very well...
> 
> > The mapping of the control area interleaves the mapping of the command
> > buffer. The control area is mapped over page, which is not right. It
> > should mapped over sizeof(struct crb_control_area).
> 
> Good
> 
> > Fixing this issue unmasks another issue. Command and response buffers
> > can interleave and they do interleave on this machine.
> 
> Do they 100% overlap because one is 'read' and the other is 'write'?

100% so it is kind of sane configuration.

> Or did the BIOS guys screw up the length of one of the regions, and
> they were supposed to be back to back? In which case it is just luck
> this proposed patch solves the issue :(
> 
> The request_io stuff is there specifically to prevent two peices of
> code from trying to control the same registers, I'm really reluctant to
> work-around it like this, though granted, crazy BIOS is a fine good reason.
> 
> Is this patch below enough to deal with it sanely?
> 
> If you do stick with this then:
> 
> > -static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
> > -				 struct resource *io_res, u64 start, u32 size)
> > +static int crb_map_res(struct device *dev, struct crb_priv *priv,
> > +		       int res_i, u64 start, u32 size)
> 
> I wouldn't change the signature at all, just add a counter to the priv and
> 'append to the list'
> 
> This change is creating a lot of needless churn which is not good at
> all for the stable rules.
> 
> Removing the pointer return is not an improvement..

Point taken but then it is better to make the fix even more localized. If
the physical addresses are equal, check the buffer sizes for sanity.
If they are not equal, emit FW_BUG and return -EINVAL.

I'll send an updated patch after I've done all testing rounds with my
laptop and Haswell NUC.

/Jarkko

> >  {
> > +	u8 __iomem *ptr;
> > +	int i;
> > +
> >  	struct resource new_res = {
> >  		.start	= start,
> >  		.end	= start + size - 1,
> > @@ -245,12 +257,25 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
> >  
> >  	/* Detect a 64 bit address on a 32 bit system */
> >  	if (start != new_res.start)
> > -		return ERR_PTR(-EINVAL);
> > +		return -EINVAL;
> >  
> > -	if (!resource_contains(io_res, &new_res))
> > -		return devm_ioremap_resource(dev, &new_res);
> > +	for (i = 0; i < CRB_NR_RESOURCES; i++) {
> > +		if (resource_contains(&priv->res[i], &new_res)) {
> > +			priv->res[res_i] = new_res;
> > +			priv->res_ptr[res_i] = priv->res_ptr[i] +
> > +				(new_res.start - priv->res[i].start);
> > +			return 0;
> > +		}
> > +	}
> 
> 
> Just add:
> 
>    id = priv->num_res++;
>    priv->res[id] = *io_res;
>    priv->res_ptr[id] = priv->iobase + (new_res.start  - io_res->start);
>    return priv->res_ptr[id];
> 
> And drop all the other hunks, except for the sizeof change and the
> above loop.
> 
> Maybe print a FW bug if it overlaps with id != 0, that is just
> crazy beans.
> 
> Jason
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 20155d55a62b..0a87c813d004 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -253,7 +253,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  	struct list_head resources;
>  	struct resource io_res;
>  	struct device *dev = &device->dev;
> -	u64 pa;
> +	u64 cmd_pa,rsp_pa;
>  	int ret;
>  
>  	INIT_LIST_HEAD(&resources);
> @@ -274,22 +274,33 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  		return PTR_ERR(priv->iobase);
>  
>  	priv->cca = crb_map_res(dev, priv, &io_res, buf->control_address,
> -				0x1000);
> +				sizeof(*priv->cca));
>  	if (IS_ERR(priv->cca))
>  		return PTR_ERR(priv->cca);
>  
> -	pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
> -	      (u64) ioread32(&priv->cca->cmd_pa_low);
> -	priv->cmd = crb_map_res(dev, priv, &io_res, pa,
> -				ioread32(&priv->cca->cmd_size));
> -	if (IS_ERR(priv->cmd))
> -		return PTR_ERR(priv->cmd);
> -
> +	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
> +		(u64) ioread32(&priv->cca->cmd_pa_low);
>  	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
> -	pa = le64_to_cpu(pa);
> -	priv->rsp = crb_map_res(dev, priv, &io_res, pa,
> -				ioread32(&priv->cca->rsp_size));
> -	return PTR_ERR_OR_ZERO(priv->rsp);
> +	rsp_pa = le64_to_cpu(pa);
> +
> +	if (cmd_pa == rsp_pa) {
> +		u32 len = max_t(size_t,ioread32(&priv->cca->cmd_size),
> +				   ioread32(&priv->cca->rsp_size));
> +		priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, len);
> +		if (IS_ERR(priv->cmd))
> +			return PTR_ERR(priv->cmd);
> +		priv->rsp = priv->cmd;
> +	} else {
> +		priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa,
> +					ioread32(&priv->cca->rsp_size));
> +		if (IS_ERR(priv->cmd))
> +			return PTR_ERR(priv->cmd);
> +		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa,
> +					ioread32(&priv->cca->rsp_size));
> +		if (IS_ERR(priv->rsp))
> +			return PTR_ERR(priv->rsp);
> +	}
> +	return 0;
>  }
>  
>  static int crb_acpi_add(struct acpi_device *device)

Re: [PATCH] tpm_crb: fix mapping of the buffers

[Jarkko Sakkinen]

On Tue, Apr 19, 2016 at 07:59:24AM +0300, Jarkko Sakkinen wrote:
> On Mon, Apr 18, 2016 at 05:34:57PM -0600, Jason Gunthorpe wrote:
> > On Tue, Apr 19, 2016 at 02:08:00AM +0300, Jarkko Sakkinen wrote:
> > > On my Lenovo x250 the following situation occurs:
> > > 
> > > [18697.813871] tpm_crb MSFT0101:00: can't request region for resource
> > > [mem 0xacdff080-0xacdfffff]
> > 
> > Sigh, the BIOS vendors seem to be screwing this up a lot.. No doubt
> > because the spec doesn't really say what to do very well...
> > 
> > > The mapping of the control area interleaves the mapping of the command
> > > buffer. The control area is mapped over page, which is not right. It
> > > should mapped over sizeof(struct crb_control_area).
> > 
> > Good
> > 
> > > Fixing this issue unmasks another issue. Command and response buffers
> > > can interleave and they do interleave on this machine.
> > 
> > Do they 100% overlap because one is 'read' and the other is 'write'?
> 
> 100% so it is kind of sane configuration.
> 
> > Or did the BIOS guys screw up the length of one of the regions, and
> > they were supposed to be back to back? In which case it is just luck
> > this proposed patch solves the issue :(
> > 
> > The request_io stuff is there specifically to prevent two peices of
> > code from trying to control the same registers, I'm really reluctant to
> > work-around it like this, though granted, crazy BIOS is a fine good reason.
> > 
> > Is this patch below enough to deal with it sanely?
> > 
> > If you do stick with this then:
> > 
> > > -static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
> > > -				 struct resource *io_res, u64 start, u32 size)
> > > +static int crb_map_res(struct device *dev, struct crb_priv *priv,
> > > +		       int res_i, u64 start, u32 size)
> > 
> > I wouldn't change the signature at all, just add a counter to the priv and
> > 'append to the list'
> > 
> > This change is creating a lot of needless churn which is not good at
> > all for the stable rules.
> > 
> > Removing the pointer return is not an improvement..
> 
> Point taken but then it is better to make the fix even more localized. If
> the physical addresses are equal, check the buffer sizes for sanity.
> If they are not equal, emit FW_BUG and return -EINVAL.

This is also correct way to handle the situation according to

http://www.trustedcomputinggroup.org/resources/pc_client_platform_tpm_profile_ptp_specification

In the table in the section 5.5.3.1 it is stated about rsp_size
that:

"Note:: If command and response buffers are implemented as a single
buffer, this field SHALL be identical to the value in the
TPM_CRB_CTRL_CMD_SIZE_x buffer."

/Jarkko

> I'll send an updated patch after I've done all testing rounds with my
> laptop and Haswell NUC.
> 
> /Jarkko
> 
> > >  {
> > > +	u8 __iomem *ptr;
> > > +	int i;
> > > +
> > >  	struct resource new_res = {
> > >  		.start	= start,
> > >  		.end	= start + size - 1,
> > > @@ -245,12 +257,25 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
> > >  
> > >  	/* Detect a 64 bit address on a 32 bit system */
> > >  	if (start != new_res.start)
> > > -		return ERR_PTR(-EINVAL);
> > > +		return -EINVAL;
> > >  
> > > -	if (!resource_contains(io_res, &new_res))
> > > -		return devm_ioremap_resource(dev, &new_res);
> > > +	for (i = 0; i < CRB_NR_RESOURCES; i++) {
> > > +		if (resource_contains(&priv->res[i], &new_res)) {
> > > +			priv->res[res_i] = new_res;
> > > +			priv->res_ptr[res_i] = priv->res_ptr[i] +
> > > +				(new_res.start - priv->res[i].start);
> > > +			return 0;
> > > +		}
> > > +	}
> > 
> > 
> > Just add:
> > 
> >    id = priv->num_res++;
> >    priv->res[id] = *io_res;
> >    priv->res_ptr[id] = priv->iobase + (new_res.start  - io_res->start);
> >    return priv->res_ptr[id];
> > 
> > And drop all the other hunks, except for the sizeof change and the
> > above loop.
> > 
> > Maybe print a FW bug if it overlaps with id != 0, that is just
> > crazy beans.
> > 
> > Jason
> > 
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index 20155d55a62b..0a87c813d004 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -253,7 +253,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
> >  	struct list_head resources;
> >  	struct resource io_res;
> >  	struct device *dev = &device->dev;
> > -	u64 pa;
> > +	u64 cmd_pa,rsp_pa;
> >  	int ret;
> >  
> >  	INIT_LIST_HEAD(&resources);
> > @@ -274,22 +274,33 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
> >  		return PTR_ERR(priv->iobase);
> >  
> >  	priv->cca = crb_map_res(dev, priv, &io_res, buf->control_address,
> > -				0x1000);
> > +				sizeof(*priv->cca));
> >  	if (IS_ERR(priv->cca))
> >  		return PTR_ERR(priv->cca);
> >  
> > -	pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
> > -	      (u64) ioread32(&priv->cca->cmd_pa_low);
> > -	priv->cmd = crb_map_res(dev, priv, &io_res, pa,
> > -				ioread32(&priv->cca->cmd_size));
> > -	if (IS_ERR(priv->cmd))
> > -		return PTR_ERR(priv->cmd);
> > -
> > +	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
> > +		(u64) ioread32(&priv->cca->cmd_pa_low);
> >  	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
> > -	pa = le64_to_cpu(pa);
> > -	priv->rsp = crb_map_res(dev, priv, &io_res, pa,
> > -				ioread32(&priv->cca->rsp_size));
> > -	return PTR_ERR_OR_ZERO(priv->rsp);
> > +	rsp_pa = le64_to_cpu(pa);
> > +
> > +	if (cmd_pa == rsp_pa) {
> > +		u32 len = max_t(size_t,ioread32(&priv->cca->cmd_size),
> > +				   ioread32(&priv->cca->rsp_size));
> > +		priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, len);
> > +		if (IS_ERR(priv->cmd))
> > +			return PTR_ERR(priv->cmd);
> > +		priv->rsp = priv->cmd;
> > +	} else {
> > +		priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa,
> > +					ioread32(&priv->cca->rsp_size));
> > +		if (IS_ERR(priv->cmd))
> > +			return PTR_ERR(priv->cmd);
> > +		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa,
> > +					ioread32(&priv->cca->rsp_size));
> > +		if (IS_ERR(priv->rsp))
> > +			return PTR_ERR(priv->rsp);
> > +	}
> > +	return 0;
> >  }
> >  
> >  static int crb_acpi_add(struct acpi_device *device)