linux-kernel - Re: [PATCH v2 1/2] tpm: Compare HMAC values in constant time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aJIMGWFDZejNwAVP@kernel.org>
Date: Tue, 5 Aug 2025 16:50:17 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Eric Biggers <ebiggers@...nel.org>
Cc: Peter Huewe <peterhuewe@....de>, linux-integrity@...r.kernel.org,
	Jason Gunthorpe <jgg@...pe.ca>,
	James Bottomley <James.Bottomley@...senpartnership.com>,
	linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] tpm: Compare HMAC values in constant time

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 <ebiggers@...nel.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