Linux-Integrity Archive on lore.kernel.org
 help / Atom feed
* [PATCH v2] tpm/tpm_crb: Avoid unaligned reads in crb_recv()
@ 2019-02-04 13:59 Jarkko Sakkinen
  2019-02-04 17:12 ` David Laight
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2019-02-04 13:59 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Jarkko Sakkinen, stable,
	James Morris, Tomas Winkler, Jerry Snitselaar

The current approach to read first 6 bytes from the response and then tail
of the response, can cause the 2nd memcpy_fromio() to do an unaligned read
(e.g. read 32-bit word from address aligned to a 16-bits), depending on how
memcpy_fromio() is implemented. If this happens, the read will fail and the
memory controller will fill the read with 1's.

This was triggered by 170d13ca3a2f, which should be probably refined to
check and react to the address alignment. Before that commit, on x86
memcpy_fromio() turned out to be memcpy(). By a luck GCC has done the right
thing (from tpm_crb's perspective) for us so far, but we should not rely on
that. Thus, it makes sense to fix this also in tpm_crb, not least because
the fix can be then backported to stable kernels and make them more robust
when compiled in differing environments.

Cc: stable@vger.kernel.org
Cc: James Morris <jmorris@namei.org>
Cc: Tomas Winkler <tomas.winkler@intel.com>
Cc: Jerry Snitselaar <jsnitsel@redhat.com>
Fixes: 30fc8d138e91 ("tpm: TPM 2.0 CRB Interface")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
---
v2:
* There was a trailing double colon in the end of the short summary.
* Check requested and expected length against TPM_HEADER_SIZE.
* Add some explanatory comments to crb_recv().
 drivers/char/tpm/tpm_crb.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 36952ef98f90..c084e61299aa 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -287,19 +287,29 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 	unsigned int expected;
 
-	/* sanity check */
-	if (count < 6)
+	/* A sanity check that the upper layer wants to get at least the header
+	 * as that is the minimum size for any TPM response.
+	 */
+	if (count < TPM_HEADER_SIZE)
 		return -EIO;
 
+	/* If this bit is set, according to the spec, the TPM is in unrecovable
+	 * condition.
+	 */
 	if (ioread32(&priv->regs_t->ctrl_sts) & CRB_CTRL_STS_ERROR)
 		return -EIO;
 
-	memcpy_fromio(buf, priv->rsp, 6);
-	expected = be32_to_cpup((__be32 *) &buf[2]);
-	if (expected > count || expected < 6)
+	/* Read 8 bytes (not just 6 bytes, which would cover the response length
+	 * field) in order to make sure that the reminding memory accesses will
+	 * be aligned.
+	 */
+	memcpy_fromio(buf, priv->rsp, 8);
+
+	expected = be32_to_cpup((__be32 *)&buf[2]);
+	if (expected > count || expected < TPM_HEADER_SIZE)
 		return -EIO;
 
-	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
+	memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);
 
 	return expected;
 }
-- 
2.19.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v2] tpm/tpm_crb: Avoid unaligned reads in crb_recv()
  2019-02-04 13:59 [PATCH v2] tpm/tpm_crb: Avoid unaligned reads in crb_recv() Jarkko Sakkinen
@ 2019-02-04 17:12 ` David Laight
  2019-02-04 23:19   ` Jarkko Sakkinen
  2019-02-04 22:09 ` Winkler, Tomas
       [not found] ` <20190207163157.DA4A62175B@mail.kernel.org>
  2 siblings, 1 reply; 6+ messages in thread
From: David Laight @ 2019-02-04 17:12 UTC (permalink / raw)
  To: Jarkko Sakkinen, linux-integrity
  Cc: linux-kernel, linux-security-module, stable, James Morris,
	Tomas Winkler, Jerry Snitselaar

> +	 * field) in order to make sure that the reminding memory accesses will

remaining

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v2] tpm/tpm_crb: Avoid unaligned reads in crb_recv()
  2019-02-04 13:59 [PATCH v2] tpm/tpm_crb: Avoid unaligned reads in crb_recv() Jarkko Sakkinen
  2019-02-04 17:12 ` David Laight
