[PATCH] tpm/tpm_crb: Avoid unaligned reads in crb_recv():

[PATCH] tpm/tpm_crb: Avoid unaligned reads in crb_recv():

[Jarkko Sakkinen]

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: Linus Torvalds <torvalds@linux-foundation.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>
---
 drivers/char/tpm/tpm_crb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 36952ef98f90..7f47e43aa9f1 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -288,18 +288,18 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	unsigned int expected;
 
 	/* sanity check */
-	if (count < 6)
+	if (count < 8)
 		return -EIO;
 
 	if (ioread32(&priv->regs_t->ctrl_sts) & CRB_CTRL_STS_ERROR)
 		return -EIO;
 
-	memcpy_fromio(buf, priv->rsp, 6);
+	memcpy_fromio(buf, priv->rsp, 8);
 	expected = be32_to_cpup((__be32 *) &buf[2]);
-	if (expected > count || expected < 6)
+	if (expected > count || expected < 8)
 		return -EIO;
 
-	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
+	memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);
 
 	return expected;
 }
-- 
2.17.1

Re: [PATCH] tpm/tpm_crb: Avoid unaligned reads in crb_recv():

[Linus Torvalds]

On Fri, Feb 1, 2019 at 3:33 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> 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.

Ack, looks sane to me, and should help both the backport and probably
generate better code too.

In the meantime, I've committed the iomem.c change with a *long*
commit message. For all we know, there might be other cases like this
lurking somewhere else that just happened to work. Plus it's the right
thing to do anyway.

               Linus

Re: [PATCH] tpm/tpm_crb: Avoid unaligned reads in crb_recv():

[Jerry Snitselaar]

On Fri Feb 01 19, Jarkko Sakkinen 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: Linus Torvalds <torvalds@linux-foundation.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>
>---
> drivers/char/tpm/tpm_crb.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>index 36952ef98f90..7f47e43aa9f1 100644
>--- a/drivers/char/tpm/tpm_crb.c
>+++ b/drivers/char/tpm/tpm_crb.c
>@@ -288,18 +288,18 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> 	unsigned int expected;
>
> 	/* sanity check */
>-	if (count < 6)
>+	if (count < 8)
> 		return -EIO;
>
> 	if (ioread32(&priv->regs_t->ctrl_sts) & CRB_CTRL_STS_ERROR)
> 		return -EIO;
>
>-	memcpy_fromio(buf, priv->rsp, 6);
>+	memcpy_fromio(buf, priv->rsp, 8);
> 	expected = be32_to_cpup((__be32 *) &buf[2]);
>-	if (expected > count || expected < 6)
>+	if (expected > count || expected < 8)
> 		return -EIO;
>
>-	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
>+	memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);
>
> 	return expected;
> }
>-- 
>2.17.1
>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

RE: [PATCH] tpm/tpm_crb: Avoid unaligned reads in crb_recv():

[Winkler, Tomas]

> 
> 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: Linus Torvalds <torvalds@linux-foundation.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>
> ---
>  drivers/char/tpm/tpm_crb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index
> 36952ef98f90..7f47e43aa9f1 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -288,18 +288,18 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf,
> size_t count)
>  	unsigned int expected;
> 
>  	/* sanity check */
> -	if (count < 6)
> +	if (count < 8)
>  		return -EIO;
Why don't you already enforce reading at least the whole TPM header 10bytes, we are reading the whole buffer at one call anyway.
Who every is asking for 8 bytes from the protocol level, is doing something wrong.

>  	if (ioread32(&priv->regs_t->ctrl_sts) & CRB_CTRL_STS_ERROR)
>  		return -EIO;
> 
> -	memcpy_fromio(buf, priv->rsp, 6);
> +	memcpy_fromio(buf, priv->rsp, 8);
Maybe a short comment will spare someone looking into git history 
>  	expected = be32_to_cpup((__be32 *) &buf[2]);
> -	if (expected > count || expected < 6)
> +	if (expected > count || expected < 8)
Expected should be at least tpm header, right?
>  		return -EIO;
> 
> -	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
> +	memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);
> 
>  	return expected;
>  }
Otherwise ready the first 8 bytes looks good. 
Thanks 
Tomas

