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

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

[Ard Biesheuvel]

On Fri, 17 May 2024 at 03:59, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Thu, 2024-05-16 at 20:25 -0400, Nícolas F. R. A. Prado wrote:
...
> > KernelCI has identified a new warning and I tracked it down to this
> > commit. It
> > was observed on the following platforms:
> > * mt8183-kukui-jacuzzi-juniper-sku16
> > * sc7180-trogdor-kingoftown
> > (but probably affects all platforms that have a tpm driver with async
> > probe)
> >
> > [    2.175146] Call trace:
> > [    2.177587]  __request_module+0x188/0x1f4
> > [    2.181596]  crypto_alg_mod_lookup+0x178/0x21c
> > [    2.186042]  crypto_alloc_tfm_node+0x58/0x114
> > [    2.190396]  crypto_alloc_shash+0x24/0x30
> > [    2.194404]  drbg_init_hash_kernel+0x28/0xdc
> > [    2.198673]  drbg_kcapi_seed+0x21c/0x420
> > [    2.202593]  crypto_rng_reset+0x84/0xb4
> > [    2.206425]  crypto_get_default_rng+0xa4/0xd8
> > [    2.210779]  ecc_gen_privkey+0x58/0xd0
> > [    2.214526]  ecdh_set_secret+0x90/0x198
> > [    2.218360]  tpm_buf_append_salt+0x164/0x2dc
>
> This looks like a misconfiguration.  The kernel is trying to load the
> ecdh module, but it should have been selected as built in by this in
> drivers/char/tpm/Kconfig:
>
> config TCG_TPM2_HMAC
>         bool "Use HMAC and encrypted transactions on the TPM bus"
>         default y
>         select CRYPTO_ECDH
>         select CRYPTO_LIB_AESCFB
>         select CRYPTO_LIB_SHA256
>

The module request is not for ECDH itself but for the DRBG it attempts
to use to generate the secret.

Given that CRYPTO_ECDH does not strictly require a DRBG in principle,
but does in this particular case, I think it makes sense to select
CRYPTO_DRBG here (or depend on it being builtin), rather than updating
the Kconfig rules for CRYPTO_ECDH itself.

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

[Jarkko Sakkinen]

On Fri May 17, 2024 at 10:20 AM EEST, Ard Biesheuvel wrote:
> On Fri, 17 May 2024 at 03:59, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> >
> > On Thu, 2024-05-16 at 20:25 -0400, Nícolas F. R. A. Prado wrote:
> ...
> > > KernelCI has identified a new warning and I tracked it down to this
> > > commit. It
> > > was observed on the following platforms:
> > > * mt8183-kukui-jacuzzi-juniper-sku16
> > > * sc7180-trogdor-kingoftown
> > > (but probably affects all platforms that have a tpm driver with async
> > > probe)
> > >
> > > [    2.175146] Call trace:
> > > [    2.177587]  __request_module+0x188/0x1f4
> > > [    2.181596]  crypto_alg_mod_lookup+0x178/0x21c
> > > [    2.186042]  crypto_alloc_tfm_node+0x58/0x114
> > > [    2.190396]  crypto_alloc_shash+0x24/0x30
> > > [    2.194404]  drbg_init_hash_kernel+0x28/0xdc
> > > [    2.198673]  drbg_kcapi_seed+0x21c/0x420
> > > [    2.202593]  crypto_rng_reset+0x84/0xb4
> > > [    2.206425]  crypto_get_default_rng+0xa4/0xd8
> > > [    2.210779]  ecc_gen_privkey+0x58/0xd0
> > > [    2.214526]  ecdh_set_secret+0x90/0x198
> > > [    2.218360]  tpm_buf_append_salt+0x164/0x2dc
> >
> > This looks like a misconfiguration.  The kernel is trying to load the
> > ecdh module, but it should have been selected as built in by this in
> > drivers/char/tpm/Kconfig:
> >
> > config TCG_TPM2_HMAC
> >         bool "Use HMAC and encrypted transactions on the TPM bus"
> >         default y
> >         select CRYPTO_ECDH
> >         select CRYPTO_LIB_AESCFB
> >         select CRYPTO_LIB_SHA256
> >
>
> The module request is not for ECDH itself but for the DRBG it attempts
> to use to generate the secret.
>
> Given that CRYPTO_ECDH does not strictly require a DRBG in principle,
> but does in this particular case, I think it makes sense to select
> CRYPTO_DRBG here (or depend on it being builtin), rather than updating
> the Kconfig rules for CRYPTO_ECDH itself.

I can spin a new PR if James can make a fix.

All previous 4 PR's for 6.10 were applied to Linus' tree so my queue
is empty. Need to have both fixes and stable-tags to save my bandwidth.

Maybe for transcript just two first lines denoting that it was
__request_module() will do. That and adding CONFIG_DRBG will take
it away should be enough for the full disclosure, right?

BR, Jarkko

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

[James Bottomley]

On Fri, 2024-05-17 at 09:20 +0200, Ard Biesheuvel wrote:
> On Fri, 17 May 2024 at 03:59, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 
> > On Thu, 2024-05-16 at 20:25 -0400, Nícolas F. R. A. Prado wrote:
> ...
> > > KernelCI has identified a new warning and I tracked it down to
> > > this
> > > commit. It
> > > was observed on the following platforms:
> > > * mt8183-kukui-jacuzzi-juniper-sku16
> > > * sc7180-trogdor-kingoftown
> > > (but probably affects all platforms that have a tpm driver with
> > > async
> > > probe)
> > > 
> > > [    2.175146] Call trace:
> > > [    2.177587]  __request_module+0x188/0x1f4
> > > [    2.181596]  crypto_alg_mod_lookup+0x178/0x21c
> > > [    2.186042]  crypto_alloc_tfm_node+0x58/0x114
> > > [    2.190396]  crypto_alloc_shash+0x24/0x30
> > > [    2.194404]  drbg_init_hash_kernel+0x28/0xdc
> > > [    2.198673]  drbg_kcapi_seed+0x21c/0x420
> > > [    2.202593]  crypto_rng_reset+0x84/0xb4
> > > [    2.206425]  crypto_get_default_rng+0xa4/0xd8
> > > [    2.210779]  ecc_gen_privkey+0x58/0xd0
> > > [    2.214526]  ecdh_set_secret+0x90/0x198
> > > [    2.218360]  tpm_buf_append_salt+0x164/0x2dc
> > 
> > This looks like a misconfiguration.  The kernel is trying to load
> > the
> > ecdh module, but it should have been selected as built in by this
> > in
> > drivers/char/tpm/Kconfig:
> > 
> > config TCG_TPM2_HMAC
> >         bool "Use HMAC and encrypted transactions on the TPM bus"
> >         default y
> >         select CRYPTO_ECDH
> >         select CRYPTO_LIB_AESCFB
> >         select CRYPTO_LIB_SHA256
> > 
> 
> The module request is not for ECDH itself but for the DRBG it
> attempts
> to use to generate the secret.
> 
> Given that CRYPTO_ECDH does not strictly require a DRBG in principle,
> but does in this particular case, I think it makes sense to select
> CRYPTO_DRBG here (or depend on it being builtin), rather than
> updating the Kconfig rules for CRYPTO_ECDH itself.

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?  And does it work
Nicolas?

