Re: [PATCH v8 18/22] tpm: add session encryption protection to tpm2_get_random()

["Jarkko Sakkinen" <jarkko@kernel.org>]

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?

1. Bug fixes need to be submitted as described in
   https://www.kernel.org/doc/html/latest/process/submitting-patches.html
2. The patch is missing the transcript:
   https://lore.kernel.org/linux-integrity/D1BRZ60B9O5S.3NAT20QPQE6KH@kernel.org/

There's nothing to review at this point.

>
> Thanks,
> Nícolas

BR, Jarkko