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

 ()

On Tue, Feb 05, 2019 at 10:57:19PM +0200, Jarkko Sakkinen wrote:

quoted

On Tue, Feb 05, 2019 at 02:56:02PM +0000, Winkler, Tomas wrote:

quoted

quoted

-----Original Message-----
From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
Sent: Tuesday, February 05, 2019 16:36
To: Winkler, Tomas <redacted>
Cc: linux-integrity@vger.kernel.org; linux-kernel@vger.kernel.org;
linux- security-module@vger.kernel.org; stable@vger.kernel.org;
James Morris [off-list ref]; Jerry Snitselaar
[off-list ref]
Subject: Re: [PATCH v3] tpm/tpm_crb: Avoid unaligned reads in
crb_recv()

On Tue, Feb 05, 2019 at 11:07:16AM +0000, Winkler, Tomas wrote:

quoted

quoted

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.

quoted

quoted

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 <redacted>
Cc: Jerry Snitselaar <redacted>
Fixes: 30fc8d138e91 ("tpm: TPM 2.0 CRB Interface")
Signed-off-by: Jarkko Sakkinen
[off-list ref]
Reviewed-by: Jerry Snitselaar <redacted>
---
v3:
* Fix typo i.e. %s/reminding/remaining/g

Why you haven't fixed all the typos I've pointed out? I think you missed

that.

quoted

quoted

quoted

I saw only comment about remaining. Was there something else? Can fix.

https://www.spinics.net/lists/stable/msg283648.html

1. unrecovable -> unrecoverable
2. /* Read 8 bytes (not just 6 bytes, which would cover the tag and
the response  length

quoted

+	 * fields) in order to make sure that the remaining memory
+accesses */

Thanks and apologies for missing these.

Fixed comments and applied the patch, thank you. Do I amend your acked-by?

Please, do.
Thanks
Tomas