| Message ID | 20190204135943.15756-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] tpm/tpm_crb: Avoid unaligned reads in crb_recv() | expand |
> + * 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)
> > 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
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
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
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; }
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(-)