@ 2019-02-04 22:09 ` Winkler, Tomas
  2019-02-04 23:24   ` Jarkko Sakkinen
       [not found] ` <20190207163157.DA4A62175B@mail.kernel.org>
  2 siblings, 1 reply; 6+ messages in thread
From: Winkler, Tomas @ 2019-02-04 22:09 UTC (permalink / raw)
  To: Jarkko Sakkinen, linux-integrity
  Cc: linux-kernel, linux-security-module, stable, James Morris,
	Jerry Snitselaar

> 
> The current approach to read first 6 bytes from the response and then tail of
> the response, can cause the 2nd memcpy_fromio() to do an unaligned read
> (e.g. read 32-bit word from address aligned to a 16-bits), depending on how
> memcpy_fromio() is implemented. If this happens, the read will fail and the
> memory controller will fill the read with 1's.
> 
> This was triggered by 170d13ca3a2f, which should be probably refined to check
> and react to the address alignment. Before that commit, on x86
> memcpy_fromio() turned out to be memcpy(). By a luck GCC has done the right
> thing (from tpm_crb's perspective) for us so far, but we should not rely on that.
> Thus, it makes sense to fix this also in tpm_crb, not least because the fix can be
> then backported to stable kernels and make them more robust when compiled
> in differing environments.
> 
> Cc: stable@vger.kernel.org
> Cc: James Morris <jmorris@namei.org>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Cc: Jerry Snitselaar <jsnitsel@redhat.com>
> Fixes: 30fc8d138e91 ("tpm: TPM 2.0 CRB Interface")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
After fixing the typos you can add my ack. 
Thanks
Tomas

> ---
> v2:
> * There was a trailing double colon in the end of the short summary.
> * Check requested and expected length against TPM_HEADER_SIZE.
> * Add some explanatory comments to crb_recv().
>  drivers/char/tpm/tpm_crb.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index
> 36952ef98f90..c084e61299aa 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -287,19 +287,29 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf,
> size_t count)
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>  	unsigned int expected;
> 
> -	/* sanity check */
> -	if (count < 6)
> +	/* A sanity check that the upper layer wants to get at least the header
> +	 * as that is the minimum size for any TPM response.
> +	 */
> +	if (count < TPM_HEADER_SIZE)
>  		return -EIO;
> 
> +	/* If this bit is set, according to the spec, the TPM is in unrecovable
                                                                                                     ^^^ typo ^^^^
> +	 * condition.
> +	 */
>  	if (ioread32(&priv->regs_t->ctrl_sts) & CRB_CTRL_STS_ERROR)
>  		return -EIO;
> 
> -	memcpy_fromio(buf, priv->rsp, 6);
> -	expected = be32_to_cpup((__be32 *) &buf[2]);
> -	if (expected > count || expected < 6)
> +	/* Read 8 bytes (not just 6 bytes, which would cover ^^^ tag and^^^ the response
> length
> +	 * field ^^^s^^^) in order to make sure that the reminding memory accesses
                                                                             ^^^ remaining^^^  
> will
> +	 * be aligned.
> +	 */
> +	memcpy_fromio(buf, priv->rsp, 8);
> +
> +	expected = be32_to_cpup((__be32 *)&buf[2]);
> +	if (expected > count || expected < TPM_HEADER_SIZE)
>  		return -EIO;
> 
> -	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
> +	memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);
> 
>  	return expected;
>  }
> --
> 2.19.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] tpm/tpm_crb: Avoid unaligned reads in crb_recv()
  2019-02-04 17:12 ` David Laight
