[PATCH] tpm: Check return value of crypto_kpp_set_secret()

[PATCH] tpm: Check return value of crypto_kpp_set_secret()

[Eric Biggers]

crypto_kpp_set_secret() can fail, for example if the kernel is booted
with fips=1 and the broken-by-design FIPS RNG used for ECC private key
generation doesn't feel like generating random bytes at the moment.
Check for error, cancelling the TPM session creation rather than
proceeding with a zero or uninitialized private key.

Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/char/tpm/tpm2-sessions.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index ca1e2bf424e14..2aaaad69760dc 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -527,7 +527,9 @@ static int tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
 	}
 	crypto_ecdh_encode_key(encoded_key, buf_len, &p);
 	/* this generates a random private key */
-	crypto_kpp_set_secret(kpp, encoded_key, buf_len);
+	rc = crypto_kpp_set_secret(kpp, encoded_key, buf_len);
+	if (rc)
+		goto err_free_kpp;
 
 	/* salt is now the public point of this private key */
 	req = kpp_request_alloc(kpp, GFP_KERNEL);

base-commit: 21a7f44fd0765fcc5194130b28767786eba1d1e7
-- 
2.56.0

Re: [PATCH] tpm: Check return value of crypto_kpp_set_secret()

[Jarkko Sakkinen]

On Sun, Oct 25, 2026 at 02:11:19PM -0700, Eric Biggers wrote:
> crypto_kpp_set_secret() can fail, for example if the kernel is booted
> with fips=1 and the broken-by-design FIPS RNG used for ECC private key
> generation doesn't feel like generating random bytes at the moment.
> Check for error, cancelling the TPM session creation rather than
> proceeding with a zero or uninitialized private key.
> 
> Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>
> ---
>  drivers/char/tpm/tpm2-sessions.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index ca1e2bf424e14..2aaaad69760dc 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -527,7 +527,9 @@ static int tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
>  	}
>  	crypto_ecdh_encode_key(encoded_key, buf_len, &p);
>  	/* this generates a random private key */

Could you delete this comment while at it? It's not helping with
anything.

> -	crypto_kpp_set_secret(kpp, encoded_key, buf_len);
> +	rc = crypto_kpp_set_secret(kpp, encoded_key, buf_len);
> +	if (rc)
> +		goto err_free_kpp;
>  
>  	/* salt is now the public point of this private key */
>  	req = kpp_request_alloc(kpp, GFP_KERNEL);
> 
> base-commit: 21a7f44fd0765fcc5194130b28767786eba1d1e7
> -- 
> 2.56.0
> 

[1] https://lore.kernel.org/linux-integrity/asQdZiv7uktfyRWU@kernel.org/

Br, Jarkko

Re: [PATCH] tpm: Check return value of crypto_kpp_set_secret()

[Eric Biggers]

On Mon, Oct 26, 2026 at 12:12:13AM +0200, Jarkko Sakkinen wrote:
> On Sun, Oct 25, 2026 at 02:11:19PM -0700, Eric Biggers wrote:
> > crypto_kpp_set_secret() can fail, for example if the kernel is booted
> > with fips=1 and the broken-by-design FIPS RNG used for ECC private key
> > generation doesn't feel like generating random bytes at the moment.
> > Check for error, cancelling the TPM session creation rather than
> > proceeding with a zero or uninitialized private key.
> > 
> > Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Eric Biggers <ebiggers@kernel.org>
> > ---
> >  drivers/char/tpm/tpm2-sessions.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> > index ca1e2bf424e14..2aaaad69760dc 100644
> > --- a/drivers/char/tpm/tpm2-sessions.c
> > +++ b/drivers/char/tpm/tpm2-sessions.c
> > @@ -527,7 +527,9 @@ static int tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
> >  	}
> >  	crypto_ecdh_encode_key(encoded_key, buf_len, &p);
> >  	/* this generates a random private key */
> 
> Could you delete this comment while at it? It's not helping with
> anything.
> 
> > -	crypto_kpp_set_secret(kpp, encoded_key, buf_len);
> > +	rc = crypto_kpp_set_secret(kpp, encoded_key, buf_len);
> > +	if (rc)
> > +		goto err_free_kpp;
> >  
> >  	/* salt is now the public point of this private key */
> >  	req = kpp_request_alloc(kpp, GFP_KERNEL);
> > 