Re: [PATCH] tpm/tpm_crb: Avoid unaligned reads in crb_recv():

[Jarkko Sakkinen]

On Fri, Feb 01, 2019 at 07:20:42PM +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: Linus Torvalds <torvalds@linux-foundation.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>
> > ---
> >  drivers/char/tpm/tpm_crb.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index
> > 36952ef98f90..7f47e43aa9f1 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -288,18 +288,18 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf,
> > size_t count)
> >  	unsigned int expected;
> > 
> >  	/* sanity check */
> > -	if (count < 6)
> > +	if (count < 8)
> >  		return -EIO;
> Why don't you already enforce reading at least the whole TPM header 10bytes, we are reading the whole buffer at one call anyway.
> Who every is asking for 8 bytes from the protocol level, is doing something wrong.
> 
> >  	if (ioread32(&priv->regs_t->ctrl_sts) & CRB_CTRL_STS_ERROR)
> >  		return -EIO;
> > 
> > -	memcpy_fromio(buf, priv->rsp, 6);
> > +	memcpy_fromio(buf, priv->rsp, 8);
> Maybe a short comment will spare someone looking into git history 
> >  	expected = be32_to_cpup((__be32 *) &buf[2]);
> > -	if (expected > count || expected < 6)
> > +	if (expected > count || expected < 8)
> Expected should be at least tpm header, right?
> >  		return -EIO;
> > 
> > -	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
> > +	memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);
> > 
> >  	return expected;
> >  }
> Otherwise ready the first 8 bytes looks good. 
> Thanks 
> Tomas

Your proposals look sane, thank you.

/Jarkko

Re: [PATCH] tpm/tpm_crb: Avoid unaligned reads in crb_recv():

[Jarkko Sakkinen]

On Fri, Feb 01, 2019 at 09:49:05AM -0800, Linus Torvalds wrote:
> On Fri, Feb 1, 2019 at 3:33 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > 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.
> 
> Ack, looks sane to me, and should help both the backport and probably
> generate better code too.
> 
> In the meantime, I've committed the iomem.c change with a *long*
> commit message. For all we know, there might be other cases like this
> lurking somewhere else that just happened to work. Plus it's the right
> thing to do anyway.
> 
>                Linus

Great!

Umh, so, should my tpm_crb change committed to 5.0 with the minor changes
implemented suggested by Tomas or not? Can also include it to my 5.1 PR.
Either WFM.

/Jarkko

RE: [PATCH] tpm/tpm_crb: Avoid unaligned reads in crb_recv():

[David Laight]

From: Jarkko Sakkinen
> Sent: 01 February 2019 11:20
> 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.

To my mind memcpy_to/fromio() should only be used on IO addresses that are
adequately like memory, and should be implemented in a way that that won't
generate invalid bus cycles.
Also memcpy_fromio() should also be allowed to do 'aligned' accesses that
go beyond the ends of the required memory area.

...
> 
> -	memcpy_fromio(buf, priv->rsp, 6);
> +	memcpy_fromio(buf, priv->rsp, 8);
>  	expected = be32_to_cpup((__be32 *) &buf[2]);
> -	if (expected > count || expected < 6)
> +	if (expected > count || expected < 8)
>  		return -EIO;
> 
> -	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
> +	memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);

Why not just use readl() or readq() ?

Bound to generate better code.

	David

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

Re: [PATCH] tpm/tpm_crb: Avoid unaligned reads in crb_recv():

[Jarkko Sakkinen]