@ 2019-02-04 23:19   ` Jarkko Sakkinen
  0 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2019-02-04 23:19 UTC (permalink / raw)
  To: David Laight
  Cc: linux-integrity, linux-kernel, linux-security-module, stable,
	James Morris, Tomas Winkler, Jerry Snitselaar

On Mon, Feb 04, 2019 at 05:12:57PM +0000, David Laight wrote:
> > +	 * field) in order to make sure that the reminding memory accesses will
> 
> remaining

Thanks for your feedback David. I'll implement your suggestions
(also in your other response).

/Jarkko

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] tpm/tpm_crb: Avoid unaligned reads in crb_recv()
  2019-02-04 22:09 ` Winkler, Tomas
@ 2019-02-04 23:24   ` Jarkko Sakkinen
  0 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2019-02-04 23:24 UTC (permalink / raw)
  To: Winkler, Tomas
  Cc: linux-integrity, linux-kernel, linux-security-module, stable,
	James Morris, Jerry Snitselaar

On Mon, Feb 04, 2019 at 10:09:51PM +0000, Winkler, Tomas wrote:
> > 
> > The current approach to read first 6 bytes from the response and then tail of
> > the response, can cause the 2nd memcpy_fromio() to do an unaligned read
> > (e.g. read 32-bit word from address aligned to a 16-bits), depending on how
> > memcpy_fromio() is implemented. If this happens, the read will fail and the
> > memory controller will fill the read with 1's.
> > 
> > This was triggered by 170d13ca3a2f, which should be probably refined to check
> > and react to the address alignment. Before that commit, on x86
> > memcpy_fromio() turned out to be memcpy(). By a luck GCC has done the right
> > thing (from tpm_crb's perspective) for us so far, but we should not rely on that.
> > Thus, it makes sense to fix this also in tpm_crb, not least because the fix can be
> > then backported to stable kernels and make them more robust when compiled
> > in differing environments.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: James Morris <jmorris@namei.org>
> > Cc: Tomas Winkler <tomas.winkler@intel.com>
> > Cc: Jerry Snitselaar <jsnitsel@redhat.com>
> > Fixes: 30fc8d138e91 ("tpm: TPM 2.0 CRB Interface")
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> After fixing the typos you can add my ack. 
> Thanks
> Tomas
> 
> > ---
> > v2:
> > * There was a trailing double colon in the end of the short summary.
> > * Check requested and expected length against TPM_HEADER_SIZE.
> > * Add some explanatory comments to crb_recv().
> >  drivers/char/tpm/tpm_crb.c | 22 ++++++++++++++++------
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index
> > 36952ef98f90..c084e61299aa 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -287,19 +287,29 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf,
> > size_t count)
> >  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >  	unsigned int expected;
> > 
> > -	/* sanity check */
> > -	if (count < 6)
> > +	/* A sanity check that the upper layer wants to get at least the header
> > +	 * as that is the minimum size for any TPM response.
> > +	 */
> > +	if (count < TPM_HEADER_SIZE)
> >  		return -EIO;
> > 
> > +	/* If this bit is set, according to the spec, the TPM is in unrecovable
>                                                                                                      ^^^ typo ^^^^
> > +	 * condition.
> > +	 */
> >  	if (ioread32(&priv->regs_t->ctrl_sts) & CRB_CTRL_STS_ERROR)
> >  		return -EIO;
> > 
> > -	memcpy_fromio(buf, priv->rsp, 6);
> > -	expected = be32_to_cpup((__be32 *) &buf[2]);
> > -	if (expected > count || expected < 6)
> > +	/* Read 8 bytes (not just 6 bytes, which would cover ^^^ tag and^^^ the response
> > length
> > +	 * field ^^^s^^^) in order to make sure that the reminding memory accesses
>                                                                              ^^^ remaining^^^  
> > will
> > +	 * be aligned.
> > +	 */
> > +	memcpy_fromio(buf, priv->rsp, 8);
> > +
> > +	expected = be32_to_cpup((__be32 *)&buf[2]);
> > +	if (expected > count || expected < TPM_HEADER_SIZE)
> >  		return -EIO;
> > 
> > -	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
> > +	memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);
> > 
> >  	return expected;
> >  }
> > --
> > 2.19.1
> 

Thanks!

/Jarkko

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] tpm/tpm_crb: Avoid unaligned reads in crb_recv()
       [not found] ` <20190207163157.DA4A62175B@mail.kernel.org>
