Received-SPF: pass (google.com: domain of linux-crypto+bounces-4234-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 17 May 2024 21:31:15 -0700
From: Eric Biggers <ebiggers@kernel.org>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: =?iso-8859-1?Q?N=EDcolas_F=2E_R=2E_A=2E?= Prado <nfraprado@collabora.com>,
	James Bottomley <James.Bottomley@hansenpartnership.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
	regressions@lists.linux.dev, kernel@collabora.com
Subject: Re: [PATCH v8 18/22] tpm: add session encryption protection to
 tpm2_get_random()
Message-ID: <20240518043115.GA53815@sol.localdomain>
References: <20240429202811.13643-1-James.Bottomley@HansenPartnership.com>
 <20240429202811.13643-19-James.Bottomley@HansenPartnership.com>
 <119dc5ed-f159-41be-9dda-1a056f29888d@notapiano>
 <0f68c283ff4bbb89b8a019d47891f798c6fff287.camel@HansenPartnership.com>
 <CAMj1kXHi4r8KY9GvX573kwqvLpMfX-J=K2hWiGAKkf5bnicwYQ@mail.gmail.com>
 <0d260c2f7a9f67ec8bd2305919636678d06000d1.camel@HansenPartnership.com>
 <CAMj1kXFE_R_x10BVkU+8vrMz0RHiX0+rz-ZL+w08FH2CLQHZXA@mail.gmail.com>
 <66ec985f3ee229135bf748f1b0874d5367a74d7f.camel@HansenPartnership.com>
 <dfb0d930-7cbe-46c5-be19-d132b4906ecf@notapiano>
 <D1C2NPOBHAHK.20O4IME8OK1FH@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <D1C2NPOBHAHK.20O4IME8OK1FH@kernel.org>

On Fri, May 17, 2024 at 07:48:48PM +0300, Jarkko Sakkinen wrote:
> On Fri May 17, 2024 at 7:22 PM EEST, N?colas F. R. A. Prado wrote:
> > On Fri, May 17, 2024 at 07:25:40AM -0700, James Bottomley wrote:
> > > On Fri, 2024-05-17 at 15:43 +0200, Ard Biesheuvel wrote:
> > > > On Fri, 17 May 2024 at 15:35, James Bottomley
> > > > <James.Bottomley@hansenpartnership.com> wrote:
> > > [...]
> > > > > Thanks for the analysis.? If I look at how CRYPTO_ECC does it, that
> > > > > selects CRYPTO_RNG_DEFAULT which pulls in CRYPTO_DRBG, so the fix
> > > > > would be the attached.? Does that look right to you Ard?
> > > > 
> > > > No it doesn't - it's CRYPTO_RNG_DEFAULT not CRYTPO_RNG_DEFAULT :-)
> > > > 
> > > > With that fixed,
> > > > 
> > > > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > > 
> > > Erm, oops, sorry about that; so attached is the update.
> > > 
> > > James
> > > 
> > > ---8>8>8><8<8<8---
> > > 
> > > From 2ac337a33e6416ef806e2c692b9239d193e8468f Mon Sep 17 00:00:00 2001
> > > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > Date: Fri, 17 May 2024 06:29:31 -0700
> > > Subject: [PATCH] tpm: Fix sessions cryptography requirement for Random Numbers
> > > MIME-Version: 1.0
> > > Content-Type: text/plain; charset=UTF-8
> > > Content-Transfer-Encoding: 8bit
> > > 
> > > The ECDH code in tpm2-sessions.c requires an initial random number
> > > generator to generate the key pair.  If the configuration doesn't have
> > > CONFIG_RNG_DEFAULT, it will try to pull this in as a module (which is
> > > impossible for the early kernel boot where the TPM starts).  Fix this
> > > by selecting the required RNG.
> > > 
> > > Reported-by: N?colas F. R. A. Prado <nfraprado@collabora.com>
> > > Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to tpm2_get_random()")
> > > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > ---
> > >  drivers/char/tpm/Kconfig | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > index 4f83ee7021d0..ecdd3db4be2b 100644
> > > --- a/drivers/char/tpm/Kconfig
> > > +++ b/drivers/char/tpm/Kconfig
> > > @@ -31,6 +31,7 @@ config TCG_TPM2_HMAC
> > >  	bool "Use HMAC and encrypted transactions on the TPM bus"
> > >  	default y
> > >  	select CRYPTO_ECDH
> > > +	select CRYPTO_RNG_DEFAULT
> > >  	select CRYPTO_LIB_AESCFB
> > >  	select CRYPTO_LIB_SHA256
> > >  	help
> > > -- 
> > > 2.35.3
> > > 
> > > 
> >
> > Hi James,
> >
> > thanks for the patch. But I actually already had that config enabled builtin. I
> > also had ECDH and DRBG which have been suggested previously:
> >
> > 	CONFIG_CRYPTO_RNG_DEFAULT=y
> >
> > 	CONFIG_CRYPTO_DRBG_MENU=y
> > 	CONFIG_CRYPTO_DRBG_HMAC=y
> > 	# CONFIG_CRYPTO_DRBG_HASH is not set
> > 	# CONFIG_CRYPTO_DRBG_CTR is not set
> > 	CONFIG_CRYPTO_DRBG=y
> >
> > 	CONFIG_CRYPTO_ECDH=y
> >
> > I've pasted my full config here: http://0x0.st/XPN_.txt
> >
> > Adding a debug print I see that the module that the code tries to load is
> > "crypto-hmac(sha512)". I would have expected to see 
> >
> > 	MODULE_ALIAS_CRYPTO("hmac(sha512)");
> >
> > in crypto/drbg.c, but I don't see it anywhere in the tree. Maybe it is missing?
> 

This is "normal" behavior when the crypto API instantiates a template:

    1. drbg.c asks for "hmac(sha512)"

    2. The crypto API looks for a direct implementation of "hmac(sha512)".
       This includes requesting a module with alias "crypto-hmac(sha512)".

    3. If none is found, the "hmac" template is instantiated instead.

There are two possible fixes for the bug.  Either fix ecc_gen_privkey() to just
use get_random_bytes() instead of the weird crypto API RNG, or make
drbg_init_hash_kernel() pass the CRYPTO_NOLOAD flag to crypto_alloc_shash().

Or if the TPM driver could be changed to not need to generate an ECC private key
at probe time, that would also avoid this problem.

- Eric