On Mon, Feb 04, 2019 at 12:17:43PM +0000, David Laight wrote:
> From: Jarkko Sakkinen
> > Sent: 01 February 2019 11:20
> > 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.
> 
> To my mind memcpy_to/fromio() should only be used on IO addresses that are
> adequately like memory, and should be implemented in a way that that won't
> generate invalid bus cycles.
> Also memcpy_fromio() should also be allowed to do 'aligned' accesses that
> go beyond the ends of the required memory area.
> 
> ...
> > 
> > -	memcpy_fromio(buf, priv->rsp, 6);
> > +	memcpy_fromio(buf, priv->rsp, 8);
> >  	expected = be32_to_cpup((__be32 *) &buf[2]);
> > -	if (expected > count || expected < 6)
> > +	if (expected > count || expected < 8)
> >  		return -EIO;
> > 
> > -	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
> > +	memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);
> 
> Why not just use readl() or readq() ?
> 
> Bound to generate better code.

For the first read can be done. The second read is of variable
length.

/Jarkko

Re: [PATCH] tpm/tpm_crb: Avoid unaligned reads in crb_recv():

[Jarkko Sakkinen]

On Tue, Feb 05, 2019 at 12:44:06PM +0200, Jarkko Sakkinen wrote:
> On Mon, Feb 04, 2019 at 12:17:43PM +0000, David Laight wrote:
> > From: Jarkko Sakkinen
> > > Sent: 01 February 2019 11:20
> > > 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.
> > 
> > To my mind memcpy_to/fromio() should only be used on IO addresses that are
> > adequately like memory, and should be implemented in a way that that won't
> > generate invalid bus cycles.
> > Also memcpy_fromio() should also be allowed to do 'aligned' accesses that
> > go beyond the ends of the required memory area.
> > 
> > ...
> > > 
> > > -	memcpy_fromio(buf, priv->rsp, 6);
> > > +	memcpy_fromio(buf, priv->rsp, 8);
> > >  	expected = be32_to_cpup((__be32 *) &buf[2]);
> > > -	if (expected > count || expected < 6)
> > > +	if (expected > count || expected < 8)
> > >  		return -EIO;
> > > 
> > > -	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
> > > +	memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);
> > 
> > Why not just use readl() or readq() ?
> > 
> > Bound to generate better code.
> 
> For the first read can be done. The second read is of variable
> length.

Neither can be done to the first one, because readq() does
le64_to_cpu(). Shoud not do any conversions, only raw read.
So I'll just stick it to what we have.

/jarkko

Re: [PATCH] tpm/tpm_crb: Avoid unaligned reads in crb_recv():

[Jarkko Sakkinen]

On Tue, Feb 05, 2019 at 12:47:47PM +0200, Jarkko Sakkinen wrote:
> On Tue, Feb 05, 2019 at 12:44:06PM +0200, Jarkko Sakkinen wrote:
> > On Mon, Feb 04, 2019 at 12:17:43PM +0000, David Laight wrote:
> > > From: Jarkko Sakkinen
> > > > Sent: 01 February 2019 11:20
> > > > 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.
> > > 
> > > To my mind memcpy_to/fromio() should only be used on IO addresses that are
> > > adequately like memory, and should be implemented in a way that that won't
> > > generate invalid bus cycles.
> > > Also memcpy_fromio() should also be allowed to do 'aligned' accesses that
> > > go beyond the ends of the required memory area.
> > > 
> > > ...
> > > > 
> > > > -	memcpy_fromio(buf, priv->rsp, 6);
> > > > +	memcpy_fromio(buf, priv->rsp, 8);
> > > >  	expected = be32_to_cpup((__be32 *) &buf[2]);
> > > > -	if (expected > count || expected < 6)
> > > > +	if (expected > count || expected < 8)
> > > >  		return -EIO;
> > > > 
> > > > -	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
> > > > +	memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);
> > > 
> > > Why not just use readl() or readq() ?
> > > 
> > > Bound to generate better code.
> > 
> > For the first read can be done. The second read is of variable
> > length.
> 
> Neither can be done to the first one, because readq() does
> le64_to_cpu(). Shoud not do any conversions, only raw read.
> So I'll just stick it to what we have.

ATM tmp_crb is only used in LE architectures (x86 and ARM64), but
I still hate to have hidden extra byte order conversions, even if
they get compiled as nil ops. It is more wrong than to use
memcpy_fromio().

/Jarkko