Re: [PATCH] tpm: tpm2-sessions: wait for async KPP completion in tpm_buf_append_salt

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, May 27, 2026 at 02:46:55PM -0400, Michael Bommarito wrote:
> tpm_buf_append_salt() in drivers/char/tpm/tpm2-sessions.c calls
> crypto_kpp_generate_public_key() and crypto_kpp_compute_shared_secret()
> without installing a completion callback, discards both return values,
> and immediately frees the kpp_request via kpp_request_free(). When the
> resolved ecdh-nist-p256 KPP backend is asynchronous (atmel-ecc, HPRE,
> keembay-ocs), either operation returns -EINPROGRESS and the deferred
> completion worker dereferences the freed request.
> 
> The path fires automatically from the hwrng_fillfn kernel thread via
> tpm_get_random -> tpm2_get_random -> tpm2_start_auth_session ->
> tpm_buf_append_salt on every entropy poll, without any userland action.
> 
> Install crypto_req_done as the completion callback, wrap both KPP
> operations in crypto_wait_req(), and propagate errors to the caller.
> The wait is a no-op for synchronous backends.
> 
> Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> Assisted-by: Claude:claude-opus-4-7
> ---
> 
> Impact: on a kernel with an async ECDH KPP provider, any local user
> can reclaim the freed kpp_request slab slot and control the indirect
> call through req->base.complete. A reproducer is available on request.
> Filing publicly per security-bugs.rst guidance.

Please send the reproducer and .config although I think I grasped
what I need to put but just in case (if only for comparison).

> 
> Notes:
> 
>     Validation (QEMU x86_64, swtpm, async ecdh-nist-p256 stub backend):
>     
>     Stock kernel: the freed kpp_request is reclaimed by an unprivileged
>     heap spray; the deferred completion worker jumps to a controlled
>     address (RIP=0x41414141) via the overwritten req->base.complete
>     callback pointer. Reproduces on production-hardened allocator
>     configs (MEMCG, RANDOM_KMALLOC_CACHES, SLAB_FREELIST_HARDENED,
>     SLAB_FREELIST_RANDOM, INIT_ON_FREE).
>     
>     Patched kernel: crypto_wait_req() blocks until the async backend
>     completes; the worker observes a live request with the correct
>     crypto_req_done callback installed; kpp_request_free() runs only
>     after both operations finish. KASAN-clean across 50 entropy polls.
> 
>  drivers/char/tpm/tpm2-sessions.c | 36 ++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index c4da6fde748f4..a23cc3a540c55 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -489,15 +489,17 @@ static void tpm2_KDFe(u8 z[EC_PT_SZ], const char *str, u8 *pt_u, u8 *pt_v,
>  	sha256_final(&sctx, out);
>  }
>  
> -static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
> -				struct tpm2_auth *auth)
> +static int tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
> +			       struct tpm2_auth *auth)
>  {
>  	struct crypto_kpp *kpp;
>  	struct kpp_request *req;
> +	DECLARE_CRYPTO_WAIT(wait);
>  	struct scatterlist s[2], d[1];
>  	struct ecdh p = {0};
>  	u8 encoded_key[EC_PT_SZ], *x, *y;
>  	unsigned int buf_len;
> +	int rc;
>  
>  	/* secret is two sized points */
>  	tpm_buf_append_u16(buf, (EC_PT_SZ + 2)*2);
> @@ -520,13 +522,14 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
>  	kpp = crypto_alloc_kpp("ecdh-nist-p256", CRYPTO_ALG_INTERNAL, 0);
>  	if (IS_ERR(kpp)) {
>  		dev_err(&chip->dev, "crypto ecdh allocation failed\n");
> -		return;
> +		return PTR_ERR(kpp);
>  	}
>  
>  	buf_len = crypto_ecdh_key_len(&p);
>  	if (sizeof(encoded_key) < buf_len) {
>  		dev_err(&chip->dev, "salt buffer too small needs %d\n",
>  			buf_len);
> +		rc = -EINVAL;
>  		goto out;
>  	}
>  	crypto_ecdh_encode_key(encoded_key, buf_len, &p);
> @@ -535,11 +538,17 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
>  
>  	/* salt is now the public point of this private key */
>  	req = kpp_request_alloc(kpp, GFP_KERNEL);
> -	if (!req)
> +	if (!req) {
> +		rc = -ENOMEM;
>  		goto out;
> +	}
> +	kpp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> +				 crypto_req_done, &wait);
>  	kpp_request_set_input(req, NULL, 0);
>  	kpp_request_set_output(req, s, EC_PT_SZ*2);
> -	crypto_kpp_generate_public_key(req);
> +	rc = crypto_wait_req(crypto_kpp_generate_public_key(req), &wait);
> +	if (rc)
> +		goto out_free_req;
>  	/*
>  	 * we're not done: now we have to compute the shared secret
>  	 * which is our private key multiplied by the tpm_key public
> @@ -551,8 +560,9 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
>  	kpp_request_set_input(req, s, EC_PT_SZ*2);
>  	sg_init_one(d, auth->salt, EC_PT_SZ);
>  	kpp_request_set_output(req, d, EC_PT_SZ);
> -	crypto_kpp_compute_shared_secret(req);
> -	kpp_request_free(req);
> +	rc = crypto_wait_req(crypto_kpp_compute_shared_secret(req), &wait);
> +	if (rc)
> +		goto out_free_req;
>  
>  	/*
>  	 * pass the shared secret through KDFe for salt. Note salt
> @@ -562,8 +572,11 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
>  	 */
>  	tpm2_KDFe(auth->salt, "SECRET", x, chip->null_ec_key_x, auth->salt);
>  
> - out:
> +out_free_req:
> +	kpp_request_free(req);
> +out:
>  	crypto_free_kpp(kpp);
> +	return rc;
>  }
>  
>  /**
> @@ -1018,7 +1031,12 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
>  	tpm_buf_append(&buf, auth->our_nonce, sizeof(auth->our_nonce));
>  
>  	/* append encrypted salt and squirrel away unencrypted in auth */
> -	tpm_buf_append_salt(&buf, chip, auth);
> +	rc = tpm_buf_append_salt(&buf, chip, auth);
> +	if (rc) {
> +		tpm2_flush_context(chip, null_key);
> +		tpm_buf_destroy(&buf);
> +		goto out;
> +	}
>  	/* session type (HMAC, audit or policy) */
>  	tpm_buf_append_u8(&buf, TPM2_SE_HMAC);
>  
> -- 
> 2.53.0
> 
> 

BR, Jarkko