In tpm_buf_check_hmac_response(), compare the HMAC values in constant
time using crypto_memneq() instead of in variable time using memcmp().
This is worthwhile to follow best practices and to be consistent with
MAC comparisons elsewhere in the kernel. However, in this driver the
side channel seems to have been benign: the HMAC input data is
guaranteed to always be unique, which makes the usual MAC forgery via
timing side channel not possible. Specifically, the HMAC input data in
tpm_buf_check_hmac_response() includes the "our_nonce" field, which was
generated by the kernel earlier, remains under the control of the
kernel, and is unique for each call to tpm_buf_check_hmac_response().
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
drivers/char/tpm/Kconfig | 1 +
drivers/char/tpm/tpm2-sessions.c | 6 +++---
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index dddd702b2454a..f9d8a4e966867 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -31,10 +31,11 @@ config TCG_TPM2_HMAC
bool "Use HMAC and encrypted transactions on the TPM bus"
default X86_64
select CRYPTO_ECDH
select CRYPTO_LIB_AESCFB
select CRYPTO_LIB_SHA256
+ select CRYPTO_LIB_UTILS
help
Setting this causes us to deploy a scheme which uses request
and response HMACs in addition to encryption for
communicating with the TPM to prevent or detect bus snooping
and interposer attacks (see tpm-security.rst). Saying Y
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index bdb119453dfbe..5fbd62ee50903 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -69,10 +69,11 @@
#include <linux/unaligned.h>
#include <crypto/kpp.h>
#include <crypto/ecdh.h>
#include <crypto/hash.h>
#include <crypto/hmac.h>
+#include <crypto/utils.h>
/* maximum number of names the TPM must remember for authorization */
#define AUTH_MAX_NAMES 3
#define AES_KEY_BYTES AES_KEYSIZE_128
@@ -827,16 +828,15 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
sha256_update(&sctx, auth->our_nonce, sizeof(auth->our_nonce));
sha256_update(&sctx, &auth->attrs, 1);
/* we're done with the rphash, so put our idea of the hmac there */
tpm2_hmac_final(&sctx, auth->session_key, sizeof(auth->session_key)
+ auth->passphrase_len, rphash);
- if (memcmp(rphash, &buf->data[offset_s], SHA256_DIGEST_SIZE) == 0) {
- rc = 0;
- } else {
+ if (crypto_memneq(rphash, &buf->data[offset_s], SHA256_DIGEST_SIZE)) {
dev_err(&chip->dev, "TPM: HMAC check failed\n");
goto out;
}
+ rc = 0;
/* now do response decryption */
if (auth->attrs & TPM2_SA_ENCRYPT) {
/* need key and IV */
tpm2_KDFa(auth->session_key, SHA256_DIGEST_SIZE
--
2.50.1On Fri, Aug 01, 2025 at 02:24:21PM -0700, Eric Biggers wrote:
> In tpm_buf_check_hmac_response(), compare the HMAC values in constant
> time using crypto_memneq() instead of in variable time using memcmp().
>
> This is worthwhile to follow best practices and to be consistent with
> MAC comparisons elsewhere in the kernel. However, in this driver the
> side channel seems to have been benign: the HMAC input data is
> guaranteed to always be unique, which makes the usual MAC forgery via
> timing side channel not possible. Specifically, the HMAC input data in
> tpm_buf_check_hmac_response() includes the "our_nonce" field, which was
> generated by the kernel earlier, remains under the control of the
> kernel, and is unique for each call to tpm_buf_check_hmac_response().
>
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>
> ---
> drivers/char/tpm/Kconfig | 1 +
> drivers/char/tpm/tpm2-sessions.c | 6 +++---
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index dddd702b2454a..f9d8a4e966867 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -31,10 +31,11 @@ config TCG_TPM2_HMAC
> bool "Use HMAC and encrypted transactions on the TPM bus"
> default X86_64
> select CRYPTO_ECDH
> select CRYPTO_LIB_AESCFB
> select CRYPTO_LIB_SHA256
> + select CRYPTO_LIB_UTILS
> help
> Setting this causes us to deploy a scheme which uses request
> and response HMACs in addition to encryption for
> communicating with the TPM to prevent or detect bus snooping
> and interposer attacks (see tpm-security.rst). Saying Y
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index bdb119453dfbe..5fbd62ee50903 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -69,10 +69,11 @@
> #include <linux/unaligned.h>
> #include <crypto/kpp.h>
> #include <crypto/ecdh.h>
> #include <crypto/hash.h>
> #include <crypto/hmac.h>
> +#include <crypto/utils.h>
>
> /* maximum number of names the TPM must remember for authorization */
> #define AUTH_MAX_NAMES 3
>
> #define AES_KEY_BYTES AES_KEYSIZE_128
> @@ -827,16 +828,15 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
> sha256_update(&sctx, auth->our_nonce, sizeof(auth->our_nonce));
> sha256_update(&sctx, &auth->attrs, 1);
> /* we're done with the rphash, so put our idea of the hmac there */
> tpm2_hmac_final(&sctx, auth->session_key, sizeof(auth->session_key)
> + auth->passphrase_len, rphash);
> - if (memcmp(rphash, &buf->data[offset_s], SHA256_DIGEST_SIZE) == 0) {
> - rc = 0;
> - } else {
> + if (crypto_memneq(rphash, &buf->data[offset_s], SHA256_DIGEST_SIZE)) {
> dev_err(&chip->dev, "TPM: HMAC check failed\n");
> goto out;
> }
> + rc = 0;
>
> /* now do response decryption */
> if (auth->attrs & TPM2_SA_ENCRYPT) {
> /* need key and IV */
> tpm2_KDFa(auth->session_key, SHA256_DIGEST_SIZE
> --
> 2.50.1
>
I think we might want to also backport this to stables.
BR, Jarkko
On Tue, Aug 05, 2025 at 04:50:17PM +0300, Jarkko Sakkinen wrote: > > I think we might want to also backport this to stables. > That's what I did originally, but on v1 James complained about it being characterized as a fix. - Eric
On Tue, Aug 05, 2025 at 09:07:40AM -0700, Eric Biggers wrote: > On Tue, Aug 05, 2025 at 04:50:17PM +0300, Jarkko Sakkinen wrote: > > > > I think we might want to also backport this to stables. > > > > That's what I did originally, but on v1 James complained about it being > characterized as a fix. Please put out v3 with backporting shenanigans and I can apply these. > > - Eric BR, Jarkko
On Sat, Aug 09, 2025 at 01:36:46PM +0300, Jarkko Sakkinen wrote: > On Tue, Aug 05, 2025 at 09:07:40AM -0700, Eric Biggers wrote: > > On Tue, Aug 05, 2025 at 04:50:17PM +0300, Jarkko Sakkinen wrote: > > > > > > I think we might want to also backport this to stables. > > > > > > > That's what I did originally, but on v1 James complained about it being > > characterized as a fix. > > Please put out v3 with backporting shenanigans and I can apply these. > v1 had Fixes and Cc stable (https://lore.kernel.org/r/20250731215255.113897-2-ebiggers@kernel.org/). Again, I removed them in response to James' complaint about it being characterized as a fix. If you want them back, please go ahead and add them back in when committing. I'm not going to go around in circles. - Eric
Hi Jarkko, On Sat, Aug 09, 2025 at 10:38:39AM -0700, Eric Biggers wrote: > On Sat, Aug 09, 2025 at 01:36:46PM +0300, Jarkko Sakkinen wrote: > > On Tue, Aug 05, 2025 at 09:07:40AM -0700, Eric Biggers wrote: > > > On Tue, Aug 05, 2025 at 04:50:17PM +0300, Jarkko Sakkinen wrote: > > > > > > > > I think we might want to also backport this to stables. > > > > > > > > > > That's what I did originally, but on v1 James complained about it being > > > characterized as a fix. > > > > Please put out v3 with backporting shenanigans and I can apply these. > > > > v1 had Fixes and Cc stable > (https://lore.kernel.org/r/20250731215255.113897-2-ebiggers@kernel.org/). > Again, I removed them in response to James' complaint about it being > characterized as a fix. If you want them back, please go ahead and add > them back in when committing. I'm not going to go around in circles. > > - Eric Could you let me know how you'd like to proceed here? Thanks. - Eric
On Wed, Sep 03, 2025 at 07:38:28PM -0700, Eric Biggers wrote: > Hi Jarkko, > > On Sat, Aug 09, 2025 at 10:38:39AM -0700, Eric Biggers wrote: > > On Sat, Aug 09, 2025 at 01:36:46PM +0300, Jarkko Sakkinen wrote: > > > On Tue, Aug 05, 2025 at 09:07:40AM -0700, Eric Biggers wrote: > > > > On Tue, Aug 05, 2025 at 04:50:17PM +0300, Jarkko Sakkinen wrote: > > > > > > > > > > I think we might want to also backport this to stables. > > > > > > > > > > > > > That's what I did originally, but on v1 James complained about it being > > > > characterized as a fix. > > > > > > Please put out v3 with backporting shenanigans and I can apply these. > > > > > > > v1 had Fixes and Cc stable > > (https://lore.kernel.org/r/20250731215255.113897-2-ebiggers@kernel.org/). > > Again, I removed them in response to James' complaint about it being > > characterized as a fix. If you want them back, please go ahead and add > > them back in when committing. I'm not going to go around in circles. > > > > - Eric > > Could you let me know how you'd like to proceed here? Thanks. Thanks for reminding. I applied them to my local tree and will push tomorrow. > > - Eric BR, Jarkko
© 2016 - 2025 Red Hat, Inc.