@ 2019-02-07 16:53   ` Jerry Snitselaar
  0 siblings, 0 replies; 6+ messages in thread
From: Jerry Snitselaar @ 2019-02-07 16:53 UTC (permalink / raw)
  To: Sasha Levin
  Cc: Jarkko Sakkinen, linux-integrity, linux-kernel, stable,
	James Morris, Tomas Winkler

On Thu Feb 07 19, Sasha Levin wrote:
>Hi,
>
>[This is an automated email]
>
>This commit has been processed because it contains a "Fixes:" tag,
>fixing commit: 30fc8d138e91 tpm: TPM 2.0 CRB Interface.
>
>The bot has tested the following trees: v4.20.6, v4.19.19, v4.14.97, v4.9.154, v4.4.172.
>
>v4.20.6: Build OK!
>v4.19.19: Build OK!
>v4.14.97: Build OK!
>v4.9.154: Failed to apply! Possible dependencies:
>    13b1f4a571cc ("tpm_crb: map locality registers")
>    b4e2eb0651ac ("tpm crb: Work around BIOS's that report the wrong ACPI region size")
>
>v4.4.172: Failed to apply! Possible dependencies:
>    13b1f4a571cc ("tpm_crb: map locality registers")

I'm guessing it is tripping over this not being applied

>    14ddfbf488a0 ("tpm_crb: drop struct resource res from struct crb_priv")
>    1bd047be37d9 ("tpm_crb: Use devm_ioremap_resource")
>    1e3ed59d6200 ("tpm_crb: Drop le32_to_cpu(ioread32(..))")
>    25112048cd59 ("tpm: rework tpm_get_timeouts()")
>    30f9c8c9e2ea ("tpm_crb/tis: fix: use dev_name() for /proc/iomem")
>    422eac3f7dea ("tpm_crb: fix mapping of the buffers")
>    4886cd80cb8e ("Revert "tmp/tpm_crb: implement runtime pm for tpm_crb"")
>    4d627e672bd0 ("tpm_tis: Do not fall back to a hardcoded address for TPM2")
>    51dd43dff74b ("tpm_tis: Use devm_ioremap_resource")
>    55a889c2cb13 ("tpm_crb: Use the common ACPI definition of struct acpi_tpm2")
>    7ab4032fa579 ("tpm_tis: Get rid of the duplicate IRQ probing code")
>    9514ff1961c6 ("tmp/tpm_crb: fix Intel PTT hw bug during idle state")
>    aa77ea0e43dc ("tpm/tpm_crb: cache cmd_size register value.")
>    ba5287b6ef6a ("tpm/tpm_crb: implement tpm crb idle state")
>    e17acbbb69d3 ("tpm/tpm_crb: implement tpm crb idle state")
>    e350e24694e4 ("tmp/tpm_crb: implement runtime pm for tpm_crb")
>    e3837e74a06d ("tpm_tis: Refactor the interrupt setup")
>    ef7b81dc7864 ("tpm_tis: Disable interrupt auto probing on a per-device basis")
>
>
>How should we proceed with this patch?
>
>--
>Thanks,
>Sasha

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-04 13:59 [PATCH v2] tpm/tpm_crb: Avoid unaligned reads in crb_recv() Jarkko Sakkinen
2019-02-04 17:12 ` David Laight
2019-02-04 23:19   ` Jarkko Sakkinen
2019-02-04 22:09 ` Winkler, Tomas
2019-02-04 23:24   ` Jarkko Sakkinen
     [not found] ` <20190207163157.DA4A62175B@mail.kernel.org>
2019-02-07 16:53   ` Jerry Snitselaar

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org linux-integrity@archiver.kernel.org
	public-inbox-index linux-integrity


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/ public-inbox