On 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 <ebigg...@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