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