I actually think the comment is helpful, because of how unreadable the
crypto_kpp API is.  It's generating a private key, but to request that,
crypto_kpp apparently requires that an empty key be encoded into a byte
string.  Then that key is "set", which actually generates a new key.

In comparison, the Curve25519 equivalent is
curve25519_generate_secret(), which is clearly named and also always
succeeds.  At some point we need to fix the ECDH API...

- Eric

Re: [PATCH] tpm: Check return value of crypto_kpp_set_secret()

[Jarkko Sakkinen]

On Sun, Oct 25, 2026 at 03:47:23PM -0700, Eric Biggers wrote:
> On Mon, Oct 26, 2026 at 12:12:13AM +0200, Jarkko Sakkinen wrote:
> > On Sun, Oct 25, 2026 at 02:11:19PM -0700, Eric Biggers wrote:
> > > crypto_kpp_set_secret() can fail, for example if the kernel is booted
> > > with fips=1 and the broken-by-design FIPS RNG used for ECC private key
> > > generation doesn't feel like generating random bytes at the moment.
> > > Check for error, cancelling the TPM session creation rather than
> > > proceeding with a zero or uninitialized private key.
> > > 
> > > Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Eric Biggers <ebiggers@kernel.org>
> > > ---
> > >  drivers/char/tpm/tpm2-sessions.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> > > index ca1e2bf424e14..2aaaad69760dc 100644
> > > --- a/drivers/char/tpm/tpm2-sessions.c
> > > +++ b/drivers/char/tpm/tpm2-sessions.c
> > > @@ -527,7 +527,9 @@ static int tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
> > >  	}
> > >  	crypto_ecdh_encode_key(encoded_key, buf_len, &p);
> > >  	/* this generates a random private key */
> > 
> > Could you delete this comment while at it? It's not helping with
> > anything.
> > 
> > > -	crypto_kpp_set_secret(kpp, encoded_key, buf_len);
> > > +	rc = crypto_kpp_set_secret(kpp, encoded_key, buf_len);
> > > +	if (rc)
> > > +		goto err_free_kpp;
> > >  
> > >  	/* salt is now the public point of this private key */
> > >  	req = kpp_request_alloc(kpp, GFP_KERNEL);
> > > 
> 
> I actually think the comment is helpful, because of how unreadable the
> crypto_kpp API is.  It's generating a private key, but to request that,
> crypto_kpp apparently requires that an empty key be encoded into a byte
> string.  Then that key is "set", which actually generates a new key.
> 
> In comparison, the Curve25519 equivalent is
> curve25519_generate_secret(), which is clearly named and also always
> succeeds.  At some point we need to fix the ECDH API...

Ah, right it gets zero filled echd. I've not consciously thought this
detail before, thanks :-)

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> - Eric

tpm2-sessions overall would need some refurbishing work now that also
Matthew's hibernate patches depend on it. I have to take a serious
look at it some point.

Br, Jarkko

Re: [PATCH] tpm: Check return value of crypto_kpp_set_secret()

[Jarkko Sakkinen]

