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

[Eric Biggers <ebiggers@kernel.org>]

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