James

---8>8>8><8<8<8---

From 8c60ffd959eaa65627aca6596c35bb9cbfd9bee6 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()")
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..12065eddb922 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 CRYTPO_RNG_DEFAULT
 	select CRYPTO_LIB_AESCFB
 	select CRYPTO_LIB_SHA256
 	help
-- 
2.35.3

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

[Ard Biesheuvel]

On Fri, 17 May 2024 at 15:35, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Fri, 2024-05-17 at 09:20 +0200, Ard Biesheuvel wrote:
> > On Fri, 17 May 2024 at 03:59, James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > >
> > > On Thu, 2024-05-16 at 20:25 -0400, Nícolas F. R. A. Prado wrote:
> > ...
> > > > KernelCI has identified a new warning and I tracked it down to
> > > > this
> > > > commit. It
> > > > was observed on the following platforms:
> > > > * mt8183-kukui-jacuzzi-juniper-sku16
> > > > * sc7180-trogdor-kingoftown
> > > > (but probably affects all platforms that have a tpm driver with
> > > > async
> > > > probe)
> > > >
> > > > [    2.175146] Call trace:
> > > > [    2.177587]  __request_module+0x188/0x1f4
> > > > [    2.181596]  crypto_alg_mod_lookup+0x178/0x21c
> > > > [    2.186042]  crypto_alloc_tfm_node+0x58/0x114
> > > > [    2.190396]  crypto_alloc_shash+0x24/0x30
> > > > [    2.194404]  drbg_init_hash_kernel+0x28/0xdc
> > > > [    2.198673]  drbg_kcapi_seed+0x21c/0x420
> > > > [    2.202593]  crypto_rng_reset+0x84/0xb4
> > > > [    2.206425]  crypto_get_default_rng+0xa4/0xd8
> > > > [    2.210779]  ecc_gen_privkey+0x58/0xd0
> > > > [    2.214526]  ecdh_set_secret+0x90/0x198
> > > > [    2.218360]  tpm_buf_append_salt+0x164/0x2dc
> > >
> > > This looks like a misconfiguration.  The kernel is trying to load
> > > the
> > > ecdh module, but it should have been selected as built in by this
> > > in
> > > drivers/char/tpm/Kconfig:
> > >
> > > config TCG_TPM2_HMAC
> > >         bool "Use HMAC and encrypted transactions on the TPM bus"
> > >         default y
> > >         select CRYPTO_ECDH
> > >         select CRYPTO_LIB_AESCFB
> > >         select CRYPTO_LIB_SHA256
> > >
> >
> > The module request is not for ECDH itself but for the DRBG it
> > attempts
> > to use to generate the secret.
> >
> > Given that CRYPTO_ECDH does not strictly require a DRBG in principle,
> > but does in this particular case, I think it makes sense to select
> > CRYPTO_DRBG here (or depend on it being builtin), rather than
> > updating the Kconfig rules for CRYPTO_ECDH itself.
>
> 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>



> And does it work
> Nicolas?
>
> James
>
> ---8>8>8><8<8<8---
>
> From 8c60ffd959eaa65627aca6596c35bb9cbfd9bee6 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()")
> 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..12065eddb922 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 CRYTPO_RNG_DEFAULT
>         select CRYPTO_LIB_AESCFB
>         select CRYPTO_LIB_SHA256
>         help
> --
> 2.35.3
>
>

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

[James Bottomley]

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

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

[Nícolas F. R. A. Prado]

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?

Thanks,
Nícolas

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

[Jarkko Sakkinen]

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

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

[Eric Biggers]

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

[PATCH] crypto: api - Do not load modules until algapi is ready

[Herbert Xu]

On Fri, May 17, 2024 at 09:31:15PM -0700, Eric Biggers wrote:
>
> 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.

Thanks for diagnosing the problem.  This is easy to fix though,
we could simply reuse the static branch that was created for
boot-time self-testing:

---8<---
When the Crypto API is built into the kernel, it may be invoked
during system initialisation before modules can be loaded.  Ensure
that it doesn't load modules if this is the case by checking
crypto_boot_test_finished().

Add a call to wait_for_device_probe so that the drivers that may
call into the Crypto API have finished probing.

Reported-by: Nícolas F. R. A. Prado" <nfraprado@collabora.com>
Reported-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 85bc279b4233..c018bcbd1f46 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -7,6 +7,7 @@
 
 #include <crypto/algapi.h>
 #include <crypto/internal/simd.h>
+#include <linux/device/driver.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/fips.h>
@@ -1056,9 +1057,12 @@ EXPORT_SYMBOL_GPL(crypto_type_has_alg);
 
 static void __init crypto_start_tests(void)
 {
-	if (IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS))
+	if (!IS_BUILTIN(CONFIG_CRYPTO_ALGAPI))
 		return;
 
+	if (IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS))
+		goto test_done;
+
 	for (;;) {
 		struct crypto_larval *larval = NULL;
 		struct crypto_alg *q;
@@ -1092,6 +1096,8 @@ static void __init crypto_start_tests(void)
 		crypto_wait_for_test(larval);
 	}
 