On Mon, Oct 26, 2026 at 01:11:21AM +0200, Jarkko Sakkinen wrote:
> On Sun, Oct 25, 2026 at 03:47:23PM -0700, Eric Biggers wrote:
> > On Mon, Oct 26, 2026 at 12:12:13AM +0200, Jarkko Sakkinen wrote:
> > > On Sun, Oct 25, 2026 at 02:11:19PM -0700, Eric Biggers wrote:
> > > > crypto_kpp_set_secret() can fail, for example if the kernel is booted
> > > > with fips=1 and the broken-by-design FIPS RNG used for ECC private key
> > > > generation doesn't feel like generating random bytes at the moment.
> > > > Check for error, cancelling the TPM session creation rather than
> > > > proceeding with a zero or uninitialized private key.
> > > > 
> > > > Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Eric Biggers <ebiggers@kernel.org>
> > > > ---
> > > >  drivers/char/tpm/tpm2-sessions.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> > > > index ca1e2bf424e14..2aaaad69760dc 100644
> > > > --- a/drivers/char/tpm/tpm2-sessions.c
> > > > +++ b/drivers/char/tpm/tpm2-sessions.c
> > > > @@ -527,7 +527,9 @@ static int tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
> > > >  	}
> > > >  	crypto_ecdh_encode_key(encoded_key, buf_len, &p);
> > > >  	/* this generates a random private key */
> > > 
> > > Could you delete this comment while at it? It's not helping with
> > > anything.
> > > 
> > > > -	crypto_kpp_set_secret(kpp, encoded_key, buf_len);
> > > > +	rc = crypto_kpp_set_secret(kpp, encoded_key, buf_len);
> > > > +	if (rc)
> > > > +		goto err_free_kpp;
> > > >  
> > > >  	/* salt is now the public point of this private key */
> > > >  	req = kpp_request_alloc(kpp, GFP_KERNEL);
> > > > 
> > 
> > I actually think the comment is helpful, because of how unreadable the
> > crypto_kpp API is.  It's generating a private key, but to request that,
> > crypto_kpp apparently requires that an empty key be encoded into a byte
> > string.  Then that key is "set", which actually generates a new key.
> > 
> > In comparison, the Curve25519 equivalent is
> > curve25519_generate_secret(), which is clearly named and also always
> > succeeds.  At some point we need to fix the ECDH API...
> 
> Ah, right it gets zero filled echd. I've not consciously thought this
> detail before, thanks :-)
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > - Eric
> 
> tpm2-sessions overall would need some refurbishing work now that also
> Matthew's hibernate patches depend on it. I have to take a serious
> look at it some point.

I'll apply this after Linus has accepted/rejected my TPM pull request.

I'm not sure if it already is but if it isn't, probably also at
linux-integrity turning on Sashiko would make sense.

Br, Jarkko

Re: [PATCH] tpm: Check return value of crypto_kpp_set_secret()

[Eric Biggers]

On Mon, Oct 26, 2026 at 01:28:41AM +0200, Jarkko Sakkinen wrote:
> I'm not sure if it already is but if it isn't, probably also at
> linux-integrity turning on Sashiko would make sense.

Sashiko currently has monitoring enabled for linux-integrity, but not
email replies
(https://github.com/sashiko-dev/sashiko/blob/main/projects/linux/mailing_lists.toml#L45).

(Note: this particular patch was Cc'ed to linux-crypto, which does have
Sashiko email replies enabled.  So Sashiko would have replied if it had
found an issue anyway.)

I definitely would support enabling email replies on linux-integrity
(for issues found only), like a lot of other lists already do:

   ["linux-integrity@vger.kernel.org"]
   reply_to = ["author", "list", "recipients"]         

If the maintainers of security/integrity/ would ack that, I can open a
pull request to the Sashiko GitHub repo that changes it.  Or someone
else can go ahead and do it if they want.

- Eric

Re: [PATCH] tpm: Check return value of crypto_kpp_set_secret()

[Jarkko Sakkinen]

Adding Mimi and Roberto.

On Sun, Oct 25, 2026 at 04:40:09PM -0700, Eric Biggers wrote:
> On Mon, Oct 26, 2026 at 01:28:41AM +0200, Jarkko Sakkinen wrote:
> > I'm not sure if it already is but if it isn't, probably also at
> > linux-integrity turning on Sashiko would make sense.
> 
> Sashiko currently has monitoring enabled for linux-integrity, but not
> email replies
> (https://github.com/sashiko-dev/sashiko/blob/main/projects/linux/mailing_lists.toml#L45).
> 
> (Note: this particular patch was Cc'ed to linux-crypto, which does have
> Sashiko email replies enabled.  So Sashiko would have replied if it had
> found an issue anyway.)
> 
> I definitely would support enabling email replies on linux-integrity
> (for issues found only), like a lot of other lists already do:
> 
>    ["linux-integrity@vger.kernel.org"]
>    reply_to = ["author", "list", "recipients"]         
> 
> If the maintainers of security/integrity/ would ack that, I can open a
> pull request to the Sashiko GitHub repo that changes it.  Or someone
> else can go ahead and do it if they want.
> 
> - Eric

Br, Jarkko