commit 3d7a850fdc1a2e4d2adbc95cc0fc962974725e88 upstream
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 Acked-by: Tomas Winkler tomas.winkler@intel.com --- backport v4.4.178 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 35308dfff754..8226e3b6dc1f 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -109,19 +109,29 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count) struct crb_priv *priv = chip->vendor.priv; 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 + * unrecoverable condition. + */ if (le32_to_cpu(ioread32(&priv->cca->sts)) & CRB_CA_STS_ERROR) return -EIO;
- memcpy_fromio(buf, priv->rsp, 6); - expected = be32_to_cpup((__be32 *) &buf[2]); - if (expected > count || expected < 6) + /* Read the first 8 bytes in order to get the length of the response. + * We read exactly a quad word in order to make sure that the remaining + * reads 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; }
On Wed, Apr 10, 2019 at 04:54:32PM +0300, Jarkko Sakkinen wrote:
commit 3d7a850fdc1a2e4d2adbc95cc0fc962974725e88 upstream
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 Acked-by: Tomas Winkler tomas.winkler@intel.com
backport v4.4.178 drivers/char/tpm/tpm_crb.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
I need a 4.9.y version first before I can take a 4.4.y version, as we do not want anyone to have a regression moving from 4.4.y to a newer kernel.
thanks,
greg k-h
On Mon, Apr 15, 2019 at 12:48:25PM +0200, Greg KH wrote:
On Wed, Apr 10, 2019 at 04:54:32PM +0300, Jarkko Sakkinen wrote:
commit 3d7a850fdc1a2e4d2adbc95cc0fc962974725e88 upstream
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 Acked-by: Tomas Winkler tomas.winkler@intel.com
backport v4.4.178 drivers/char/tpm/tpm_crb.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
I need a 4.9.y version first before I can take a 4.4.y version, as we do not want anyone to have a regression moving from 4.4.y to a newer kernel.
OK, will do.
/Jarkko
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Jarkko Sakkinen