+test_done:
+	wait_for_device_probe();
 	set_crypto_boot_test_finished();
 }
 
diff --git a/crypto/api.c b/crypto/api.c
index 6aa5a3b4ed5e..5c970af04ba9 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -31,9 +31,8 @@ EXPORT_SYMBOL_GPL(crypto_alg_sem);
 BLOCKING_NOTIFIER_HEAD(crypto_chain);
 EXPORT_SYMBOL_GPL(crypto_chain);
 
-#ifndef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
+#if IS_BUILTIN(CONFIG_CRYPTO_ALGAPI)
 DEFINE_STATIC_KEY_FALSE(__crypto_boot_test_finished);
-EXPORT_SYMBOL_GPL(__crypto_boot_test_finished);
 #endif
 
 static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg);
@@ -280,7 +279,7 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type,
 	mask &= ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
 
 	alg = crypto_alg_lookup(name, type, mask);
-	if (!alg && !(mask & CRYPTO_NOLOAD)) {
+	if (crypto_boot_test_finished() && !alg && !(mask & CRYPTO_NOLOAD)) {
 		request_module("crypto-%s", name);
 
 		if (!((type ^ CRYPTO_ALG_NEED_FALLBACK) & mask &
diff --git a/crypto/internal.h b/crypto/internal.h
index 63e59240d5fb..d27166a92eca 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -66,7 +66,7 @@ extern struct blocking_notifier_head crypto_chain;
 
 int alg_test(const char *driver, const char *alg, u32 type, u32 mask);
 
-#ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
+#if !IS_BUILTIN(CONFIG_CRYPTO_ALGAPI)
 static inline bool crypto_boot_test_finished(void)
 {
 	return true;
@@ -84,7 +84,7 @@ static inline void set_crypto_boot_test_finished(void)
 {
 	static_branch_enable(&__crypto_boot_test_finished);
 }
-#endif /* !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS */
+#endif /* !IS_BUILTIN(CONFIG_CRYPTO_ALGAPI) */
 
 #ifdef CONFIG_PROC_FS
 void __init crypto_init_proc(void);
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: api - Do not load modules until algapi is ready

[Jarkko Sakkinen]

On Sat May 18, 2024 at 10:03 AM EEST, Herbert Xu wrote:
> When the Crypto API is built into the kernel, it may be invoked
> during system initialisation before modules can be loaded.  Ensure
> that it doesn't load modules if this is the case by checking
> crypto_boot_test_finished().
>
> Add a call to wait_for_device_probe so that the drivers that may
> call into the Crypto API have finished probing.
>
> Reported-by: Nícolas F. R. A. Prado" <nfraprado@collabora.com>
> Reported-by: Eric Biggers <ebiggers@kernel.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Right does this mean for TPM driver that a crypto API invocation not
having everthing needed loaded will block until this is not the case?

BR, Jarkko

Re: [PATCH] crypto: api - Do not load modules until algapi is ready

[Herbert Xu]

On Sat, May 18, 2024 at 02:04:18PM +0300, Jarkko Sakkinen wrote:
>
> Right does this mean for TPM driver that a crypto API invocation not
> having everthing needed loaded will block until this is not the case?

All this does is disable module loading by the Crypto API (because
there is no point and it may deadlock) until such a point where
all/most drivers have finished loading.

So if the algorithm is missing (which shouldn't happen because of
Kconfig selects), then it will simply fail.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: api - Do not load modules until algapi is ready

[Jarkko Sakkinen]

On Sat May 18, 2024 at 3:32 PM EEST, Herbert Xu wrote:
> On Sat, May 18, 2024 at 02:04:18PM +0300, Jarkko Sakkinen wrote:
> >
> > Right does this mean for TPM driver that a crypto API invocation not
> > having everthing needed loaded will block until this is not the case?
>
> All this does is disable module loading by the Crypto API (because
> there is no point and it may deadlock) until such a point where
> all/most drivers have finished loading.
>
> So if the algorithm is missing (which shouldn't happen because of
> Kconfig selects), then it will simply fail.
>
> Cheers,

Ok, you can add

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

BR, Jarkko

Re: [PATCH] crypto: api - Do not load modules until algapi is ready

[James Bottomley]

On May 18, 2024 5:32:56 AM PDT, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>On Sat, May 18, 2024 at 02:04:18PM +0300, Jarkko Sakkinen wrote:
>>
>> Right does this mean for TPM driver that a crypto API invocation not
>> having everthing needed loaded will block until this is not the case?
>
>All this does is disable module loading by the Crypto API (because
>there is no point and it may deadlock) until such a point where
>all/most drivers have finished loading.
>
>So if the algorithm is missing (which shouldn't happen because of
>Kconfig selects), then it will simply fail.

I have a curiosity question: if Eric is right and it's looking for an optional hmac accelerator module, why don't I see this?  The only real config difference between what I tested and what Nicholas did is he's arm and I'm x86.

James

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] crypto: api - Do not load modules until algapi is ready

[Herbert Xu]

On Sat, May 18, 2024 at 06:07:39AM -0700, James Bottomley wrote:
>
> I have a curiosity question: if Eric is right and it's looking for an optional hmac accelerator module, why don't I see this?  The only real config difference between what I tested and what Nicholas did is he's arm and I'm x86.

It depends on your kernel config.  Perhaps you didn't build the
TPM driver into the kernel?

It's not an issue with an optional algorithm that's not included.

It's the fact the Crypto API tries to load a module for any
algorithm that requires instantiation (anything with "()" in its
name).

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: api - Do not load modules until algapi is ready

[Nícolas F. R. A. Prado]

On Sat, May 18, 2024 at 03:03:51PM +0800, Herbert Xu wrote:
> On Fri, May 17, 2024 at 09:31:15PM -0700, Eric Biggers wrote:
> >
> > 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.
> 
> Thanks for diagnosing the problem.  This is easy to fix though,
> we could simply reuse the static branch that was created for
> boot-time self-testing:
> 
> ---8<---
> When the Crypto API is built into the kernel, it may be invoked
> during system initialisation before modules can be loaded.  Ensure
> that it doesn't load modules if this is the case by checking
> crypto_boot_test_finished().
> 
> Add a call to wait_for_device_probe so that the drivers that may
> call into the Crypto API have finished probing.
> 
> Reported-by: Nícolas F. R. A. Prado" <nfraprado@collabora.com>
> Reported-by: Eric Biggers <ebiggers@kernel.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 85bc279b4233..c018bcbd1f46 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -7,6 +7,7 @@
>  
>  #include <crypto/algapi.h>
>  #include <crypto/internal/simd.h>
> +#include <linux/device/driver.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
>  #include <linux/fips.h>
> @@ -1056,9 +1057,12 @@ EXPORT_SYMBOL_GPL(crypto_type_has_alg);
>  
>  static void __init crypto_start_tests(void)
>  {
> -	if (IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS))
> +	if (!IS_BUILTIN(CONFIG_CRYPTO_ALGAPI))
>  		return;
>  
> +	if (IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS))
> +		goto test_done;
> +
>  	for (;;) {
>  		struct crypto_larval *larval = NULL;
>  		struct crypto_alg *q;
> @@ -1092,6 +1096,8 @@ static void __init crypto_start_tests(void)
>  		crypto_wait_for_test(larval);
>  	}
>  
> +test_done:
> +	wait_for_device_probe();
>  	set_crypto_boot_test_finished();
>  }
>  
> diff --git a/crypto/api.c b/crypto/api.c
> index 6aa5a3b4ed5e..5c970af04ba9 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -31,9 +31,8 @@ EXPORT_SYMBOL_GPL(crypto_alg_sem);
>  BLOCKING_NOTIFIER_HEAD(crypto_chain);
>  EXPORT_SYMBOL_GPL(crypto_chain);
>  
> -#ifndef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
> +#if IS_BUILTIN(CONFIG_CRYPTO_ALGAPI)
>  DEFINE_STATIC_KEY_FALSE(__crypto_boot_test_finished);
> -EXPORT_SYMBOL_GPL(__crypto_boot_test_finished);
>  #endif
>  
>  static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg);
> @@ -280,7 +279,7 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type,
>  	mask &= ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
>  
>  	alg = crypto_alg_lookup(name, type, mask);
> -	if (!alg && !(mask & CRYPTO_NOLOAD)) {
> +	if (crypto_boot_test_finished() && !alg && !(mask & CRYPTO_NOLOAD)) {
>  		request_module("crypto-%s", name);
>  
>  		if (!((type ^ CRYPTO_ALG_NEED_FALLBACK) & mask &
> diff --git a/crypto/internal.h b/crypto/internal.h
> index 63e59240d5fb..d27166a92eca 100644
> --- a/crypto/internal.h
> +++ b/crypto/internal.h
> @@ -66,7 +66,7 @@ extern struct blocking_notifier_head crypto_chain;
>  
>  int alg_test(const char *driver, const char *alg, u32 type, u32 mask);
>  
> -#ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
> +#if !IS_BUILTIN(CONFIG_CRYPTO_ALGAPI)
>  static inline bool crypto_boot_test_finished(void)
>  {
>  	return true;
> @@ -84,7 +84,7 @@ static inline void set_crypto_boot_test_finished(void)
>  {
>  	static_branch_enable(&__crypto_boot_test_finished);
>  }
> -#endif /* !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS */
> +#endif /* !IS_BUILTIN(CONFIG_CRYPTO_ALGAPI) */
>  
>  #ifdef CONFIG_PROC_FS
>  void __init crypto_init_proc(void);
> -- 

Hi Herbert,

Unfortunately this patch didn't work either. The warning is still there
unchanged.

The warning is triggered by a driver that probes asynchronously registering a
TPM device, which ends up requesting a module to be loaded synchronously. So I
don't think waiting would even help here. I believe one possible solution would
be to request the module asynchronously and defer probe. Not ideal but I think
would work.

Alternatively, could the initialization code that loads this module
(hmac(sha512)) be run synchronously before the TPM device is registered? Or is
it device specific?

PS: the wait_for_device_probe() call in the patch didn't actually wait for
anything in this case since when the crypto tests code ran there weren't any
probes in progress

Thanks,
Nícolas

[v2 PATCH] crypto: api - Do not load modules if called by async probing

[Herbert Xu]

On Mon, May 20, 2024 at 11:49:56AM -0400, Nícolas F. R. A. Prado wrote:
>
> Unfortunately this patch didn't work either. The warning is still there
> unchanged.

OK perhaps we can do it by calling current_is_async ourselves.
But this is really a nasty hack because it basically defeats
the whole point of loading optional algorithm by module.

Linus/Tejun, is it time perhaps to remove the warning introduced
by commit 0fdff3ec6d87856cdcc99e69cf42143fdd6c56b4 since it's
been ten years since the warning caused a real problem?

For the Crypto API, if it is called by some random driver via the
async context, this warning stops us from loading any modules
without printing a nasty warning that isn't relevant as the Crypto
API never calls async_synchronize_full.

---8<---
Do not call request_module if this is the case or a warning will
be printed.

Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Reported-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/api.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/api.c b/crypto/api.c
index 22556907b3bc..7c4b9f86c1ad 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -10,6 +10,7 @@
  * and Nettle, by Niels Möller.
  */
 
+#include <linux/async.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/jump_label.h>
@@ -280,7 +281,8 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type,
 	mask &= ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
 
 	alg = crypto_alg_lookup(name, type, mask);
-	if (!alg && !(mask & CRYPTO_NOLOAD)) {
+	if (!alg && !(mask & CRYPTO_NOLOAD) &&
+	    (!IS_BUILTIN(CONFIG_CRYPTO) || !current_is_async())) {
 		request_module("crypto-%s", name);
 
 		if (!((type ^ CRYPTO_ALG_NEED_FALLBACK) & mask &
-- 
2.39.2

-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v2 PATCH] crypto: api - Do not load modules if called by async probing

[Nícolas F. R. A. Prado]

On Tue, May 21, 2024 at 10:53:18AM +0800, Herbert Xu wrote:
> On Mon, May 20, 2024 at 11:49:56AM -0400, Nícolas F. R. A. Prado wrote:
> >
> > Unfortunately this patch didn't work either. The warning is still there
> > unchanged.
> 
> OK perhaps we can do it by calling current_is_async ourselves.
> But this is really a nasty hack because it basically defeats
> the whole point of loading optional algorithm by module.
> 
> Linus/Tejun, is it time perhaps to remove the warning introduced
> by commit 0fdff3ec6d87856cdcc99e69cf42143fdd6c56b4 since it's
> been ten years since the warning caused a real problem?
> 
> For the Crypto API, if it is called by some random driver via the
> async context, this warning stops us from loading any modules
> without printing a nasty warning that isn't relevant as the Crypto
> API never calls async_synchronize_full.
> 
> ---8<---
> Do not call request_module if this is the case or a warning will
> be printed.
> 
> Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Reported-by: Eric Biggers <ebiggers@kernel.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
>  crypto/api.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/api.c b/crypto/api.c
> index 22556907b3bc..7c4b9f86c1ad 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -10,6 +10,7 @@
>   * and Nettle, by Niels Möller.
>   */
>  
> +#include <linux/async.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
>  #include <linux/jump_label.h>
> @@ -280,7 +281,8 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type,
>  	mask &= ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
>  
>  	alg = crypto_alg_lookup(name, type, mask);
> -	if (!alg && !(mask & CRYPTO_NOLOAD)) {
> +	if (!alg && !(mask & CRYPTO_NOLOAD) &&
> +	    (!IS_BUILTIN(CONFIG_CRYPTO) || !current_is_async())) {
>  		request_module("crypto-%s", name);
>  
>  		if (!((type ^ CRYPTO_ALG_NEED_FALLBACK) & mask &
> -- 
> 2.39.2

FWIW this patch fixes the warning. So feel free to add

Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

if you choose to apply this patch (I'm happy to help test other patches too). In
any case, please also add the following trailers so the regression gets closed
automatically in regzbot:

Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to tpm2_get_random()")
Link: https://lore.kernel.org/r/119dc5ed-f159-41be-9dda-1a056f29888d@notapiano/

Thanks,
Nícolas

[v3 PATCH] hwrng: core - Remove add_early_randomness

[Herbert Xu]

On Tue, May 21, 2024 at 03:37:16PM -0400, Nícolas F. R. A. Prado wrote:
>
> FWIW this patch fixes the warning. So feel free to add
> 
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Could you please test this patch instead?

---8<---
A potential deadlock was reported with the config file at

https://web.archive.org/web/20240522052129/https://0x0.st/XPN_.txt

In this particular configuration, the deadlock doesn't exist because
the warning triggered at a point before modules were even available.
However, the deadlock can be real because any module loaded would
invoke async_synchronize_full.

The issue is spurious for software crypto algorithms which aren't
themselves involved in async probing.  However, it would be hard to
avoid for a PCI crypto driver using async probing.

In this particular call trace, the problem is easily avoided because
the only reason the module is being requested during probing is the
add_early_randomness call in the hwrng core.  This feature is
vestigial since there is now a kernel thread dedicated to doing
exactly this.

So remove add_early_randomness as it is no longer needed.

Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Reported-by: Eric Biggers <ebiggers@kernel.org>
Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to tpm2_get_random()")
Link: https://lore.kernel.org/r/119dc5ed-f159-41be-9dda-1a056f29888d@notapiano/
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index f5c71a617a99..4084df65c9fa 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -64,19 +64,6 @@ static size_t rng_buffer_size(void)
 	return RNG_BUFFER_SIZE;
 }
 
-static void add_early_randomness(struct hwrng *rng)
-{
-	int bytes_read;
-
-	mutex_lock(&reading_mutex);
-	bytes_read = rng_get_data(rng, rng_fillbuf, 32, 0);
-	mutex_unlock(&reading_mutex);
-	if (bytes_read > 0) {
-		size_t entropy = bytes_read * 8 * rng->quality / 1024;
-		add_hwgenerator_randomness(rng_fillbuf, bytes_read, entropy, false);
-	}
-}
-
 static inline void cleanup_rng(struct kref *kref)
 {
 	struct hwrng *rng = container_of(kref, struct hwrng, ref);
@@ -340,13 +327,12 @@ static ssize_t rng_current_store(struct device *dev,
 				 const char *buf, size_t len)
 {
 	int err;
-	struct hwrng *rng, *old_rng, *new_rng;
+	struct hwrng *rng, *new_rng;
 
 	err = mutex_lock_interruptible(&rng_mutex);
 	if (err)
 		return -ERESTARTSYS;
 
-	old_rng = current_rng;
 	if (sysfs_streq(buf, "")) {
 		err = enable_best_rng();
 	} else {
@@ -362,11 +348,8 @@ static ssize_t rng_current_store(struct device *dev,
 	new_rng = get_current_rng_nolock();
 	mutex_unlock(&rng_mutex);
 
-	if (new_rng) {
-		if (new_rng != old_rng)
-			add_early_randomness(new_rng);
+	if (new_rng)
 		put_rng(new_rng);
-	}
 
 	return err ? : len;
 }
@@ -544,7 +527,6 @@ int hwrng_register(struct hwrng *rng)
 {
 	int err = -EINVAL;
 	struct hwrng *tmp;
-	bool is_new_current = false;
 
 	if (!rng->name || (!rng->data_read && !rng->read))
 		goto out;
@@ -573,25 +555,8 @@ int hwrng_register(struct hwrng *rng)
 		err = set_current_rng(rng);
 		if (err)
 			goto out_unlock;
-		/* to use current_rng in add_early_randomness() we need
-		 * to take a ref
-		 */
-		is_new_current = true;
-		kref_get(&rng->ref);
 	}
 	mutex_unlock(&rng_mutex);
-	if (is_new_current || !rng->init) {
-		/*
-		 * Use a new device's input to add some randomness to
-		 * the system.  If this rng device isn't going to be
-		 * used right away, its init function hasn't been
-		 * called yet by set_current_rng(); so only use the
-		 * randomness from devices that don't need an init callback
-		 */
-		add_early_randomness(rng);
-	}
-	if (is_new_current)
-		put_rng(rng);
 	return 0;
 out_unlock:
 	mutex_unlock(&rng_mutex);
@@ -602,12 +567,11 @@ EXPORT_SYMBOL_GPL(hwrng_register);
 
 void hwrng_unregister(struct hwrng *rng)
 {
-	struct hwrng *old_rng, *new_rng;
+	struct hwrng *new_rng;
 	int err;
 
 	mutex_lock(&rng_mutex);
 
-	old_rng = current_rng;
 	list_del(&rng->list);
 	complete_all(&rng->dying);
 	if (current_rng == rng) {
@@ -626,11 +590,8 @@ void hwrng_unregister(struct hwrng *rng)
 	} else
 		mutex_unlock(&rng_mutex);
 
-	if (new_rng) {
-		if (old_rng != new_rng)
-			add_early_randomness(new_rng);
+	if (new_rng)
 		put_rng(new_rng);
-	}
 
 	wait_for_completion(&rng->cleanup_done);
 }
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v3 PATCH] hwrng: core - Remove add_early_randomness

[Jarkko Sakkinen]

On Wed May 22, 2024 at 8:37 AM EEST, Herbert Xu wrote:
> On Tue, May 21, 2024 at 03:37:16PM -0400, Nícolas F. R. A. Prado wrote:
> >
> > FWIW this patch fixes the warning. So feel free to add
> > 
> > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>
> Could you please test this patch instead?
>
> ---8<---
> A potential deadlock was reported with the config file at
>
> https://web.archive.org/web/20240522052129/https://0x0.st/XPN_.txt
>
> In this particular configuration, the deadlock doesn't exist because
> the warning triggered at a point before modules were even available.
> However, the deadlock can be real because any module loaded would
> invoke async_synchronize_full.
>
> The issue is spurious for software crypto algorithms which aren't
> themselves involved in async probing.  However, it would be hard to
> avoid for a PCI crypto driver using async probing.
>
> In this particular call trace, the problem is easily avoided because
> the only reason the module is being requested during probing is the
> add_early_randomness call in the hwrng core.  This feature is
> vestigial since there is now a kernel thread dedicated to doing
> exactly this.
>
> So remove add_early_randomness as it is no longer needed.

"vestigial" did not know that word before ;-) Something learned.

What is the kthread doing this currently?

>
> Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Reported-by: Eric Biggers <ebiggers@kernel.org>
> Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to tpm2_get_random()")
> Link: https://lore.kernel.org/r/119dc5ed-f159-41be-9dda-1a056f29888d@notapiano/
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index f5c71a617a99..4084df65c9fa 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -64,19 +64,6 @@ static size_t rng_buffer_size(void)
>  	return RNG_BUFFER_SIZE;
>  }
>  
> -static void add_early_randomness(struct hwrng *rng)
> -{
> -	int bytes_read;
> -
> -	mutex_lock(&reading_mutex);
> -	bytes_read = rng_get_data(rng, rng_fillbuf, 32, 0);
> -	mutex_unlock(&reading_mutex);
> -	if (bytes_read > 0) {
> -		size_t entropy = bytes_read * 8 * rng->quality / 1024;
> -		add_hwgenerator_randomness(rng_fillbuf, bytes_read, entropy, false);
> -	}
> -}
> -
>  static inline void cleanup_rng(struct kref *kref)
>  {
>  	struct hwrng *rng = container_of(kref, struct hwrng, ref);
> @@ -340,13 +327,12 @@ static ssize_t rng_current_store(struct device *dev,
>  				 const char *buf, size_t len)
>  {
>  	int err;
> -	struct hwrng *rng, *old_rng, *new_rng;
> +	struct hwrng *rng, *new_rng;
>  
>  	err = mutex_lock_interruptible(&rng_mutex);
>  	if (err)
>  		return -ERESTARTSYS;
>  
> -	old_rng = current_rng;
>  	if (sysfs_streq(buf, "")) {
>  		err = enable_best_rng();
>  	} else {
> @@ -362,11 +348,8 @@ static ssize_t rng_current_store(struct device *dev,
>  	new_rng = get_current_rng_nolock();
>  	mutex_unlock(&rng_mutex);
>  
> -	if (new_rng) {
> -		if (new_rng != old_rng)
> -			add_early_randomness(new_rng);
> +	if (new_rng)
>  		put_rng(new_rng);
> -	}
>  
>  	return err ? : len;
>  }
> @@ -544,7 +527,6 @@ int hwrng_register(struct hwrng *rng)
>  {
>  	int err = -EINVAL;
>  	struct hwrng *tmp;
> -	bool is_new_current = false;
>  
>  	if (!rng->name || (!rng->data_read && !rng->read))
>  		goto out;
> @@ -573,25 +555,8 @@ int hwrng_register(struct hwrng *rng)
>  		err = set_current_rng(rng);
>  		if (err)
>  			goto out_unlock;
> -		/* to use current_rng in add_early_randomness() we need
> -		 * to take a ref
> -		 */
> -		is_new_current = true;
> -		kref_get(&rng->ref);
>  	}
>  	mutex_unlock(&rng_mutex);
> -	if (is_new_current || !rng->init) {
> -		/*
> -		 * Use a new device's input to add some randomness to
> -		 * the system.  If this rng device isn't going to be
> -		 * used right away, its init function hasn't been
> -		 * called yet by set_current_rng(); so only use the
> -		 * randomness from devices that don't need an init callback
> -		 */
> -		add_early_randomness(rng);
> -	}
> -	if (is_new_current)
> -		put_rng(rng);
>  	return 0;
>  out_unlock:
>  	mutex_unlock(&rng_mutex);
> @@ -602,12 +567,11 @@ EXPORT_SYMBOL_GPL(hwrng_register);
>  
>  void hwrng_unregister(struct hwrng *rng)
>  {
> -	struct hwrng *old_rng, *new_rng;
> +	struct hwrng *new_rng;
>  	int err;
>  
>  	mutex_lock(&rng_mutex);
>  
> -	old_rng = current_rng;
>  	list_del(&rng->list);
>  	complete_all(&rng->dying);
>  	if (current_rng == rng) {
> @@ -626,11 +590,8 @@ void hwrng_unregister(struct hwrng *rng)
>  	} else
>  		mutex_unlock(&rng_mutex);
>  
> -	if (new_rng) {
> -		if (old_rng != new_rng)
> -			add_early_randomness(new_rng);
> +	if (new_rng)
>  		put_rng(new_rng);
> -	}
>  
>  	wait_for_completion(&rng->cleanup_done);
>  }

I have no doubts that such thread would not exist, so:

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

BR, Jarkko

Re: [v3 PATCH] hwrng: core - Remove add_early_randomness

[Herbert Xu]

On Wed, May 22, 2024 at 02:51:36PM +0300, Jarkko Sakkinen wrote:
>
> What is the kthread doing this currently?

It's hwrng_fillfn.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v3 PATCH] hwrng: core - Remove add_early_randomness

[Nícolas F. R. A. Prado]

On Wed, May 22, 2024 at 01:37:54PM +0800, Herbert Xu wrote:
> On Tue, May 21, 2024 at 03:37:16PM -0400, Nícolas F. R. A. Prado wrote:
> >
> > FWIW this patch fixes the warning. So feel free to add
> > 
> > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> Could you please test this patch instead?
> 
> ---8<---
> A potential deadlock was reported with the config file at
> 
> https://web.archive.org/web/20240522052129/https://0x0.st/XPN_.txt
> 
> In this particular configuration, the deadlock doesn't exist because
> the warning triggered at a point before modules were even available.
> However, the deadlock can be real because any module loaded would
> invoke async_synchronize_full.
> 
> The issue is spurious for software crypto algorithms which aren't
> themselves involved in async probing.  However, it would be hard to
> avoid for a PCI crypto driver using async probing.
> 
> In this particular call trace, the problem is easily avoided because
> the only reason the module is being requested during probing is the
> add_early_randomness call in the hwrng core.  This feature is
> vestigial since there is now a kernel thread dedicated to doing
> exactly this.
> 
> So remove add_early_randomness as it is no longer needed.
> 
> Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Reported-by: Eric Biggers <ebiggers@kernel.org>
> Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to tpm2_get_random()")
> Link: https://lore.kernel.org/r/119dc5ed-f159-41be-9dda-1a056f29888d@notapiano/
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

This patch also fixes the warning.

Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Thanks,
Nícolas

Re: [v3 PATCH] hwrng: core - Remove add_early_randomness

[Linus Torvalds]

On Tue, 21 May 2024 at 22:38, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> In this particular configuration, the deadlock doesn't exist because
> the warning triggered at a point before modules were even available.
> However, the deadlock can be real because any module loaded would
> invoke async_synchronize_full.

I think this crapectomy is good regardless of any deadlock - the
"register this driver" should not just blindly call back into the
driver.

That said, looking at the code in question, there are other oddities
going on. Even the "we found a favorite new rng" case looks rather
strange. The thread we use - nice and asynchronous - seems to sleep
only if the randomness source is emptied.

What if you have a really good source of hw randomness? That looks
like a busy loop to me, but hopefully I'm missing something obvious.

So I think this hw_random code has other serious issues, and I get the
feeling there might be more code that needs looking at..

              Linus

Re: [v3 PATCH] hwrng: core - Remove add_early_randomness

[Herbert Xu]

On Wed, May 22, 2024 at 03:53:23PM -0700, Linus Torvalds wrote:
> 
> That said, looking at the code in question, there are other oddities
> going on. Even the "we found a favorite new rng" case looks rather
> strange. The thread we use - nice and asynchronous - seems to sleep
> only if the randomness source is emptied.
> 
> What if you have a really good source of hw randomness? That looks
> like a busy loop to me, but hopefully I'm missing something obvious.

Yes that does look strange.  So I dug up the original patch at

	https://lore.kernel.org/all/20140317165012.GC1763@lst.de/

and therein lies the answer.  It's relying on random.c to push back
when the amount of new entropy exceeds what it needs.  IOW we will
sleep via add_hwgenerator_randomness when random.c decides that
enough is enough.  In fact the rate is much less now compared to
when the patch was first applied.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v3 PATCH] hwrng: core - Remove add_early_randomness

[Jarkko Sakkinen]

On Thu May 23, 2024 at 7:49 AM EEST, Herbert Xu wrote:
> On Wed, May 22, 2024 at 03:53:23PM -0700, Linus Torvalds wrote:
> > 
> > That said, looking at the code in question, there are other oddities
> > going on. Even the "we found a favorite new rng" case looks rather
> > strange. The thread we use - nice and asynchronous - seems to sleep
> > only if the randomness source is emptied.
> > 
> > What if you have a really good source of hw randomness? That looks
> > like a busy loop to me, but hopefully I'm missing something obvious.
>
> Yes that does look strange.  So I dug up the original patch at
>
> 	https://lore.kernel.org/all/20140317165012.GC1763@lst.de/
>
> and therein lies the answer.  It's relying on random.c to push back
> when the amount of new entropy exceeds what it needs.  IOW we will
> sleep via add_hwgenerator_randomness when random.c decides that
> enough is enough.  In fact the rate is much less now compared to
> when the patch was first applied.

Just throwing something because came to mind, not a serious suggestion.

In crypto_larval_lookup I see statements like this:

	request_module("crypto-%s", name);

You could potentially bake up a section/table to vmlinux which would
have entries like:

	"module name", 1/0

'1' would mean built-in. Then for early randomness use only stuff
that is built-in.

Came to mind from arch/x86/realmode for which I baked in a table
for relocation (this was a collaborative work with H. Peter Anvin
in 2012 to make trampoline code relocatable but is still a legit
example to do such shenanigans in a subystem).

BR, Jarkko

Re: [v3 PATCH] hwrng: core - Remove add_early_randomness

[Herbert Xu]

On Thu, May 23, 2024 at 12:53:04PM +0300, Jarkko Sakkinen wrote:
>
> Just throwing something because came to mind, not a serious suggestion.
> 
> In crypto_larval_lookup I see statements like this:
> 
> 	request_module("crypto-%s", name);
> 
> You could potentially bake up a section/table to vmlinux which would
> have entries like:
> 
> 	"module name", 1/0
> 
> '1' would mean built-in. Then for early randomness use only stuff
> that is built-in.

This early random stuff is obsolete not just because we have a
kernel thread doing the same thing, but moreover random.c itself
has been modified so that it is no longer starved of entropy on
startup.  There is no reason to feed any early randomness.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v3 PATCH] hwrng: core - Remove add_early_randomness

[Jarkko Sakkinen]

On Thu May 23, 2024 at 12:58 PM EEST, Herbert Xu wrote:
> On Thu, May 23, 2024 at 12:53:04PM +0300, Jarkko Sakkinen wrote:
> >
> > Just throwing something because came to mind, not a serious suggestion.
> > 
> > In crypto_larval_lookup I see statements like this:
> > 
> > 	request_module("crypto-%s", name);
> > 
> > You could potentially bake up a section/table to vmlinux which would
> > have entries like:
> > 
> > 	"module name", 1/0
> > 
> > '1' would mean built-in. Then for early randomness use only stuff
> > that is built-in.
>
> This early random stuff is obsolete not just because we have a
> kernel thread doing the same thing, but moreover random.c itself
> has been modified so that it is no longer starved of entropy on
> startup.  There is no reason to feed any early randomness.

As a feature that would still sometimes nice to have. I've
sometimes wished there was "lsmod -b" or similar to display
built-in stuff ;-)

So overally I think it was good to have at least documented
here... 

BR, Jarkko

Re: [v3 PATCH] hwrng: core - Remove add_early_randomness

[Jarkko Sakkinen]

On Thu May 23, 2024 at 12:53 PM EEST, Jarkko Sakkinen wrote:
> On Thu May 23, 2024 at 7:49 AM EEST, Herbert Xu wrote:
> > On Wed, May 22, 2024 at 03:53:23PM -0700, Linus Torvalds wrote:
> > > 
> > > That said, looking at the code in question, there are other oddities
> > > going on. Even the "we found a favorite new rng" case looks rather
> > > strange. The thread we use - nice and asynchronous - seems to sleep
> > > only if the randomness source is emptied.
> > > 
> > > What if you have a really good source of hw randomness? That looks
> > > like a busy loop to me, but hopefully I'm missing something obvious.
> >
> > Yes that does look strange.  So I dug up the original patch at
> >
> > 	https://lore.kernel.org/all/20140317165012.GC1763@lst.de/
> >
> > and therein lies the answer.  It's relying on random.c to push back
> > when the amount of new entropy exceeds what it needs.  IOW we will
> > sleep via add_hwgenerator_randomness when random.c decides that
> > enough is enough.  In fact the rate is much less now compared to
> > when the patch was first applied.
>
> Just throwing something because came to mind, not a serious suggestion.
>
> In crypto_larval_lookup I see statements like this:
>
> 	request_module("crypto-%s", name);
>
> You could potentially bake up a section/table to vmlinux which would
> have entries like:
>
> 	"module name", 1/0
>
> '1' would mean built-in. Then for early randomness use only stuff
> that is built-in.
>
> Came to mind from arch/x86/realmode for which I baked in a table
> for relocation (this was a collaborative work with H. Peter Anvin
> in 2012 to make trampoline code relocatable but is still a legit
> example to do such shenanigans in a subystem).

This could be even parameter in __request_module() itself e.g.

int __request_module(bool wait, bool builtin_only, const char *fmt, ...);

And would not matter which initcall layer we are running at.

BR, Jarkko

Re: [v3 PATCH] hwrng: core - Remove add_early_randomness

[Torsten Duwe]

On Thu, 23 May 2024 12:49:50 +0800
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Wed, May 22, 2024 at 03:53:23PM -0700, Linus Torvalds wrote:
> > 
> > That said, looking at the code in question, there are other oddities
> > going on. Even the "we found a favorite new rng" case looks rather
> > strange. The thread we use - nice and asynchronous - seems to sleep
> > only if the randomness source is emptied.
> > 
> > What if you have a really good source of hw randomness? That looks
> > like a busy loop to me, but hopefully I'm missing something obvious.
> 
> Yes that does look strange.  So I dug up the original patch at
> 
> 	https://lore.kernel.org/all/20140317165012.GC1763@lst.de/
> 
> and therein lies the answer.  It's relying on random.c to push back
> when the amount of new entropy exceeds what it needs.  IOW we will
> sleep via add_hwgenerator_randomness when random.c decides that
> enough is enough.

Yes, I thought that this was the obvious choice, the lesser evil. If it
just discarded the excess and returned immediately I had expected some
kernel threads to spin constantly.

>  In fact the rate is much less now compared to
> when the patch was first applied.

You mean the rate of required entropy? Doesn't that make things worse?
Maybe redesign the API to use a pull scheme? RNGs register at the
randomness facility with a callback that can be used when there is a
need for fresh entropy?

	Torsten

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

[Jarkko Sakkinen]

On Sat May 18, 2024 at 7:31 AM EEST, Eric Biggers wrote:
> 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.

Issues:

- IMA extends PCR's. This requires encrypted communications path.
- HWRNG uses auth session (see tpm2_get_radom()).
- TPM trusted keys

Null key is required before any other legit use in initialization.

Even something like 

--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -36,6 +36,8 @@ config TCG_TPM2_HMAC
        bool "Use HMAC and encrypted transactions on the TPM bus"
        default y
+       select CRYPTO_DRBG
        select CRYPTO_ECDH
+       select CRYPTO_HMAC
+       select CRYPTO_SHA512
        select CRYPTO_LIB_AESCFB
        select CRYPTO_LIB_SHA256
        help

would be more decent.

>
> - Eric

BR, Jarkko

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

[Herbert Xu]

On Sat, May 18, 2024 at 01:56:44PM +0300, Jarkko Sakkinen wrote:
>
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -36,6 +36,8 @@ config TCG_TPM2_HMAC
>         bool "Use HMAC and encrypted transactions on the TPM bus"
>         default y
> +       select CRYPTO_DRBG
>         select CRYPTO_ECDH
> +       select CRYPTO_HMAC
> +       select CRYPTO_SHA512
>         select CRYPTO_LIB_AESCFB
>         select CRYPTO_LIB_SHA256
>         help

This isn't necessary because ECDH selects ECC which already
selects the DRBG and all the required algorithms.

You can verify this with the failing .config file as it has
everything needed built into the kernel (which is in fact
where the problem is).

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt